Skip to content

Count distinct entities in TestAdvancedSearch data-API helpers - #4

Open
dmaffitt wants to merge 1 commit into
mainfrom
fix/advanced-search-distinct-count
Open

Count distinct entities in TestAdvancedSearch data-API helpers#4
dmaffitt wants to merge 1 commit into
mainfrom
fix/advanced-search-distinct-count

Conversation

@dmaffitt

@dmaffitt dmaffitt commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This change hardens TestAdvancedSearch so it can take into account pre-existing data. This surfaced while running tests on the tomcat10-compatible XNAT.

Problem

TestAdvancedSearch.testSubjectSearch and testSubjectJoinSearch fail with count mismatches (e.g. expected:<66> but was:<64>) whenever any subject visible to the test user is shared into another project — including the shared subjects this test itself creates in addTestProjects().

Root cause: the count assertions compare the stored-search result size against get*CountFromDataApi(), which returned the raw row count of the /subjects and /experiments data-API listings. Those listings fan out one row per accessible project membership, so a shared subject/session is counted multiple times, while the stored searches return DISTINCT entities. The two cardinalities are therefore only equal on a pristine, share-free server. This is a test-side cardinality mismatch, not a server regression.

Relationship to the earlier hardening commit

This is the same fragility the earlier commit 8877213 "Make TestAdvancedSearch more resilient to pre-existing data" set out to fix — and it got the label assertions right by switching exact-equality to assertContainsAll(...) subset checks (those still pass). But for the counts it replaced assertEquals(subjectLabels.size(), results.size()) (own-object count) with assertEquals(getSubjectCountFromDataApi(), results.size()) (data-API row count). That made the counts robust to pre-existing data (both endpoints see it) but newly fragile to sharing fan-out, because /data/subjects fans out per membership while the search is DISTINCT. In other words, 8877213 hardened the subset checks but left the count checks comparing two endpoints with different cardinality semantics. This PR completes that work.

Fix

Rework all four get*CountFromDataApi() helpers to count distinct identities via shared dataApiRows() / distinctRowCount() helpers:

  • subject / session / qc → key on ID
  • scan → key on (xnat:mrsessiondata/id, scan/id)

This keeps the "compare against current global server state" intent of 8877213 while making the counts comparable to the DISTINCT searches regardless of sharing or leftover data.

Verification

Ran -Dtest=TestAdvancedSearch against a live server: testSubjectSearch and testSubjectJoinSearch now pass (class failures 3 → 1). Compiles under JDK 8.

The subject/session/scan/qc count assertions compared the stored-search
result size against get*CountFromDataApi(), which returned the raw row
count of the /subjects and /experiments listings. Those listings fan out
one row per accessible project membership, so any subject/session shared
into another project (including the shared subjects this test creates in
addTestProjects) is counted multiple times, while the stored searches
return DISTINCT entities. This made testSubjectSearch and
testSubjectJoinSearch fail (e.g. 66 vs 64) whenever shared data was
visible -- not a server regression but a test-side cardinality mismatch.

Rework all four get*CountFromDataApi() helpers to count distinct
identities via shared dataApiRows()/distinctRowCount() helpers
(subject/session/qc key on ID; scan keys on session-id + scan-id), so the
counts are comparable to the DISTINCT searches regardless of sharing or
pre-existing data. Verified against a live server: testSubjectSearch and
testSubjectJoinSearch now pass.

Note: testScanSearch remains failing due to a separate mrScanData
search-vs-/experiments-listing visibility discrepancy, not addressed here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dmaffitt
dmaffitt requested review from a team, bin-xnatworks and xnatworks-tim August 10, 2026 18:44
@dmaffitt
dmaffitt marked this pull request as ready for review August 10, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants