Re-register the DI container in setUp() of tests with PHP-version-dependent reflection assertions - fixes racy/flaky tests#5917
Conversation
89b991c to
feb08f2
Compare
setUp() of type tests with PHP-version-dependent reflection assertionssetUp() of tests with PHP-version-dependent reflection assertions - fixes racy/flaky tests
|
Should the fix be in the base TestCase then @staabm ? |
|
my understanding is, that the lazy construction of the DI container is a performance optimization. |
VincentLanglet
left a comment
There was a problem hiding this comment.
What about mooving this logic in the PHPStanTestCase setup inside a condition (set to false by default). This way it's easy to enable on the right files
…-version-dependent reflection assertions - Add `setUp()` to `HasPropertyTypeTest`, `ObjectTypeTest`, `TypeCombinatorTest` and `GenericObjectTypeTest` that calls `self::getContainer()`. These tests call `Type` methods directly (without going through the analyser), so they resolve reflection through the global `ReflectionProviderStaticAccessor`. Another test can leave a container configured with a different `PhpVersion` registered there; re-registering the default (runtime) container before each test makes the version-dependent assertions deterministic, independent of test ordering or the test runner (the existing PHPUnit event subscriber does not fire reliably under paratest). - Add a deterministic regression test `testIsSuperTypeOfClosureRespectsActivePhpVersion` that exercises both sides of the dynamic-properties boundary (PHP 8.1 -> `Maybe`, PHP 8.2 -> `No`) by building containers with explicit `PhpVersion`s, and restores the default container afterwards so it never leaks global state. - The flaky `hasProperty(foo) -> isSuperTypeOf(Closure)` assertion was caused by the result of `ObjectType::hasInstanceProperty()` for a final class depending on `ClassReflection::allowsDynamicProperties()`, which is version-gated, while the expected value was derived from `PHP_VERSION_ID`; the two diverged when the active container's `PhpVersion` did not match the runtime. - Probed `HasMethodTypeTest`: its `Closure` case is `No` regardless of version (method existence is not affected by dynamic properties), so it is not affected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
46b6e1d to
38dd381
Compare
…flag Replace the per-class setUp() duplication with a $reinitializeContainerBeforeEachTest flag (default false) on PHPStanTestCase. Tests with PHP-version-dependent reflection assertions opt in by setting it to true, so the runtime container is pinned before each test without each class re-implementing setUp(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PHPStanTestCase now defines setUp(), so subclasses that override it must call parent::setUp() to run the (opt-in) container re-registration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. I addressed VincentLanglet's review by moving the container re-registration logic into the base What changed
The four affected type unit tests ( Seven other subclasses that override Verification
Committed as two logical commits (the refactor, then the One trade-off worth noting for the reviewer: making this opt-in via the base class means every existing |
|
WDYT of this way @staabm ? |
Which one ? Extensions need to be updated after |
Summary
HasPropertyTypeTest::testIsSuperTypeOfwas intermittently failing in CI on thehasProperty(foo) -> isSuperTypeOf(Closure)data set withExpected 'Maybe', Actual 'No'.The data set's expected value is
PHP_VERSION_ID < 80200 ? Maybe : No, while the actualvalue comes from
ObjectType(Closure)->hasInstanceProperty('foo'), which for a final classreturns
Maybe/Nodepending onClassReflection::allowsDynamicProperties()— and that isversion-gated (
PhpVersion::deprecatesDynamicProperties(), i.e.>= 8.2).These tests construct
Typeobjects and call their methods directly, so reflection isresolved through the global
ReflectionProviderStaticAccessor. When another test leaves acontainer configured with a different
PhpVersionregistered there, theClosureClassReflectionis built with that foreign version and the result no longer matchesPHP_VERSION_ID. An existing PHPUnit event subscriber re-initializes the container beforeeach test, but it does not fire reliably under the actual test runner (
paratest), so theflake persisted.
setUp()is a runner-agnostic guarantee.Changes
tests/PHPStan/Type/Accessory/HasPropertyTypeTest.phpsetUp()callingself::getContainer()to re-register the default (runtime)container before every test.
testIsSuperTypeOfClosureRespectsActivePhpVersionthat asserts
Maybeunder PHP 8.1 andNounder PHP 8.2 by building containers withexplicit
PhpVersions, restoring the default container in afinallyblock so noglobal state leaks.
tests/PHPStan/Type/ObjectTypeTest.php,tests/PHPStan/Type/TypeCombinatorTest.php,tests/PHPStan/Type/Generic/GenericObjectTypeTest.phpsetUp()re-registration. These are the sibling type unit tests that callTypemethods directly and have version-dependent assertions (dynamic-property handlingof final classes / reflected variance of built-in generics), so they share the same
latent flakiness.
Root cause
A family of type unit tests assert PHP-version-dependent reflection results (computed from
the active container's
PhpVersion) against expectations derived from the globalPHP_VERSION_IDconstant. These two sources diverge whenever the globally-registeredreflection provider belongs to a container whose
PhpVersiondiffers from the runtime —which happens when another test registered such a container and nothing re-registers the
default container before the test executes. Calling
self::getContainer()insetUp()pins the runtime container before each test, eliminating the coupling.
Tests that go through the analyser (
RuleTestCase,TypeInferenceTestCase) already callself::getContainer()during their analyse/assert flow, so they are not affected.Test
testIsSuperTypeOfClosureRespectsActivePhpVersiondeterministically verifies theversion-dependent behavior on every runtime by switching the active container's
PhpVersion.setUp()removed, the
Closuredata set fails (Expected 'No', Actual 'Maybe') once a foreigncontainer is registered; re-enabling
setUp()fixes it without the subscriber.HasMethodTypeTest(the closest sibling): itsClosurecase isNoregardless ofversion, so it is not affected and was left unchanged.
Fixes phpstan/phpstan#14860