Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Ensure that PHPUnit's error handler is still working in isolated tests#24575
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
53473e9 to6a63b3fCompare| * Note that for the deprecation handler to work in a separate process we need to disable the preservation of global | ||
| * state. This is because composer's autoloader stores which files have been autoloaded in the global | ||
| * '__composer_autoload_files'. If this is preserved then bootstrap.php will not run again meaning that deprecations | ||
| * won't be collected. |
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.
Wouldn't it be enough to reset the autoloaded state then?
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.
For the use case of deprecating an interface (and other scenarios), you'd add@trigger_error() after thenamespace declaration. That means the error will be triggered on discovery and then never repeated, for a false positive.
Disabling@preserveGlobalState here (on a test) implies to me that for some process-isolated tests to reflect all deprecations, they should have this same annotation, so a documentation update would be needed, and/or the test listener needs to set this value on isolated tests.
Alternately, merge deprecations from discovery and parent process with the ones from the child process.
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.
@xabbuh I'm not sure when we'd get a chance to do that? We need bootstrap.php to be loaded before the test is run in isolation.
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.
Worked out something about the failing tests on travis for 7.2 - it's because of something in src/Symfony/Bridge/PhpUnit/bin/simple-phpunit - i've downloaded the travis docker image and replayed the build script. Running phpunit via that script it fails. Running vanilla phpunit it passes.
alexpott commentedOct 18, 2017
So this issue starts with not knowing how to run unit tests for Symfony. My steps to reproduce the issue this is trying to fix:
Nothing fails!!! However now I understand that to run phpunit tests on Symfony you need to
In this instance the test fails as expected. However as the travis tests show we still have a problem in isolated tests because the other warnings are not being converted into exceptions by PHPUnit's error handler as expected. This is because even though the PR makes the necessary changes to \Symfony\Bridge\PhpUnit\DeprecationErrorHandler::collectDeprecations() to make this happen the version that is autoload is the one installed in /vendor by the composer install. And not changed one in the local repo. |
alexpott commentedOct 18, 2017
I've updated the PR to only make the necessary changes to fix the fact that \Symfony\Bridge\PhpUnit\DeprecationErrorHandler::collectDeprecations() can result in not having PHPUnit's error handler in isolated tests. This will still fail the travis tests because the way that phpunit is built in Travis results in using a stale version of the phpunit bridge code because it uses composer to get it. |
| } | ||
| $deprecations[] =array(error_reporting(),$msg); | ||
| }); | ||
| // This can registered before the PHPUnit error handler. |
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.
typo: missing "be" (... can be...
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.
Fixed in latest push - I did a rebase to get the latest changes so had to force push.
511faf9 to1fb801aComparenicolas-grekas commentedOct 24, 2017
Thank you@alexpott. |
This PR fixes the \Symfony\Bridge\PhpUnit\Tests\ProcessIsolationTest and adds new coverage to ensure PHPUnit error handling works as expected. Tested with both PHPUnit 4.8.35 and 6.2.4