[PhpUnitBridge] fix deprecation triggering test detection #24597
Conversation
xabbuh
commented
Oct 17, 2017
•
| Q | A |
|---|---|
| Branch? | 3.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #24548 (comment), #24568 |
| License | MIT |
| Doc PR |
|
Would that fix and replace #24575 also? |
| $class = isset($trace[$i]['object']) ? get_class($trace[$i]['object']) : $trace[$i]['class']; | ||
| $method = $trace[$i]['function']; | ||
| $Test = $UtilPrefix.'Test'; | ||
| if (isset($trace[$i]['class']) && 'Symfony\Bridge\PhpUnit\SymfonyTestsListener' === $trace[$i]['class']) { |
There was a problem hiding this comment.
Do we have to deal with the fact that this might be the legacy version?
ie. \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListener
There was a problem hiding this comment.
That's no problem as the trait is called by the listener. That's why we see the listener here.
| } else { | ||
| $class = isset($trace[$i]['object']) ? get_class($trace[$i]['object']) : $trace[$i]['class']; | ||
| $method = $trace[$i]['function']; | ||
| $Test = $UtilPrefix.'Test'; |
There was a problem hiding this comment.
$Test is not assigned if the class is Symfony\Bridge\PhpUnit\SymfonyTestsListener. This might cause problems in the later if
| @@ -264,9 +264,9 @@ public function endTest($test, $time) | |||
| unlink($this->runsInSeparateProcess); | |||
| foreach ($deprecations ? unserialize($deprecations) : array() as $deprecation) { | |||
| if ($deprecation[0]) { | |||
| trigger_error($deprecation[1], E_USER_DEPRECATED); | |||
| trigger_error(serialize(array('deprecation' => $deprecation[1], 'class' => $deprecation[2], 'method' => $deprecation[3])), E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
I think the class and method here need to be determined by $test and not what's in $deprecation
There was a problem hiding this comment.
This code happens to work for \Symfony\Bridge\PhpUnit\Tests\ProcessIsolationTest() because the deprecation takes place in the test method. However, that won't be the case for a deprecation occurring in non test code.
There was a problem hiding this comment.
Well, we don't want to show the code triggering the deprecation. But we want to display a summary of tests that lead to these deprecations. Because you either need to fix the code executed by these tests or you will have to add these tests to the legacy group.
There was a problem hiding this comment.
Yes I totally agree but that's not what is in $deprecation[2] and $deprecation[3]. These contain the code that has triggered the deprecation not the test. That's in $test.
There was a problem hiding this comment.
I don't think so. We filter the stack trace before serealizing the same way as we do when not running tests in isolation. Do you have an example that wouldn't work?
There was a problem hiding this comment.
Ah I see I missed that :( sorry. I've tested with a deprecation in another file and you're correct that the stack trace filtering works. I kinda think that using $test is simpler but you're right it works.
|
If we go this route of adding the test and method to the deprecation thrown by ::endTest() then we also need to parse out the message in \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::handleError() so that the @expectedDeprecation annotation works. |
|
Do you have an example of what wouldn't work otherwise? I mean the message is never touched, is it? |
|
Here are the changes I think we need to address the points from my review: |
|
Thank you @xabbuh. |
…(xabbuh) This PR was merged into the 3.3 branch. Discussion ---------- [PhpUnitBridge] fix deprecation triggering test detection | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24548 (comment), #24568 | License | MIT | Doc PR | Commits ------- da617e8 fix deprecation triggering test detection
|
@alexpott I'm merging because this fixes our builds on branches 2.7 and 2.8. But in alexpott@6f45707#commitcomment-25097923 there is one remaining question for you. Does this mean you identified another edge case? |
This PR was merged into the 3.3 branch. Discussion ---------- Fix deprecation triggering test deduction | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> <!-- - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the 3.4, legacy code removals go to the master branch. - Please fill in this template according to the PR you're about to submit. - Replace this comment by a description of what your PR is solving. --> This PR is just testing some additions symfony/symfony#24597 - I want to see if the travis run is successful. Commits ------- 5333680 Fix review points
This PR was merged into the 3.3 branch. Discussion ---------- Fix deprecation triggering test deduction | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> <!-- - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the 3.4, legacy code removals go to the master branch. - Please fill in this template according to the PR you're about to submit. - Replace this comment by a description of what your PR is solving. --> This PR is just testing some additions #24597 - I want to see if the travis run is successful. Commits ------- 5333680 Fix review points