Ensure that PHPUnit's error handler is still working in isolated tests #24575
Conversation
53473e9 to
6a63b3f
Compare
| * 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.
Wouldn't it be enough to reset the autoloaded state then?
There was a problem hiding this comment.
For the use case of deprecating an interface (and other scenarios), you'd add @trigger_error() after the namespace 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.
@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.
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.
|
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. |
|
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. |
| @@ -247,6 +247,12 @@ public static function collectDeprecations($outputFile) | |||
| } | |||
| $deprecations[] = array(error_reporting(), $msg); | |||
| }); | |||
| // This can registered before the PHPUnit error handler. | |||
There was a problem hiding this comment.
typo: missing "be" (... can be...
There was a problem hiding this comment.
Fixed in latest push - I did a rebase to get the latest changes so had to force push.
511faf9 to
1fb801a
Compare
|
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