New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix KernelTestCase compatibility for PhpUnit 8 #30084

Merged
merged 1 commit into from Feb 8, 2019

Conversation

Projects
None yet
5 participants
@alexander-schranz
Copy link
Contributor

alexander-schranz commented Feb 5, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30071
License MIT
Doc PR

As the PhpUnit 8 Testcase has different return types as PhpUnit 7 there need to be 2 different classes to support both PhpUnit 8 and PhpUnit 7. With a class alias we can then change which version is used based on the PhpUnit Version constant. The fix is a little bit hacky but to support different major versions I see no other way.

Not sure as we can't upgrade symfony/symfony to PhpUnit 8 how we can create a TestCase for this change.

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:feature/phpunit-8-compatibility branch 2 times, most recently from 203cf61 to ddf2a25 Feb 5, 2019

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:feature/phpunit-8-compatibility branch from ddf2a25 to e216c0d Feb 5, 2019

@alexander-schranz alexander-schranz changed the title Add compatibility for phpunit 8 Fix compatibility for phpunit 8 Feb 5, 2019

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:feature/phpunit-8-compatibility branch 5 times, most recently from 9850f7d to 254cd15 Feb 5, 2019

@alexander-schranz alexander-schranz changed the title Fix compatibility for phpunit 8 Fix compatibility for phpunit 8 for KernelTestCase Feb 5, 2019

@alexander-schranz alexander-schranz changed the title Fix compatibility for phpunit 8 for KernelTestCase Fix KernelTestCase compatibility for phpunit 8 Feb 5, 2019

@alexander-schranz alexander-schranz changed the title Fix KernelTestCase compatibility for phpunit 8 Fix KernelTestCase compatibility for PhpUnit 8 Feb 6, 2019

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:feature/phpunit-8-compatibility branch 2 times, most recently from d26f2bf to ff5de16 Feb 6, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2019

Instead of doing this change, would adding the @after annotation on ensureKernelShutdown work?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 7, 2019

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:feature/phpunit-8-compatibility branch from ff5de16 to 83a56a0 Feb 7, 2019

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

alexander-schranz commented Feb 7, 2019

@nicolas-grekas sounds like a good idea! Did change it.

@@ -208,6 +208,8 @@ protected static function createKernel(array $options = [])
}
/**
* @after
*
* Shuts the kernel down if it was used in the test.
*/
protected static function ensureKernelShutdown()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 7, 2019

Member

could you please confirm it works even if this method is protected?

This comment has been minimized.

@alexander-schranz

alexander-schranz Feb 7, 2019

Author Contributor

Can confirm it was called in a simple test extending the KernelTestCase.

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

There is potential BC break, if ppl call parent::tearDown(). We could work around by implementing __call(). Not sure it's worth it, wdyt?

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

alexander-schranz commented Feb 7, 2019

@nicolas-grekas tried it but __call seems never be called when parent::tearDown() is called it seems directly calling the PhpUnit teardown. (https://wtools.io/php-sandbox/6y)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2019

it seems directly calling the PhpUnit teardown

all is fine then, great!

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

alexander-schranz commented Feb 7, 2019

@nicolas-grekas and the @after seems to be called after the tearDown so there should be no problems if the kernel is still needed in the tearDown.

@nicolas-grekas nicolas-grekas merged commit 83a56a0 into symfony:3.4 Feb 8, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 8, 2019

bug #30084 Fix KernelTestCase compatibility for PhpUnit 8 (alexander-…
…schranz)

This PR was merged into the 3.4 branch.

Discussion
----------

Fix KernelTestCase compatibility for PhpUnit 8

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  |no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30071
| License       | MIT
| Doc PR        |

As the PhpUnit 8 Testcase has different return types as PhpUnit 7 there need to be 2 different classes to support both PhpUnit 8 and PhpUnit 7. With a class alias we can then change which version is used based on the PhpUnit Version constant. The fix is a little bit hacky but to support different major versions I see no other way.

Not sure as we can't upgrade symfony/symfony to PhpUnit 8 how we can create a TestCase for this change.

Commits
-------

83a56a0 Fix phpunit 8 compatibility

@alexander-schranz alexander-schranz deleted the alexander-schranz:feature/phpunit-8-compatibility branch Feb 8, 2019

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 8, 2019

This is a huge BC break and must be reverted. It's common practice to overwrite tearDown to not shut down the kernel. This won't work anymore with this change.

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Feb 9, 2019

Revert "bug symfony#30084 Fix KernelTestCase compatibility for PhpUni…
…t 8 (alexander-schranz)"

This reverts commit a36dc51, reversing
changes made to db445c4.
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 9, 2019

:( thanks for the review.
Here is the follow up: #30124

fabpot added a commit that referenced this pull request Feb 12, 2019

bug #30124 Fix KernelTestCase compatibility for PhpUnit 8 (bis) (nico…
…las-grekas)

This PR was squashed before being merged into the 3.4 branch (closes #30124).

Discussion
----------

Fix KernelTestCase compatibility for PhpUnit 8 (bis)

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Follow up of #30084 and @Tobion's comment there.

Fabbot failure is a false-positive.

Commits
-------

1077df6 Fix KernelTestCase compatibility for PhpUnit 8 (bis)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment