[PHPUnit60] Fix false positive in AddDoesNotPerformAssertionToNonAssertingTestRector#687
Merged
Merged
Conversation
…rtingTestRector The nesting-depth counter in AssertCallAnalyzer was incremented on every containsAssertCall() but never decremented, so it accumulated across the whole depth-first walk of a single test method instead of tracking real recursion depth. When a non-asserting call (e.g. a production static call) was visited before an assert-performing helper, it exhausted the level-5 budget; the later helper was then short-circuited to "no assertions" and the test wrongly got @doesNotPerformAssertions. Wrap the body in try/finally so the level is restored on unwind. Add a keep fixture with a deep non-asserting chain preceding an assert helper.
Member
Author
|
This might be a perf bottleneck, but lets give it a go. We can adjust nested level to lower value if needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
AssertCallAnalyzer::containsAssertCall()incremented$classMethodNestingLevelon every call but never decremented it. So instead of tracking real recursion depth, the counter accumulated across the whole depth-first walk of a single test method.Why it caused false positives
When a non-asserting call (e.g. a production static call) was visited before an assert-performing helper, it burned through the level-5 budget. The later helper recursion then hit
level > MAX_NESTED_METHOD_CALL_LEVELand short-circuited tofalse, so a test that does assert via a helper was wrongly tagged@doesNotPerformAssertions.Order-dependent: any test where a non-asserting call precedes the assert helper in source order.
Found while investigating why
webonyx/graphql-phpskips this rule as "False-positive". Running the rule there flagged 7+ tests that assert through helpers ($this->runTestCase(),$this->expectGraphQLValue(),$this->checkHandlesNonNullableLists(), ...). With this fix only genuinemarkTestSkipped()tests remain.Fix
Wrap the body in
try { ... } finally { --$this->classMethodNestingLevel; }so the depth is restored on unwind — sibling calls in the same DFS keep their full budget.Test
Added keep fixture
keep_deep_calls_before_assert_helper.php.inc: a deep non-asserting chain visited before an assert helper. Fails on the old code (annotation added), passes with the fix.