Repository navigation
[PHPUnit60] Cache containsAssertCall() per node in AddDoesNotPerformAssertionToNonAssertingTestRector - #829
Open
TomasVotruba wants to merge 2 commits into
Open
TomasVotruba wants to merge 2 commits into
TomasVotruba wants to merge 2 commits into
Conversation
…ssertionToNonAssertingTestRector The cache key printed the whole method body on every call. Store the result as a node attribute instead - same body in different classes no longer shares a result, and no printing is needed. "parent::method()" now checks the class that declares the method, so a method inherited from TestCase through a parent that does not override it is skipped too, instead of parsing TestCase. Add bench/does_not_perform_assertion_cache_bench.php to cover both.
TomasVotruba
force-pushed
the
tv-assert-call-cache-perf
branch
from
November 14, 2026 17:08
c154c65 to
2f1b501
Compare
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
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.
Follow-up to #826.
Change
containsAssertCall()cache: the key wasmd5(prettyPrint(method) . className), so the whole method was printed on every call. The result is now stored as a node attribute. Identical bodies in different classes don't share a result, without needing the class name in the key. A node attribute instead ofWeakMap, so the code still downgrades to PHP 7.4.parent::method()now checks the class that declares the method, not only the direct parent. Before,parent::setUp()through a parent that doesn't override it was resolved up toTestCase, which then had to be parsed.Bench
New
bench/does_not_perform_assertion_cache_bench.php: 300 test classes calling helpers from 5 abstract parents, plus aparent::setUp()call declared only inTestCase. Fails above 1.0 s CPU.CPU, 3 runs, local PHP 8.5 (busy machine, CI is ~3x faster):
maindoes_not_perform_assertion_cache_bench.phpdoes_not_perform_assertion_lifecycle_bench.phpMost of the remaining time is parsing each parent file once. That work is required: before #826,
mainskipped it only because of the shared cache entry.Trade-off
The cached result stays on the node for the rest of the run. If another rule later adds an assert inside a method that was already checked, the cached value is out of date.