Skip to content
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

Prep work to migrate to PHPUnit 9.x #5269

Merged
merged 1 commit into from Dec 1, 2020

Conversation

sanmai
Copy link
Contributor

@sanmai sanmai commented Nov 14, 2020

This is a fully backward-compatible change.

The general idea is to make the package testable under PHP 5.6 - 7.x - 8.0. This requires using both PHPUnit 5.x and 9.x, because only the last works correctly under PHP 8 (as is - collects coverage report).

But PHPUnit 9 has quite a lot of legacy methods removed, and PHPUnit 8 has these methods but with void return type. Therefore implementing these methods straight away is a no-go. This can be with a version-dependent trait, but for a small handful of functions used here it easier to do this by overloading them from __call and __callStatic. This is where DeprecatedTestMethods trait comes into play.

And there are also setUp(): void etc. which are not taken care in this PR.

  • There's also above-mentioned PolyfillAssertTrait from symfony/phpunit-bridge, but it is tagged @internal so it's a risky game to use it.
  • And there's also phpunitgoodpractices/polyfill, but it should be trivial to switch to any of those down the line.

Extracted from #5262 (it handles setUp(): void among other things)

@sanmai sanmai changed the base branch from 2.16 to 2.15 November 14, 2020 14:09
@sanmai sanmai changed the title Prep work to migrate to PHPUnit 8 Prep work to migrate to PHPUnit 9.x Nov 14, 2020
@sanmai sanmai mentioned this pull request Nov 14, 2020
8 tasks
@sanmai sanmai marked this pull request as ready for review November 15, 2020 04:11
phpstan.neon Outdated Show resolved Hide resolved
phpstan.neon Outdated Show resolved Hide resolved
@SpacePossum SpacePossum added this to the 2.15.10 milestone Nov 16, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Nov 16, 2020
@sanmai
Copy link
Contributor Author

sanmai commented Nov 16, 2020

@keradus should it be more appropriate to use phpunitgoodpractices/polyfill here instead of a purpose-build short polyfill?

@SpacePossum
Copy link
Contributor

utilizing phpunitgoodpractices/polyfill would be great as more people found benefit from your work : )

(also we would have less maintenance in this project so I'm biased ;) )

@sanmai
Copy link
Contributor Author

sanmai commented Nov 16, 2020

Good point. Give me a minute to sort this out.

@sanmai
Copy link
Contributor Author

sanmai commented Nov 16, 2020

Should I add phpunitgoodpractices/polyfill as a direct dependency?

@keradus
Copy link
Member

keradus commented Nov 16, 2020

👍
feel free to raise PRs to that polyfill repo too, don't hesitate to reach out to me over https://gitter.im/keradus

@sanmai
Copy link
Contributor Author

sanmai commented Nov 16, 2020

@keradus DM'ing you there!

@sanmai
Copy link
Contributor Author

sanmai commented Nov 16, 2020

As of now phpunitgoodpractices/polyfill has none of the methods we need here. First PR for phpunitgoodpractices/polyfill is go PHPUnitGoodPractices/polyfill#16. Once there's a CI, I'll add other necessary methods.

Meanwhile this is ready for merge. Migrating to phpunitgoodpractices/polyfill will be as simple as removing one file and changing one line.

This is a blocker to #5262. Reviewing and merging this will let me to continue my work on #5262. Please consider this.

@@ -23,7 +23,7 @@ $config = PhpCsFixer\Config::create()
->setRiskyAllowed(true)
->setRules([
'@PHP56Migration' => true,
'@PHPUnit60Migration:risky' => true,
'@PHPUnit75Migration:risky' => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@keradus keradus modified the milestones: 2.15.10, 2.16.8 Nov 20, 2020
@keradus keradus changed the base branch from 2.15 to 2.16 November 20, 2020 13:52
@GrahamCampbell
Copy link
Contributor

What is the reason for the re-target?

@GrahamCampbell
Copy link
Contributor

Oh, I see. The last 2.15.x release has just happened?

@GrahamCampbell
Copy link
Contributor

So, what is the status with phpunitgoodpractices/polyfill? I think this is the only outstanding thing blocking merge now. The rest looks great, from what I can see.

@sanmai
Copy link
Contributor Author

sanmai commented Nov 20, 2020

@GrahamCampbell what we're doing there is fairly tricky, and, considering that @keradus is, understandably, looking for an uncompromising quality, at these circumstances I'd like to see it getting as thorough review as possible. Therefore, we have to wait for it to happen.

Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one ;)

@sanmai
Copy link
Contributor Author

sanmai commented Nov 23, 2020

Do I need to squash-rebase it? It's not a big deal, but squash merging will be easier.

@keradus
Copy link
Member

keradus commented Nov 23, 2020

squash doesn't matter.
i only await for CI to confirm it works on all supported combinations of phpunit

@sanmai sanmai force-pushed the pr/2.15/future-phpunit branch 2 times, most recently from acec576 to cd278fb Compare November 26, 2020 22:56
@sanmai sanmai mentioned this pull request Nov 27, 2020
@GrahamCampbell
Copy link
Contributor

This can be (squash) merged now, I think?

@keradus
Copy link
Member

keradus commented Nov 29, 2020

easy @GrahamCampbell , it will be merged when it will be merged. no need for pinging, especially when you see movement around related changes

@GrahamCampbell
Copy link
Contributor

I wasn't rushing. I was just re-approving after the changes you just made. :)

@keradus
Copy link
Member

keradus commented Dec 1, 2020

Thank you @sanmai.

@keradus keradus merged commit 694dfb7 into PHP-CS-Fixer:2.16 Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants