Convert some unit tests to integration tests #6820
Conversation
morozov
force-pushed
the
alter-table-integration-testing
branch
from
e7837c9 to
31849c1
Compare
morozov
changed the title
|
|
||
| class AlterTableTest extends FunctionalTestCase | ||
| { | ||
| /** @throws Exception */ |
There was a problem hiding this comment.
What's the point of this annotation?
There was a problem hiding this comment.
@throws annotations on unit tests don't make much sense. Noboby's going to catch them except for PHPUnit itself. We should remove them.
There was a problem hiding this comment.
What's the point of this annotation?
Normally, it shows that a method throws a checked exception, which the caller should either catch or declare as @throws in its own signature.
IIRC, PhpStorm by default considers all exceptions that don't extend RuntimeException or LogicException checked (I'll add the screenshot just for reference, we don't design our code for PhpStorm).
If this annotation is removed, it starts underlining the methods that throw an unhandled checked exception.
I don't think this annotation makes a lot of sense for the readers of this code because this method is not invoked explicitly. However it makes PhpStorm happier. Maybe instead of adding these annotations on test methods, I could suppress this inspection for tests in PhpStorm.
There was a problem hiding this comment.
Yes… maybe this could even be reported to PHPStorm, I guess they must have some kind of PHPUnit integration.
There was a problem hiding this comment.
I'm generally not happy with the way PhpStorm deals with exceptions, but I guess that's what we get when Java people build a PHP editor. 😉 This inspection is more or less the first thing I deactivate or disable the highlighting for when configuring PhpStorm for a new project.
| } | ||
|
|
||
| /** @throws Exception */ | ||
| private function testMigration(Table $oldTable, callable $migration): void |
There was a problem hiding this comment.
We should not use the test prefix on methods that we don't want PHPUnit to pick up as tests. I am aware that PHPUnit will skip the method because it's private, but still: we can probably find a better name.
There was a problem hiding this comment.
I don't think using the test prefix is a problem – right because PHPUnit will skip it, and in this specific case, the method does contain the body of the test (so the name is appropriate). I'm open to suggestions on renaming the method.
There was a problem hiding this comment.
I'm going to merge as it's not a blocker. Happy to rename this one and potentially other similar methods in a separate PR.
morozov
force-pushed
the
alter-table-integration-testing
branch
from
31849c1 to
90e365c
Compare
The new test is loaded with exceptions but it is what it is. We may address the underlying issues separately.
The code coverage has slightly dropped, which I anticipated. The original unit tests produced coverage for the code that generates DDL for MySQL, but didn't validate its correctness. Now these code paths may be not covered, because they are incorrect and would fail the test (see the tests marked as incomplete on MySQL).
We have to skip the majority of the tests on the majority of the platforms because DBAL doesn't support dropping primary key constraints there (the same issue is documented #6819).