[bbrala]

Part of rectorphp/rector#9965, split from #822. Stacked on #826: only the last commit (e6105ff) is new here.

PHPUnit counts assertions made in lifecycle methods towards the test. A test whose own body asserts nothing still "performs assertions" when setUp(), tearDown(), assertPreConditions(), assertPostConditions(), a #[Before]/#[After]/#[PreCondition]/#[PostCondition] method or a @before/@after method asserts, in the class, an ancestor or a used trait. AddDoesNotPerformAssertionToNonAssertingTestRector adds #[DoesNotPerformAssertions] to those tests, and with failOnRisky they then fail.

Change

New LifecycleMethodAssertAnalyzer. A test is skipped when its class, an ancestor or a used trait has an asserting lifecycle method.

• The test's own class is taken from the current file's AST (found in getFile()->getNewStmts()), so the file being processed is not parsed again. The rule stays on ClassMethod.
• Ancestors and traits go through AstResolver, but only when a cheap regex on the file contents finds a hook name or a Before/After/PreCondition/PostCondition attribute or annotation. The regex can give false positives (an extra parse), never miss a hook.
• An overriding hook hides the ancestor's hook unless it calls parent::<hook>(). Each hook is evaluated on its own, so long parent::setUp() chains are not cut off by AssertCallAnalyzer's nesting limit.
• Results are cached per class plus the set of hooks overridden below it.
• In a trait parsed on its own, $this is unknown. $this->x(), self::x() and static::x() are resolved against the trait's own methods; if that fails (a method of the using class), the call is treated as possibly asserting.

Two behaviour changes worth a look:

• Test methods in abstract classes are now skipped: a child class can add an asserting hook, and the attribute on the abstract method would then fail the child's tests.
• A trait helper calling a method of the using class (or of TestCase) now counts as possibly asserting, so such tests keep missing the attribute. This only ever removes a change, never adds a wrong one.

Bench

The #824 bench now has 3 scenarios: the original inherited-test shape, every test class with its own setUp(), and asserting hooks at the top of each parent chain. It builds a File per test class and sets it on CurrentFileProvider, like AbstractRectorTestCase does, because the rule now reads the current file. CPU, 3 runs, local ddev PHP 8.5:

scenario	main	#826	this PR
inherited	0.12 s	0.24 s	0.48 s
own setUp()	0.12 s	0.24 s	0.51 s
asserting hooks	0.13 s	0.21 s	0.49 s

On main and #826 the "asserting hooks" scenario changes all 1800 methods; with this PR none. The remaining cost is parsing the ~30 parent files that declare hooks.

Tests

New fixtures cover: own and inherited setUp()/tearDown()/assertPostConditions(), #[Before] (also aliased), @after, a 7-level parent::setUp() chain, hooks and helpers in traits (same file, other file, used by a parent), parent::setUp() in a trait hook, overrides without a parent call, anonymous classes, and abstract classes. Full suite (727 tests), ECS, Rector, class-leak and finalize-classes pass. PHPStan: the same 6 pre-existing errors as main.

Context

Found running the composer-based PHPUnit set on Drupal core (https://www.drupal.org/i/3638148), which runs with failOnRisky and has many base classes asserting in setUp() or assertPostConditions().

[TomasVotruba]

Thanks for the detailed work here! Most of it landed in smaller pieces in the meantime:

• [PHPUnit60] Follow parent::test() calls in AddDoesNotPerformAssertionToNonAssertingTestRector #826 - follow parent::test() calls
• [PHPUnit60] Skip test methods in abstract classes in AddDoesNotPerformAssertionToNonAssertingTestRector #828 - skip test methods in abstract classes, as a child can add an asserting hook
• [PHPUnit60] Change AddDoesNotPerformAssertionToNonAssertingTestRector node from ClassMethod to Class_ #830 - rule now visits Class_, so class-level checks run once per class; traits are skipped implicitly
• [PHPUnit60] Skip tests in classes with own asserting lifecycle methods in AddDoesNotPerformAssertionToNonAssertingTestRector #831 - skip tests when the class's own lifecycle hook asserts (setUp(), tearDown(), assertPreConditions(), assertPostConditions(), #[Before]/#[After]/#[PreCondition]/#[PostCondition], @before/@after/...)
• [PHPUnit60] Cache containsAssertCall() per node in AddDoesNotPerformAssertionToNonAssertingTestRector #829 - containsAssertCall() cached per node; parent::method() resolved to the declaring class
• [PHPUnit60] Never parse PHPUnit and Symfony base test case methods in AssertCallAnalyzer #833 - PHPUnit and Symfony base test cases are never parsed
• [PHPUnit60] Skip tests in classes with asserting parent lifecycle methods in AddDoesNotPerformAssertionToNonAssertingTestRector #834 - parent lifecycle hooks:
  • walks parent classes up to the framework TestCase, max 5 parents
  • a hook overridden without parent::<hook>() hides the parent one
  • chains with no assert/expect/should/mock text in their files (incl. traits) are skipped before parsing
  • results cached per parent class

Limits now:

• lifecycle hooks declared in traits are not analyzed, only methods of the class and its parents
• parent chains deeper than 5 parents are not walked
• a hook asserting through another object, e.g. $this->checker->verify($this) in an unrelated class, is not detected

The branch now conflicts with all of the above. If something from here is still missing (e.g. trait hooks), please send a new PR on top of current main - happy to merge it. Thank you!

[bbrala]

Thanks! With the first reports coming in from people using the composer based sets I guessed it would be a good idea to do some testing on core. Especially for phpunit that seemed like a good test with almost 4000 test filed in drupal core.

Although I still have to disable some symfony rules I think, mostly the even subscribers are broken by removing an interface, this will help the phpunit side not break :)

I'll have a look later and see if we are now safe