Skip to content

test(hardening): fix order-dependent tests surfaced by phased (ES→OS) runs (#36501)#36562

Open
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36501-test-hardening-phase2
Open

test(hardening): fix order-dependent tests surfaced by phased (ES→OS) runs (#36501)#36562
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36501-test-hardening-phase2

Conversation

@fabrizzio-dotCMS

Copy link
Copy Markdown
Member

Proposed Changes

Two test-quality issues exposed by running the integration suite under a non-zero ES→OS migration phase (phase 2, reads from OpenSearch). Neither is a product bug. Contributes to #36501.

  • RelationshipFactoryImplTest.testGetDBRelatedChildrenMultilingualContent — the DB-focused test's setup relied on index read-after-write consistency: a legacy-relationship checkin with no explicit relationships auto-preserves related content by reading the index, and under OS reads (phase 2) that read isn't read-after-write consistent mid-test, dropping one related version (3 vs 4). Added TestDataUtils.waitForEmptyQueue() after the relationship is established so the language-variant check-ins see a consistent index.
  • ESSiteSearchAPITest.test_deleteOldSiteSearchIndices — used fixed index names with no cleanup, so a class retry (rerunFailingTestsCount) collided on a leftover index (createSiteSearchIndex is fail-hard on already-exists). Now uses unique, strictly increasing timestamps per run (relative order preserved).

Verification

Both verified under phase 2 in an isolated worktree: RelationshipFactoryImplTest 1/0/0, ESSiteSearchAPITest 1/0/0.

Not included

PublisherTest.autoUnpublishContent (initially bucketed here as flaky) is excluded: re-firing IdentifierDateJob with the correct content type on every poll still leaves the identifier's SysExpireDate null under phase 2 (deterministic across 4 reruns), so it is deeper than a race — a candidate product gap in the OS auto-expire path, being re-investigated separately under #36501.

Refs #36501.

… runs (#36501)

Two test-quality issues exposed by running the integration suite under a non-zero
migration phase; neither is a product bug.

- RelationshipFactoryImplTest.testGetDBRelatedChildrenMultilingualContent: the
  DB-focused test's setup relied on index read-after-write consistency (legacy-
  relationship checkin auto-preserves related content by reading the index). Under
  OS reads (phase 2) that dropped one related version (3 vs 4). Add waitForEmptyQueue()
  after the relationship is established so the language-variant check-ins see a
  consistent index. Verified under phase 2 (1/0/0).

- ESSiteSearchAPITest.test_deleteOldSiteSearchIndices: used fixed index names with no
  cleanup, so a class retry collided on a leftover index (createSiteSearchIndex is
  fail-hard on already-exists). Use unique, strictly increasing timestamps per run
  (order preserved) so retries never collide. Verified under phase 2 (1/0/0).

Note: PublisherTest.autoUnpublishContent (initially bucketed here as a flaky
IdentifierDateJob race) is NOT included — re-firing the job with the correct content
type on every poll still leaves the identifier's SysExpireDate null under phase 2,
so it is deeper than a race (candidate product gap in the OS auto-expire path) and
is being re-investigated separately.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 3m 9s —— View job


PR Review: test-hardening (order-dependent tests, phase 2)

  • Read PR diff against origin/main
  • Verify TestDataUtils.waitForEmptyQueue() exists and is imported
  • Verify deleteOldSiteSearchIndices() semantics vs. the new test assertions
  • Check timestamp uniqueness / ordering logic

I reviewed both changed files against the production code paths they exercise.

Verification notes

  • RelationshipFactoryImplTest: TestDataUtils.waitForEmptyQueue() is a real static helper (TestDataUtils.java:2683) with the no-arg overload delegating to the DEFAULT_INDEX_QUEUE_WAIT timeout; import is present. Inserting it after the legacy-relationship checkin (which auto-preserves related content by reading the index) is the correct fix for the read-after-write gap under OS reads. Harmless when the queue is already empty. ✅
  • ESSiteSearchAPITest: unique, strictly-increasing epoch-millis timestamps (base-3000 < base-2000 < base-1000 < base) do resolve the fixed-name collision on class retry, since createSiteSearchIndex is fail-hard on already-exists. I confirmed against ESSiteSearchAPI.deleteOldSiteSearchIndices() (ESSiteSearchAPI.java:726) that the assertions still hold: the activated idxDefault is excluded from removal, and the remaining non-aliased indices are deleted, so assertFalse(indices.contains(idxMiddle)) and the size-change assertion remain valid. ✅

New Issues

No issues found.

Two small non-blocking observations (both pre-existing patterns, not introduced by this PR, so not flagged as findings):

  • The test still has no @After cleanup, so each run leaves the activated idxDefault behind. Unique names avoid the collision, so this no longer breaks retries — just a minor index leak in the shared namespace.
  • The updated comment calls idxMiddle the "middle (old, non-default) index," but numerically it is the second-newest (base-1000). The assertion is still correct (all non-default indices are removed); only the label reads slightly off.

This is a well-scoped, correctly-targeted test-hardening change. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant