Skip to content

[Bridge] Corrects bug in test listener trait #46792

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

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

magikid
Copy link
Contributor

@magikid magikid commented Jun 27, 2022

If you enable the SymfonyTestsListenerTrait, use the
ExpectDeprecationTrait, and call
$this->expectNotToPerformAssertions(), an error was returned in your
test saying it was risky as it did not perform any assertions even
though it was expected not to.

This bug turned out to be caused by not checking if the
doesNotPerformAssertions flag had been set.

Issue: #41444

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #41444
License MIT
Doc PR symfony/symfony-docs#...

This fixes a bug that could occur when using this listener along with the ExpectDeprecationTrait trait and the TestCase::expectNotToPerformAssertions() method. When that method is called, the user is saying that no assertions will happen but the listener didn't care and saw no assertions and none skipped so threw the 'No assertions' warning message. This updates the listener to check the value of TestCase::doesNotPerformAssertions when deciding to warn about no assertions or not.

@carsonbot
Copy link

Hey!

Oh no, it looks like you have made this PR towards a branch that is not maintained anymore. :/
Could you update the PR base branch to target one of these branches instead? 4.4, 5.4, 6.0, 6.1, 6.2.

Cheers!

Carsonbot

@magikid magikid force-pushed the 41444-fix-listener branch from 2959cb4 to ebf388a Compare June 27, 2022 18:32
@magikid magikid changed the base branch from 5.2 to 5.4 June 27, 2022 18:33
Copy link
Contributor

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this works to fix the issue that's referenced. However, I would suggest to add a test case too to avoid regressions.

@magikid
Copy link
Contributor Author

magikid commented Jul 11, 2022

I've added a test for this based on the initial report of the issue. A bunch of the tests are failing but the failures seem to be unrelated to this change.

@fabpot fabpot force-pushed the 41444-fix-listener branch from 16af875 to d7d4bd7 Compare July 20, 2022 09:31
@fabpot
Copy link
Member

fabpot commented Jul 20, 2022

Thank you @magikid.

@fabpot fabpot merged commit df6cb75 into symfony:5.4 Jul 20, 2022
@magikid magikid deleted the 41444-fix-listener branch July 20, 2022 11:50
This was referenced Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants