[PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code #35178
Conversation
what does that mean? We do have tests that expect deprecations, and have many other assertions in the body of the test case. |
does this mean that a test that makes no assertion won't be reported as risky as soon as it also expects deprecations? |
Yes and that is fine. Removing this code does not change anything about that. The only thing that is changed is that we don't reset the original "beStrictAboutTestsThatDoNotTestAnything" flag value to true (when it was true) at the end of each test anymore.
Yes and that is the current behavior. When a test expects a deprecation we always set the "beStrictAboutTestsThatDoNotTestAnything" flag to false (that overrides it if it was true). So a test that expects an annotation can never be considered as risky if it makes no assertion. The current behavior is not changed. I would like to have @xabbuh input about this. |
1ad1677 to
fb48bbc
Compare
|
Thank you @fancyweb. |
…nneeded code (fancyweb) This PR was merged into the 3.4 branch. Discussion ---------- [PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Risky errors when there are no assertions are added before the test end listeners are called (ie, before the code in endTest is executed) so forcing beStrictAboutTestsThatDoNotTestAnything to false when there is a expectedDeprecation annotation is enough. If the goal is to reset the value to the original value, then I think we should not do it since we basically "lie" to the next listeners. Let's assume that when a test expect a deprecation, it can have 0 assertions. Also this flag is not used anymore by PHPUnit after we reset it. Ref #21786 btw Commits ------- fb48bbc [PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code
I don't know why we had the removed code here initially anyway, but as long as this still works, the changes look good to me. |
Risky errors when there are no assertions are added before the test end listeners are called (ie, before the code in endTest is executed) so forcing beStrictAboutTestsThatDoNotTestAnything to false when there is a expectedDeprecation annotation is enough.
If the goal is to reset the value to the original value, then I think we should not do it since we basically "lie" to the next listeners. Let's assume that when a test expect a deprecation, it can have 0 assertions. Also this flag is not used anymore by PHPUnit after we reset it.
Ref #21786 btw