Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code #35178

Merged

Conversation

@fancyweb
Copy link
Contributor

fancyweb commented Jan 2, 2020

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 4, 2020

Let's assume that when a test expect a deprecation, it can have 0 assertions

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 4, 2020

Let's assume that when a test expect a deprecation, it can have 0 assertions

does this mean that a test that makes no assertion won't be reported as risky as soon as it also expects deprecations?
What's the current behavior, and why change it?

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Jan 5, 2020

what does that mean? We do have tests that expect deprecations, and have many other assertions in the body of the test case.

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.

does this mean that a test that makes no assertion won't be reported as risky as soon as it also expects deprecations?
What's the current behavior, and why change it?

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.

@fancyweb fancyweb force-pushed the fancyweb:phpunit-bridge-remove-useless-code branch from 1ad1677 to fb48bbc Jan 14, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 20, 2020

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Jan 20, 2020
…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
@nicolas-grekas nicolas-grekas merged commit fb48bbc into symfony:3.4 Jan 20, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
@fancyweb fancyweb deleted the fancyweb:phpunit-bridge-remove-useless-code branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.