IBX-12606: Migrated the test suites to PHPUnit 11 - #831
Conversation
959dcae to
e2c8fa1
Compare
There was a problem hiding this comment.
- Rector kept the old
@coversstrings verbatim, so ~350 CoversMethod attributes in 34 files still have parentheses or a parameter list in the method name, e.gContentServiceAuthorizationTest.php:30: 'createContent($contentCreateStruct, $locationCreateStructs)'. PHPUnit 11 errors on this once coverage is collected (CI runs without coverage, hence green). Do we fix it here or leave it for a follow-up? - many
CoversClass/CoversMethodtargets point atIbexa\Contracts\Core\Repository\*interfaces or at the wrong class, e.g. covers the ContentService interface, which has
no mapFieldsForCreate. PHPUnit 11 rejects interface targets and missing methods once coverage runs. The description mentions fixing wrong@coverstargets, so should these be pointed at theIbexa\Core\Repository\*implementations here, or left for a follow-up? - richtext 6.0 still has six non-static overrides of hooks made static here, e.g. https://github.com/ibexa/fieldtype-richtext/blob/6.0/tests/integration/Repository/RichTextFieldTypeIntegrationTest.php#L274, and richtext#352 does not
touch them. Could we fix the merge-order note?
|
|
||
| class EZP22612URLAliasTranslations extends BaseTestCase | ||
| { | ||
| public function __construct() |
There was a problem hiding this comment.
This constructor drops the test method name PHPUnit passes in, so the test cannot run. It goes unnoticed only because the file lacks the Test suffix and is never discovered. Could we remove it?
There was a problem hiding this comment.
Fixed in 8a125e6. The override was added in this PR and replaced the test name PHPUnit passes in; EZP22840RoleLimitations has the same override and is handled in a separate commit.
| @@ -458,27 +460,31 @@ public function providerForTestEvaluate() | |||
| protected function assertContentHandlerExpectations($callNo, $persistenceCalled, $contentId, $contentInfo) | |||
| { | |||
| $this->getPersistenceMock() | |||
| ->expects(self::at($callNo + ($persistenceCalled ? 1 : 0))) | |||
| ->expects(self::once()) | |||
There was a problem hiding this comment.
once() is registered per target on the same mock method (also 463, 468, 516). With two or more targets the first call violates the other with() and fails; it passes only because every provider row has one target. Could we use exactly(count($targets)) with willReturnCallback, like the other conversions in this PR?
There was a problem hiding this comment.
Fixed in b402202. Both handler methods now use one exactly(count) expectation with willReturnCallback keyed on the invocation number, and a two-target LocationCreateStruct provider row was added so the multi-target path is exercised.
|
|
||
| class ContextualizerTest extends TestCase | ||
| { | ||
| private const SA_NODE_NAME = 'heyho'; |
There was a problem hiding this comment.
| private const SA_NODE_NAME = 'heyho'; | |
| private const string SA_NODE_NAME = 'heyho'; |
| /** @var \PHPUnit\Framework\MockObject\Stub&\Symfony\Component\Console\Command\Command */ | ||
| private \PHPUnit\Framework\MockObject\Stub $command; |
There was a problem hiding this comment.
use Symfony\Component\Console\Command\Command;
use PHPUnit\Framework\MockObject\Stub;
| /** @var \PHPUnit\Framework\MockObject\Stub&\Symfony\Component\Console\Command\Command */ | |
| private \PHPUnit\Framework\MockObject\Stub $command; | |
| private Command & Stub $command; |
There was a problem hiding this comment.
Fixed in 3bafc39. Applied to every property in tests/ that carried the real type only in a docblock, 18 properties in 15 files, and the 5 properties whose docblock said intersection while the native type was plain got the intersection type as well.
| /** @var \PHPUnit\Framework\MockObject\Stub&\Symfony\Component\HttpKernel\HttpKernelInterface */ | ||
| private \PHPUnit\Framework\MockObject\Stub $httpKernel; |
There was a problem hiding this comment.
| /** @var \PHPUnit\Framework\MockObject\Stub&\Symfony\Component\HttpKernel\HttpKernelInterface */ | ||
| private \PHPUnit\Framework\MockObject\Stub $httpKernel; |
There was a problem hiding this comment.
| /** @var \PHPUnit\Framework\MockObject\Stub&\Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface */ | ||
| private \PHPUnit\Framework\MockObject\Stub $configResolver; | ||
|
|
||
| /** @var \PHPUnit\Framework\MockObject\MockObject */ | ||
| private $router; | ||
| /** @var \PHPUnit\Framework\MockObject\Stub&\Symfony\Component\Routing\RouterInterface */ | ||
| private \PHPUnit\Framework\MockObject\Stub $router; | ||
|
|
||
| /** @var \PHPUnit\Framework\MockObject\MockObject|\Psr\Log\LoggerInterface */ | ||
| private $logger; | ||
| /** @var \PHPUnit\Framework\MockObject\Stub&\Psr\Log\LoggerInterface */ | ||
| private \PHPUnit\Framework\MockObject\Stub $logger; |
There was a problem hiding this comment.
| /** @var \Ibexa\Core\FieldType\Validator\FileExtensionBlackListValidator&\PHPUnit\Framework\MockObject\Stub */ | ||
| protected \PHPUnit\Framework\MockObject\Stub $fileExtensionBlackListValidatorMock; |
There was a problem hiding this comment.
| private UrlRedecoratorInterface & MockObject $redecorator; | ||
|
|
||
| private PathGenerator & MockObject $pathGenerator; | ||
| private PathGenerator&\PHPUnit\Framework\MockObject\Stub $pathGenerator; |
There was a problem hiding this comment.
Shouldn't be here and below:
use \PHPUnit\Framework\MockObject\Stub;
| private PathGenerator&\PHPUnit\Framework\MockObject\Stub $pathGenerator; | |
| private PathGenerator&Stub $pathGenerator |
?
There was a problem hiding this comment.
Fixed in 068e27e, including the other properties in this file.
| * field type has no "empty" representation, since PHPUnit 11 treats an empty data provider | ||
| * as a hard error rather than a skip. | ||
| */ | ||
| protected const NO_EMPTY_VALUE_DATA = '__no_empty_value_data__'; |
There was a problem hiding this comment.
Should be moved up - under the class declaration
| protected const NO_EMPTY_VALUE_DATA = '__no_empty_value_data__'; | |
| protected const string NO_EMPTY_VALUE_DATA = '__no_empty_value_data__'; |
There was a problem hiding this comment.
Fixed in 5c06464. NO_DATA_METHOD in AbstractServiceTestCase got the same treatment.
| /** @var \Ibexa\Core\Persistence\Legacy\Content\Language\Handler&\PHPUnit\Framework\MockObject\MockObject */ | ||
| private Handler $contentLanguageHandler; | ||
| /** @var \Ibexa\Core\Persistence\Legacy\Content\Language\Handler&\PHPUnit\Framework\MockObject\Stub */ | ||
| private \PHPUnit\Framework\MockObject\Stub $contentLanguageHandler; |
There was a problem hiding this comment.
| private \PHPUnit\Framework\MockObject\Stub $contentLanguageHandler; | |
| private Handler & Stub $contentLanguageHandler; |
| @@ -23,12 +23,12 @@ final class RelationListFacadeTest extends TestCase | |||
|
|
|||
| private RelationListFacade $relationListFacade; | |||
|
|
|||
| private VersionInfo&MockObject $versionInfo; | |||
| private VersionInfo&\PHPUnit\Framework\MockObject\Stub $versionInfo; | |||
There was a problem hiding this comment.
| private VersionInfo&\PHPUnit\Framework\MockObject\Stub $versionInfo; | |
| private VersionInfo & Stub $versionInfo; |
…ions regression test
…ion in ParentContentTypeLimitationTypeTest
| public function __construct() | ||
| { | ||
| parent::__construct(static::class); | ||
| } |
There was a problem hiding this comment.
Same here: https://github.com/ibexa/core/pull/831/changes#r4036505180
| } |
alongosz
left a comment
alongosz
left a comment
There was a problem hiding this comment.
General finding:
Three files were never converted. HTTPHandlerTest still uses docblock
@dataProviderat three places; UniqueIdentifierTest and MultiLanguageTestTrait likewise escaped the Rector pass.
…e implementations
|
Review points addressed in separate commits:
|
… intersection types
…rminateListenerTest
…ions in URLCheckerTest
…est::testDeleteGroup
|
Remaining docblock annotations converted in 71e0c1e: HTTPHandlerTest (covers, 3 data providers), UniqueIdentifierTest (covers), MultiLanguageTestTrait (4 depends), RelationSearchBaseIntegrationTestTrait (groups moved to the two consuming classes, including the previously unlisted relation group). rg for @dataProvider/@depends/@covers/@group in tests/ now returns only a prose mention. |
ciastektk
left a comment
ciastektk
left a comment
There was a problem hiding this comment.
✅ given how heavy this PR already is and how difficult is to reviewing all of these changes.
Following things could be improved as follow-ups:
- Deprecation gate: phpunit-bridge's bootstrap bails out early on PHPUnit 10+, so
SYMFONY_DEPRECATIONS_HELPERis effectively dead now and CI no longer tracks Symfony deprecations. I know it's planned as an org-wide pass, but I'd like to see it tracked somewhere so it doesn't get lost. The leftoverenventries in thephpunit*.xmlfiles can go at the same time. - PHPUnit 12 prep:
will(self::returnValue()),getMockForAbstractClass()and thenumberOfInvocations()blocks. A lot of the latter could just bewillReturnMap()where order doesn't matter, which would also help with Sonar. - Return types on tests Most test methods still lack
: voidand the related data providers have no return type. ParentDepthLimitationTypeTesthas the sameonce()-in-foreachissue as the one fixed here. Pre-existing, so not for this PR.- The two
EZP*regression tests still have noTestsuffix and never run. Renaming them will tell us if they even pass. RelationIntegrationTestcoversBaseIntegrationTestCase— a test base class as a coverage target makes no sense, it's leftover from an old@covers.- Test double typing consistency — the pre-existing docblock-only intersections and
static::infinaltest classes. Might be worth addingself_static_accessortoibexa/code-styleso it stays that way.
this is already not tracked, there is limit at 850 in core and the actual number is two order lower.
this will just introduce additional mess. |
|
4 participants
Description:
Migrates
phpunit/phpunit^9.6→^11.5andmatthiasnoback/symfony-dependency-injection-test^5.0→^6.0(symfony/phpunit-bridgestayed^7.4,dama/doctrine-test-bundlestayed^8.0,both already correct). Migrated all 4 PHPUnit configs (
phpunit.xml,phpunit-integration.xml,phpunit-integration-legacy.xml,phpunit-integration-legacy-solr.xml) to the 11.5 schema..gitignorealready had.phpunit.cache//.phpunit.result.cache.Rector (
PHPUNIT_100,PHPUNIT_110,ANNOTATIONS_TO_ATTRIBUTES) converted 680 files(
tests/lib429,tests/bundle93,tests/integration158);composer fix-cstouched 138 more.WithConsecutiveRectorauto-converted nearly all 48 baseline->withConsecutive()sites into the$matcher = self::exactly(N); ->willReturnCallback(...)idiom, except two sites where the sourcehad
withConsecutive()/willReturnOnConsecutiveCalls()as separate chained calls — Rector mergedthe argument assertions but silently dropped the return-value logic, causing
TypeErrors in twoshared base classes (
AbstractInMemoryCacheHandlerTestCase,AbstractCacheHandlerTestCase,TrashHandlerTest,IOVariationPurgerTest,ImageFileVariationPurgerTest) that had to berestored by hand.
self::at()removal — 230 call sites across 33 files (the task's own pattern-inventory said"a few"; the actual repo usage is spelled
self::at(, not->at(). Converted to the$matcher = self::exactly(N)/self::once()+willReturnCallbackidiom, including aglobal-cross-method-invocation-order variant (a shared incrementing counter across two
differently-named mocked methods) in
FieldHandlerTest,RelationProcessorTest,PermissionTest.setMethods()restoration — 69 call sites, resolved by reading PHPUnit's ownGenerator::generateCodeForTestDoubleClass()source (9.6 vs 11.5) rather than guessing: for aclass target,
setMethods(null)= mock nothing (→onlyMethods([])),setMethods([])= mockeverything (→ Rector's deletion was correct),
setMethods([...])= partial mock (→onlyMethods([...])); for an interface target, Rector's unconditional deletion is always safe.A handful of call sites passed method names that don't exist on the real class (old
setMethods()silently tolerated this;
onlyMethods()throwsCannotUseOnlyMethodsException) — fixed withaddMethods()for the non-existent names, split fromonlyMethods()for the real ones.Non-static data providers — statified across ~120 files in
tests/libandtests/integration, following the "yield scalars/plain data, build the mock or call therepository in the consuming test method" pattern throughout:
tests/lib/MVC/Symfony/Matcher/ContentBased/{Id,Identifier}/*Test.php(11 files sharing anear-identical
matchLocationProvider()/matchContentInfoProvider()shape),Limitation/*,voters,
PermissionCriterionResolverTest(280-line provider with heavy inline mockconstruction),
RelationProcessorTest,ContentTest,PermissionTest(6 providers).BaseFieldTypeTestCase,BaseNumericValidatorTestCase,AbstractServiceTestCase,BaseRepositoryFilteringTestCase(this one was an actual PHP fatal at class-load time — ahalf-fixed override was already static while the base wasn't).
BackgroundIndexingTerminateListenerTestadditionally usedself::returnValue()statically,which is non-static in PHPUnit 11 — rewritten to
willReturn()/willReturnCallback().PHPUnit\Framework\Assert::markTestSkipped()inside a static#[DataProvider]is wrapped into a hardInvalidDataProviderException, not a clean skip (rootcause traced to
DataProvider::dataProvidedByMethods()wrapping anyThrowable) — fixed with asentinel data row + an explicit skip check inside the consuming test method
(
BaseFieldTypeTestCase,AbstractServiceTestCase,SearchBaseIntegrationTestCase'sfull-text-search provider,
CheckboxIntegrationTest::providerForTestIsEmptyValue()).SearchBaseIntegrationTestCase/SearchMultivaluedBaseIntegrationTestCase: amuch larger blocker than the FieldType-provider pattern elsewhere —
findProvider(),sortProvider(),fullTextFindProvider(),findMultivaluedProvider()and their supportinggetSearchTargetValue*()/getValidSearchValue*()/getAdditionallyIndexedFieldData()helpers(shared by ~30 FieldType integration test classes) resolve a live
Repositoryat provider timeto look up fixed system reference data (content types, sections). Added
BaseTestCase::resolveSetupFactory()/resolveRepository()static-safe variants (mirroring theexisting instance-cached
getSetupFactory()/getRepository()— same env-var/factoryresolution, just without the per-instance cache, since a provider has no instance to cache on)
and rewired ~40 files' search-value/target helpers to use them. One resulting bug caught and
fixed: a helper statified for provider use (
RolePolicyLimitationTest) was still called,unstatified, from inside the actual test method after the test had created its own role/user
fixtures — reusing the static, uncached repository resolver there wiped that just-created state
(
getRepository(true)always reinitializes the schema). Split into two methods: the originalinstance-cached one for the test body, a new
*ForFreshRepository()static one for theprovider only.
to FieldType) in
tests/integration/Core/Repository/SearchService/Aggregation/*(9 files) anda dozen more scattered
tests/integration/Core/Repository/*providers(
RoleServiceTest,PermissionResolverTest,PureNegativeQueryTest,RemoteIdIndexingTest,SearchServiceImageTest,SearchServiceContentNameTest,RoleLimitationTest,ContentLimitationsMixIntegrationTest,SearchServiceTest's remaining 5 providers) — thesewere causing a hard PHP fatal that killed the entire integration suite before any test ran,
not something scoped to this migration's task brief but blocking
composer integrationentirely.
#[DependsExternal(...)]same-namespace double-resolution bug — 210 sites across 18 filesin
tests/integration/Core/Repository/: an unqualified, fully-spelled FQCN class-const referenceinside the attribute (no
useimport, no leading\) gets the current namespace prepended byPHP, producing
Ibexa\Tests\Integration\Core\Repository\Ibexa\Tests\Integration\Core\Repository\XTestand a "this test depends on ... which does not exist" error. A prior session's checkpoint fixed
only the self-reference case (
self::class); this pass added a leading\for the remainingcross-class, same-namespace references.
Stale
@param/@covers/@usesdocblocks surfaced as real errors once Rector turned them intoattributes (previously inert free text, tolerated by PHPUnit 9's plain-comment parsing):
14
MVC/Symfony/Matcher/ContentBased/*files kept@param \...\Location $locationdocblocksfrom before an earlier session's provider refactor moved mock construction into the test body
(removed, redundant with the native
int $contentIdtype hint); 5 files had a@covers/@usesannotation whose trailing free-text prose got swallowed into the attribute's string argument by
Rector's conversion (
BaseTestCase::createCustomUserVersion1→ realUsesMethod, twoEZP20018*Testregression tests,DynamicPathFilesystemAdapterDecoratorTest); 3 more had apre-existing (present in
origin/6.0already) wrong-namespace@coverstarget that was harmlessas a doc comment but a hard
class.notFoundonce attributed(
LocationArgumentResolverTest→...\Converter\...instead of...\ControllerArgumentResolver\...;2×
DoctrineDatabaseTest→ global-namespace\DoctrineDatabaseinstead of the imported class;ContentHandlerTest's#[CoversMethod]→Contracts\Core\...instead ofCore\...).PHPStan: pruned ~39 stale baseline entries whose underlying code (self::at()/setMethods/provider
patterns) no longer exists; ~26 sites needed
array_values()around anarray_diff()/array_filter()result passed toonlyMethods()/addMethods()(PHPUnit 11's signature islist<non-empty-string>, stricter than 9's plainarray). Remaining ~84 new baseline entries arePHPUnit-11 API-surface changes matching the wave's established precedent for the same repo-wide
issue classes: mock
->expects()called on an interface-typed variable PHPStan won't widen to&MockObject,varTag.nativeTypemismatches on mocked properties, two confirmed-safevariable.undefined: $thisfalse positives in a closure deferred-bound after construction(
SiteAccessAware/SearchServiceTest,UrlAliasServiceTest), andonlyMethods()list-strictnesson parameter-typed (not just array-diff'd) method-name arrays this session didn't touch the
bodies of. No new ignores added beyond what
--generate-baselineproduced from thesePHPUnit-11-caused findings;
getMockForAbstractClass()deprecations (97 baseline hits,unchanged) are left for the PHPUnit 12 step per the wave's convention.
composer check-cs: 0 of 3378 files need fixing (onestatic_lambdafix applied along the way).vendor/bin/rector process --dry-run --clear-cache: clean (only a pre-existing, unrelateddeprecated-skip-rule warning).
composer deptrac: 0 violations, 0 errors/warnings (97pre-existing skipped violations, baseline unchanged — no abstract
*Test.phpbases were renamedin this repo, so no FQCN keys needed updating). No removed PHPUnit CLI flags (
-v/--verbose)in
composer.jsonscripts or.github/workflows/*.yaml.Unit suite (
composer unit --display-phpunit-deprecations): 7510 tests, 0 failures/errors,1 risky (
DownloadControllerTest—symfony/phpunit-bridge'sExpectDeprecationTraitsets astatic error handler restored by the bridge's own listener lifecycle, not by test code; confirmed
not fixable from test code), 2520 PHPUnit deprecations (all forward-looking PHPUnit-12 removals —
getMockForAbstractClass(),->will($this->returnValue())/returnCallback()/returnValueMap()/onConsecutiveCalls(), stub-with-expects()— a much larger count than the wave's smaller repossince core's test suite predates all of them; left for the PHPUnit 12 migration per precedent).
Integration suite (
phpunit-integration-legacy.xml, SQLite): 11417 tests — exactly matchesthe Stage-0 baseline test count, 0 failures, only the 5 pre-existing
#[Depends]-on-renamed/removed-method errors already present and documented at Stage 0
(
ContentServiceTest::testLoadContentDrafts/loadRelationListno longer exist), 1 pre-existingrisky fixed along the way (
BinaryBaseStorageTest— dropped a contradictoryexpectNotToPerformAssertions()).phpunit-integration.xml(modern) is still red at thebootstrap step with the exact IBX-12530
UNIQUE constraint failed: ibexa_content_field.iderror,unchanged from Stage 0 and owned by
ibexa/test-core, not by this migration.Carried the tests from IBX-6773 (IBX-6773: Fixed loading Bookmarks for non-accessible content items #476) and [Tests] Fixed stale
@dependstargets and outdated assertions in ContentService integration tests #833 into PHPUnit 11 form during the rebase: the newIsBookmarkedQueryBuilderTest,IdSortClauseQueryBuilderTestand theisBookmarkedProvider()providers in
ContentFilteringTest/LocationFilteringTestneeded static providers + attributes,and
IdSortClauseQueryBuilderTestusedDriverManager::getConnection(['url' => …])andQueryBuilder::getQueryPart(), both gone in DBAL 4 — rewritten withgetSQL()-based assertionspreserving the original intent.
Review follow-ups (separate commits on top of the migration commit): constructor overrides removed
from the two
Regression/EZP*classes;ParentContentTypeLimitationTypeTestusesexactly(N)expectations for multi-target rows; all
#[CoversMethod]attributes replaced by deduplicated#[CoversClass]pointing at theIbexa\Core\*implementations (PHPUnit 11 rejects interfacetargets; verified by reflection); test-double properties use native
X&Stub/X&MockObjecttypes with imported classes; typed constants for the new sentinels;
URLCheckerTesthandlersare mocks (stubs do not verify expectations);
ObjectStateHandlerTest::testDeleteGroupassertsthe recorded call sequence; the last docblock
@dataProvider/@depends/@covers/@groupannotations (files Rector skipped, incl. traits) converted to attributes.
Not in this PR: the deprecation gate.
SYMFONY_DEPRECATIONS_HELPERthresholds are dead underPHPUnit 10+ (also with phpunit-bridge 8.0); the PHPUnit-native replacement (
failOnDeprecationignoreSuppressionOfDeprecations+ restricted<source>with a generated baseline) has toland in every wave repo at once and follows in a second pass after CI is unblocked.
For QA:
N/A
Documentation:
N/A