Skip to content

test(index): decouple ContentletIndexAPIImplTest from ElasticSearch internals (#36320)#36580

Open
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36320-decouple-contentletindexapitest
Open

test(index): decouple ContentletIndexAPIImplTest from ElasticSearch internals (#36320)#36580
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36320-decouple-contentletindexapitest

Conversation

@fabrizzio-dotCMS

Copy link
Copy Markdown
Member

Proposed Changes

Decouples ContentletIndexAPIImplTest from ElasticSearch internals so it is vendor/phase-agnostic — the in-scope work of #36320.

  • Neutralize the ES-direct search helper: isDocIndexed() now uses the phase-aware SiteSearchAPI.search(index, query, start, rows) (routes to ES or OS per the active migration phase) instead of the private indexSearch() that used RestHighLevelClientProvider + SearchRequest/SearchSourceBuilder/QueryBuilders/SearchHits. The indexSearch() helper is deleted.
  • Vendor-neutral exception assertion: testGetIndexDocumentCountWithInvalidIndexNameFails now assertThrows(RuntimeException.class, ...) instead of @Test(expected = ElasticsearchStatusException.class). The concrete type differs by backend (ES → ElasticsearchStatusException, OS read path → DotRuntimeException); asserting the common RuntimeException passes under every phase without coupling to a vendor type.
  • Removed all 8 org.elasticsearch.* imports + RestHighLevelClientProvider + the now-unused INDEX_OPERATIONS_TIMEOUT_IN_MS / Sneaky imports. Zero org.elasticsearch imports remain.

Verification

ContentletIndexAPIImplTest under phase 0 (ES): 16/0/0 ✅. testSearch's doc-indexed check now passes via the neutral path.

Out of scope / note

testSearch's highlight assertions still depend on OpenSearch site-search highlights, which are not implemented yet (#36576 / #34609) — so full phase-2/3 green for that one method is gated on that separate product gap, not on this decoupling.

Refs #36320, #36501.

…nternals (#36320)

Neutralize the ES-direct pieces so the test is vendor/phase-agnostic:
- Replace the private ES-direct indexSearch() helper (RestHighLevelClientProvider +
  SearchRequest/SearchSourceBuilder/QueryBuilders/SearchHits) with the phase-aware
  SiteSearchAPI.search(index, query, start, rows) in isDocIndexed() — it routes to ES
  or OS per the active migration phase.
- Change testGetIndexDocumentCountWithInvalidIndexNameFails to expect the neutral
  DotRuntimeException the router wraps the backend index-not-found in, instead of the
  ES-specific ElasticsearchStatusException.
- Remove the 8 org.elasticsearch.* imports + RestHighLevelClientProvider + the unused
  INDEX_OPERATIONS_TIMEOUT_IN_MS/Sneaky imports.

Note: testSearch's highlight assertions still depend on OS site-search highlights,
which are not implemented yet (#36576 / #34609), so full phase-2/3 green for that one
method is gated on that separate product gap.

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

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 1m 20s —— View job


Code Review — decouple ContentletIndexAPIImplTest from ES internals

  • Read PR diff and surrounding context
  • Verify SiteSearchAPI.search signature and return type
  • Verify isDocIndexed call sites for lost behavior
  • Verify vendor-neutral exception assertion

New Issues

No issues found.

This is a well-scoped, test-only decoupling. Verified:

  • SiteSearchAPI.search(String indexName, String query, int start, int rows) exists (dotCMS/.../sitesearch/business/SiteSearchAPI.java:57) and returns SiteSearchResults, whose getTotalResults() returns long (PaginatedArrayList.java:44). So greaterThan(0L) in the Awaitility until(...) matches the returned type correctly — no autoboxing/matcher mismatch.
  • No lost behavior from deleting indexSearch(). The only caller, isDocIndexed, has a single call site (ContentletIndexAPIImplTest.java:740) that always passes a real timestamped indexName. The deleted helper's indexName == null branch (which created/activated an index) was never exercised by that path, so dropping it changes nothing.
  • assertThrows(RuntimeException.class, ...) at line ~1320 is the right generalization — ElasticsearchStatusException and the OS-path DotRuntimeException both extend RuntimeException, so the assertion holds under every migration phase without coupling to a vendor type. It's slightly broader than the old @Test(expected = ...), but for an invalid-index-name case that's an acceptable trade for phase-agnosticism.
  • All 8 org.elasticsearch.* imports removed, plus RestHighLevelClientProvider, Sneaky, and INDEX_OPERATIONS_TIMEOUT_IN_MS. Zero org.elasticsearch imports remain in the file. The esIndexAPI field is still used elsewhere in the class, so no dead-field cleanup is needed here.

The out-of-scope note on testSearch's OS highlight assertions (#36576 / #34609) is accurate and appropriately deferred.

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