Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code#35178
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
[PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code#35178
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJan 4, 2020
what does that mean? We do have tests that expect deprecations, and have many other assertions in the body of the test case. |
nicolas-grekas commentedJan 4, 2020
does this mean that a test that makes no assertion won't be reported as risky as soon as it also expects deprecations? |
fancyweb commentedJan 5, 2020
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 tofb48bbcComparenicolas-grekas commentedJan 20, 2020
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 btwCommits-------fb48bbc [PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code
xabbuh commentedMar 9, 2020
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. |
Uh oh!
There was an error while loading.Please reload this page.
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