[Tests] Fixed stale @depends targets and outdated assertions in ContentService integration tests - #833
Merged
Conversation
ViniTou
force-pushed
the
fix-stale-depends-5.0
branch
from
September 16, 2026 12:29
9c5f1af to
26a7363
Compare
…ntService integration tests Several @Depends annotations still targeted testLoadContentDrafts and loadRelationList, method names that no longer exist since the testLoadContentDrafts -> testLoadContentDraftList rename (aba61e9). PHPUnit 9 silently skips tests whose @Depends target is unresolvable, so these dependents had not actually run since that rename. Retargeted: - ContentServiceAuthorizationTest::testLoadContentDraftListReturnsEmptyListForUserWithoutVersionReadAccess(WithUser) (was testLoadContentDraftsThrowsUnauthorizedException(WithUser)): testLoadContentDrafts -> testLoadContentDraftList - ContentServiceTest::testLoadRelationsSkipsArchivedContent/SkipsDraftContent: loadRelationList -> testLoadRelationList - ContentServiceTest::testDeleteVersionInTransactionWithCommit: testLoadContentDrafts -> testLoadContentDraftList A full reflection-based sweep of tests/ found no other stale @Depends targets. Running the previously-skipped tests uncovered that several no longer test what their names claim, because loadRelationList()/loadContentDraftList() were redesigned (already true on 4.6) to never throw UnauthorizedException - they return an empty/partial list instead: - testLoadRelationsThrowsUnauthorizedException, testLoadRelationsForDraftVersionThrowsUnauthorizedException, testLoadContentDraftsThrowsUnauthorizedException(WithUser): renamed and rewritten to assert the actual current behaviour (empty list back). - testLoadRelationsWithUnauthorizedRelations: loadRelationList() keeps one list item per relation, exposing unreadable ones as UnauthorizedRelationListItem placeholders rather than omitting them, so the test now asserts 4 items (2 authorized + 2 unauthorized) instead of 2. ContentServiceTest::testLoadRelationsSkipsArchivedContent also had its own fixture bug: it created its throwaway draft under the very Location it then trashed, so trashing purged the draft (and its relations) itself, instead of just archiving the "Demo Design" relation target. Moved the draft under the unrelated Home location instead. No src/ changes. - testLoadContentWithinGracePeriod overrode the shared ContentService's grace period via reflection and never restored it. With the @Depends above resolvable again PHPUnit reorders execution so this test now runs before ContentServiceAuthorizationTest::testLoadContentThrowsUnauthorizedExceptionsOnArchives, which then read the archived version inside the leaked grace period. The original value is now restored in a finally block. - The four renamed authorization tests declare `: void` instead of keeping their phpstan-baseline "no return type" entries.
ViniTou
force-pushed
the
fix-stale-depends-5.0
branch
from
September 16, 2026 12:47
26a7363 to
3e3e238
Compare
|
konradoboza
approved these changes
Sep 16, 2026
konradoboza
requested review from
Steveb-p,
alongosz,
barw4,
bnowak,
ciastektk,
mikadamczyk,
tbialcz and
wiewiurdp
September 17, 2026 06:18
alongosz
changed the title
@depends targets and outdated assertions in ContentService integration tests
alongosz
approved these changes
Sep 17, 2026
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
3 participants
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.
Description:
Six
@dependsannotations in theContentServiceintegration tests point at methods that no longer exist since theloadContentDrafts()→loadContentDraftList()rename (aba61e9098):ContentServiceTest::testLoadContentDrafts(×4, incl. a duplicated line) and a typoloadRelationList(×2, meanttestLoadRelationList). PHPUnit 9 silently skips dependents of a missing target, so 7 tests have not executed on 5.0/6.0 since then; PHPUnit 11 turns this into a hard error (surfaced by #831). 4.6 is unaffected (the targets still exist there and the tests pass).Retargeting the annotations makes those 7 tests run again, and 6 of them failed (the 7th,
testDeleteVersionInTransactionWithCommit, passes). Each was a test bug, none a product regression (the tested code is byte-identical between 4.6 and 5.0):ContentServiceTest::testLoadRelationsSkipsArchivedContent— created its draft under location 56 and then trashed location 56;TrashService::trash()purges draft-only content in the subtree, deleting the test's own draft and relations. Draft now created under "Home" (2).ContentServiceTest::testLoadContentWithinGracePeriodset the sharedContentService's grace period to 10 via reflection and never restored it. Resolving the@dependschanges PHPUnit's execution order, so this test now runs beforeContentServiceAuthorizationTest::testLoadContentThrowsUnauthorizedExceptionsOnArchives, which then read the archived version inside the leaked grace period (passes on unmodified 5.0 only because of the old order). Fixed at the source: original value restored in afinally.testLoadContentDraftsThrowsUnauthorizedException/…WithUser→ renamedtestLoadContentDraftListReturnsEmptyListForUserWithoutVersionReadAccess/…WithUser:loadContentDraftList()never throws, it returns an empty list withoutversionread(the throwingloadContentDrafts()was removed on 5.0).testLoadRelationsThrowsUnauthorizedException/testLoadRelationsForDraftVersionThrowsUnauthorizedException→ renamedtestLoadRelationsReturnsEmptyListForUserWithoutReadAccess/testLoadRelationsForDraftVersionReturnsEmptyListForUserWithoutVersionReadAccess:loadRelationList()returns an empty list for a non-readable source version instead of throwing.testLoadRelationsWithUnauthorizedRelations—loadRelationList()keeps one item per relation and exposes non-readable targets asUnauthorizedRelationListItem(hasRelation() === false) rather than omitting them; assertions now expect 4 items, 2 of them unauthorized.Reflection sweep of all 1179
@dependsannotations undertests/finds no other stale target.phpstan-baseline.neon: 4missingType.returnentries dropped, the renamed methods now declare: void. Nosrc/changes.Legacy integration (
ContentServiceTest|ContentServiceAuthorizationTest|ContentTypeServiceTest|UserServiceTest): 557 tests, 0 failures. Unit: 7462 tests, 0 failures. PHPStan and check-cs clean.Merge-up note: #831 (PHPUnit 11) carries a minimal retarget of the same annotations; after this merges up to 6.0, #831 gets rebased and that part dropped.
For QA:
N/A
Documentation:
N/A