-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
Support PHPUnit 10 #589
Support PHPUnit 10 #589
Conversation
…his is a temporary fix state where we have dropped phpunit-bridge in favor of phpunit/phpunit, since the bridge is not yet 10 compliant; there is a chance we will also want to do a bit of file restructuring/reorganization but I will wait to see what the maintainers think about that
I am not sure how to address the failures above, if I could have guidance on that as well. I made sure PHPStan was clean (is there a reason it's not included as a dev package?). Also, if there a reason your coding stanrad tool is not a dev package? What should I install to lint locally? It looks like there is an issue with supporting php 8.0... maybe the new Again, guidance would be much appreciated. |
composer.json
Outdated
"ext-dom": "*", | ||
"ext-libxml": "*", | ||
"php-webdriver/webdriver": "^1.8.2", | ||
"phpstan/phpstan": "^1.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be removed (PHPStan is not a hard dependency of this project).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to dev dependencies: arderyp@5cf344a
@dunglas resolve if this is sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe you don't even want it in the dev dependencies? If so, I can delete this and php-cs-fixer
from dev dependencies, just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please delete it. The bot uses PHP CS Fixer under the hood but we don't need to install it locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've deleted phpstan and php-cs-fixer from dev dependencies: arderyp@cdcd7e0
That said, this is a confusing decision to me. Presumably, we should all be running phpstan before committing and only commit non-failing code. If so, why not keep phpstan as a dev dependency? Otherwise devs need to know to manually install it, then run it, then remember to manually delete it, then do it all over again for each commit. Furthermore, having it as a dev dependency doesn't hurt anything (or maybe I'm wrong?).
Likewise, on my initial PR, one of the Github actions failed for fixer related issues. If contributors are supposed to run the fixer themselves manually before committing, then it's the same weird expectation on dev to know they need to install it manually, run it, then remove it before commit, then do it all over again for the next commit.
EDIT: I just had to re-install-temporarily php-cs-fixer to resolver the failing GitHub action. I'm still perplexed with this workflow. I think it should either be a dev dependency (doesn't hurt anything), or the GitHub action should use --dry-run
and should actually clean the commit (this may be less ideal because then the dev needs to notice that and pull those changes into their local).
@dunglas resolve if we don't need to run fixer/phpstan manually before committing (or we do and you still think they shouldn't be a dev dependencies for some reason)
composer.json
Outdated
@@ -44,9 +45,9 @@ | |||
"sort-packages": true | |||
}, | |||
"require-dev": { | |||
"phpunit/phpunit": "^10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest keeping symfony/phpunit-bridge
for now, but creating a temporary GitHub Actions workflow that will execute composer remove symfony/phpunit-bridge
and then composer require phpunit/phpunit
.
So we can test Panther using the bridge (which is necessary to detect deprecations), as well as with PHPUnit 10.
We'll remove this temporary workflow when PHPUnit Bridge will support PHPUnit 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dunglas I can do this, but the only problem is that it prevents me from being able to run tests locally. I can remove phpunit/phpunit
and require dev symfony/phpunit-bridge
and use <server name="SYMFONY_PHPUNIT_VERSION" value="10.0"/>
in phpunit.xml.dist
but I cannot run vendor/bin/simple-phpunit
because the bridge doesn't support phpunit 10 yet:
PHP Fatal error: Uncaught Error: Class "PHPUnit\TextUI\Command" not found in /home/arderyp/panther/vendor/symfony/phpunit-bridge/Legacy/CommandForV9.php:25
Stack trace:
#0 /home/arderyp/panther/vendor/bin/.phpunit/phpunit-10.0-0/vendor/composer/ClassLoader.php(571): include()
#1 /home/arderyp/panther/vendor/bin/.phpunit/phpunit-10.0-0/vendor/composer/ClassLoader.php(428): Composer\Autoload\includeFile()
#2 [internal function]: Composer\Autoload\ClassLoader->loadClass()
#3 /home/arderyp/panther/vendor/symfony/phpunit-bridge/TextUI/Command.php(17): class_alias()
#4 /home/arderyp/panther/vendor/bin/.phpunit/phpunit-10.0-0/vendor/composer/ClassLoader.php(571): include('...')
#5 /home/arderyp/panther/vendor/bin/.phpunit/phpunit-10.0-0/vendor/composer/ClassLoader.php(428): Composer\Autoload\includeFile()
#6 /home/arderyp/panther/vendor/bin/.phpunit/phpunit-10.0-0/phpunit(22): Composer\Autoload\ClassLoader->loadClass()
#7 /home/arderyp/panther/vendor/symfony/phpunit-bridge/bin/simple-phpunit.php(441): include('...')
#8 /home/arderyp/panther/vendor/symfony/phpunit-bridge/bin/simple-phpunit(13): require('...')
#9 /home/arderyp/panther/vendor/bin/simple-phpunit(120): include('...')
#10 {main}
thrown in /home/arderyp/panther/vendor/symfony/phpunit-bridge/Legacy/CommandForV9.php on line 25
furthermore, I haven't tested yet, but this would probably prevent me from actually being able to use Panther with PHPUnit 10. Does what I'm saying make sense, or do you think I am misunderstanding something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo test locally with PHPUnit 10, you'll have to do the same steps as in the CI (then revert the changes in composer.json
before committing).
This is definitely not ideal, but quite common an only temporary. We'll be able to remove this hack when PHPUnit Bridge will support PHPUnit 10.
As PHPUnit is not a hard dependency of Panther, this will not prevent users to use Panther with PHPUnit 10 in their own projects.
TL;DR: restore the content of Panther's composer.json
and change it only in a specific CI workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info.
I am inexperienced with GitHub workflows. Should I be looking into changing our github workflow somehow, or are you just saying "revert back to phpunit-bridge, which will allow Github workflow to work properly, and just manually add/remove phpunit for my own testing locally"?
I am swamped tomorrow but hopefully can carve out some time in the afternoon, otherwise I can look on Friday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: arderyp@cdcd7e0
@dunglas please resolve if sufficient
phpstan.neon
Outdated
@@ -3,8 +3,9 @@ parameters: | |||
paths: | |||
- src | |||
- tests | |||
bootstrapFiles: | |||
- vendor/bin/.phpunit/phpunit/vendor/autoload.php | |||
# @todo use below if/when we are back on symfony/phpunit-bridge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the strategy previously mentioned, we can revert the changes in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: arderyp@3c0c3f6
@dunglas please resolve if sufficient
phpunit.xml.dist
Outdated
@@ -2,30 +2,34 @@ | |||
|
|||
<!-- http://phpunit.de/manual/4.1/en/appendixes.configuration.html --> | |||
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the changes in this file mandatory? If we can keep it "as-is", we ensure that the project still work as expected with older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are necessary. I have converted our own testsuite to be PHPUnit 10 compatible (there are BC breaks).
I don't think there is any problem in our project requiring a dev dependency explicitly of phpunit 10, as our tests have been updated and now require 10. since it is a dev dependency and not a hard dependency, consumers can pull in Panther and use whatever PHPUnit they provide in their project's requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an optional dependency, but not only a dev one. We must ensure in our CI that Panther still run on projects using PHPUnit <10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how you want to proceed here. In order to make Panther's internal tests phpunit 10 compliant, I had to make various changes you'll see to the test files. additionally, I had to migrate to phpunit.xml.dist
structure/formatting/config.
I am not immediately certain how we can make our own internal test suite runnable on 10 and >10. It looks like the doctrine-test-bundle
created two different phpunit.xml
files, one for <10 and one for 10, so maybe there is a solution somehow: https://github.com/dmaicher/doctrine-test-bundle/tree/master/tests
In short, I'm not sure what you are asking here... do you want me to revert the changes to phpunit.xml.dist
and then roll back all the PHPUnit 10 specific changes to our internal test files (DataProvider
attribute, etc)? Or so I need to figure out how our internal testsuite can pass both PHPUnit8/9 and also PHPUnit10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not possible to use a PHPUnit configuration that is compatible with both PHPUnit 10 and previous versions, then yes having two files will be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two files thing isn't really the problem, that's the easy part. The potential problem is having internal tests that are compatible with 10 and <10. But doctrine-test-bundle seems to be doing it, so I will give it a go and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before wasting your time, I would like to hear what's @nicolas-grekas opinion on this.
Do we need to do so much efforts to support old PHPUnit versions, or can we expect a PHPUnit Bridge version compatible with PHPUnit soon and just release a new major?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, may not be worth wasting his time. I spoke before testing. I think I have this working. Commit incoming...
EDIT: on re-reading you, I am very much interested in hearing what @nicolas-grekas has to say about supporting old PHPVersions and a potential new major to phpunit-bridge that requires phpunit >=10 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: arderyp@3c0c3f6
there is a new phpunit.xml.dist.10
file for usage in PHPUnit 10 GitHub Action
@dunglas please resolve if sufficient
src/PantherTestCaseTrait.php
Outdated
@@ -129,14 +128,19 @@ public static function isWebServerStarted(): bool | |||
|
|||
public function takeScreenshotIfTestFailed(): void | |||
{ | |||
if (!\in_array($this->getStatus(), [BaseTestRunner::STATUS_ERROR, BaseTestRunner::STATUS_FAILURE], true)) { | |||
// provided by PHPUnit\Framework\TestCase | |||
if (!method_exists($this, 'status')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you adapt this to support both PHPUnit 10 and older versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: arderyp@e7b3520
@dunglas resolve if this is sufficient
src/PantherTestCaseTrait.php
Outdated
@@ -239,4 +243,9 @@ private static function getWebServerDir(array $options): string | |||
|
|||
return $_SERVER['PANTHER_WEB_SERVER_DIR']; | |||
} | |||
|
|||
private static function isGetClientStaticMethodAvailable(): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't look necessary. Can you please keep the diff as small as possible to help the review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert this, but there will still be associated changes, as I replaced \is_callable()
with method_exists()
in order to resolve 2 pre-existing phpstan errors that were/are present on symfony/panther:main
But if you want me to remove the function and deal instead with the two line changes instead, or accept the phpstan errors in which case I can fully refert to \is_callable()
. Let me know @dunglas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this new method, but the remaining \is_callable()
to method_exists()
change remains to solve 2 phpstan errors.
@dunglas please resolve if sufficient
Thanks for working on this!
|
I ran the fixer. I also added |
…nit; moving phpstan to dev dependencies
okay @dunglas, I've addressed what I can and need more feedback where noted. Please let me know. Also, is there a reason we are excluding |
It's a best practice for libraries. |
composer.json
Outdated
@@ -44,9 +45,9 @@ | |||
"sort-packages": true | |||
}, | |||
"require-dev": { | |||
"phpunit/phpunit": "^10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo test locally with PHPUnit 10, you'll have to do the same steps as in the CI (then revert the changes in composer.json
before committing).
This is definitely not ideal, but quite common an only temporary. We'll be able to remove this hack when PHPUnit Bridge will support PHPUnit 10.
As PHPUnit is not a hard dependency of Panther, this will not prevent users to use Panther with PHPUnit 10 in their own projects.
TL;DR: restore the content of Panther's composer.json
and change it only in a specific CI workflow.
phpunit.xml.dist
Outdated
@@ -2,30 +2,34 @@ | |||
|
|||
<!-- http://phpunit.de/manual/4.1/en/appendixes.configuration.html --> | |||
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an optional dependency, but not only a dev one. We must ensure in our CI that Panther still run on projects using PHPUnit <10.
…ting most of the tests changes; restoring phpstan.neon; adding new phpunit.xml.dist.10 file for usage with phpunit 10; tests passing on phpunit-bridge with phpunit.xml.dist and phpstan 10 with phpunit.xml.dist.10
okay @dunglas, lots of reversions in arderyp@3c0c3f6 which reduced the file change set by over 33%. I think I've addressed all of your comments above. It would be helpful to de-clutter the space if you could just review all of my latest comments and resolve everything that you consider sufficient. If there are any new things to discuss, I think we should probably resolve everything we have first before diving into new items. Our internal tests are passing on I've also pulled this branch into my own project, which is running phpunit 10, and panther is working 🙏 One thing to note, which isn't necessarily relevant and may not be important, but EDIT: this may actually be relevant, as I believe the GitHub Action is failing intermittently on this (example) and potentially others. Some of my commits pass phpunit tests in GitHub Actions, and some done, without any meaningful code changes between (like a change to composer.json, for example). The latest failure is due to the driver port being already in use. It the phpunit tests are run in parallel, they should probably each use a different port, if they don't already. EDIT 2: I've documented the inconsistent behavior and added a workaround to skip the problematic assertion if the html is empty. I suspect you won't like this approach, but I've been debugging for 2 hours and can't determine what the source of the issue is, I can revert the change if you want, but don't know how else to address the fact that the test works sometimes and fails others with no clear pattern and no different test state between success and failure. There was an interesting GitHub Action failure just now, emitting from the workaround linked above, that may provide some context to help you debug further (https://github.com/symfony/panther/actions/runs/4327785335/jobs/7556809373) |
hey @dunglas, I feel like we had some traction last week and managed to get some good work done. I think this is mostly ready to go, once you review. I'd love to get back on it, if you've got the time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
@dunglas I committed your suggestions, but the change to arderyp@e508f0c are we good to merge and release? Do the GitHub actions need to be updated to test phpunit 10 using the workaround you proposed here--it looks like they are still just running |
Yes, please create new GHA workflow to run the test with PHPUnit 10, then we'll able to merge. @symfony/mergers could you take a look? |
@dunglas I will look at this next week. Should I be duplicating eact phpunit workflow in |
Just running the tests on 8.1 with PHPUnit 10 is good enough. |
Yes |
… and phpunit 10 (in place of symfony/phpunit-bridge, which is not yet phpunit 10 compliant)
@dunglas @symfony/mergers I think we are good to go here 😃 php8.1/phpunit10 test coverage added to new GitHub Action Workflow here (and passing): arderyp@48de639 |
hey @dunglas, this has been approved for over a week. What's the next step to get it merged? |
@dunglas I would appreciate a lot if this PR is merged soon for update several github actions pipelines to PHPUnit 10. Thanks! |
hey @dunglas. It's been many weeks. How can we get the ball rolling on a merge? Who can I @ to get you what you need? |
Thank you @arderyp, sorry for the long delay. |
THANK YOU @dunglas! Is there any chance we will get a new release soon that includes this? |
Yes! I'm on it. |
Released! |
issue: #588
This PR may not be ready yet. I am submitting it for review/guidance on the following:
Maintain dependence on PHPUnit Bridge?
This PR does indeed allow Panther to work under PHPUnit 10. It should preserve backwards compatibility and allow those using PHPUnit 8 and 9 to also continue using Panther without issue. But, in order to support 10, I had to (temporarily?) drop
symfony/phpunit-bridge
in favor ofphpunit/phpunit
, since the bridge is not yet PHPUnit 10 compatible. Obviously, we could hold off on merging this PR until the bridge is ready, at which point I would drop the direct PHPUnit dependency and put the bridge dependency back in place pre-merge. But this raises the question, do we actually want/need to depend onsymfony/phpunit-bridge
here, or can we let the consumer decide, and perhaps they only want/usephpunit/phpunit
?I could very well be missing something, but I'm not currently seeing any specific reason to depend on the bridge over PHPUnit directly--we aren't using any of it's code, its blocking us here and will continue to do so in the future, and depending on PHPUnit directly will leave it up to the consumer to decide for themself whether they want PHPUnit directly or the bridge or
symfony/test-pack
or something else.Maintain support for PHPUnit <10?
We could make this a new major release (
v3.0.0
) and simply require 10+. There are no actual changes to the package between the latest2.x
and this, so people on lower versions of PHPUnit could be directed to usev2.x
and phpunit 10 users could be directed to usev3.x
. Furthermore @dunglas has noted that the package is stable and relatively unchanging at this point, so if we imagine continuing that trend, then it could be fine to drop <10 support in a newv3.x
release.If you all are okay with dropping <10 support and cutting a new release, then I should clean some stuff up here. But if you want to continue supporting multiple versions, what we have here should suffice. It's messier and less simple/straight forward than it would be without supporting multiple versions, but I guess that's par for the course with third party packages.
Restructuring
What I have here is the simplest "make it work" approach following Panther's current approach, and borrowing influence from doctrine-test-bundle's approach to solving this problem. I didn't want to get crazy and start restructuring things only for you all to not accept that. The current structure I'd like to clean up is as follows:
I'd like to change it to:
If we drop PHPUnit <10 support, then it would be a cleaner and simpler:
These would all be very small and clean classes.
Both restructures would be BC breaks, as it would require updating the namespace of the extension referenced in
phpunit.xml
. We could avoid BC break by creating a class alias, but I don't think it's worth doing that. It's extra clutter that's not very useful and fixing thew BC break would be very easy for everyone.I look forward to hearing what you think.