Improve IndicesService test coverage

[GypsyJR777]

Description

Improves unit test coverage for IndicesService.

This adds coverage for index lookup by UUID, index creation validation, temporary index service behavior, pending delete validation, deletion checks, stats snapshots, rewrite contexts, shard cache delegation, alias filters, and request cache eligibility checks.

Related Issues

Resolves #9858

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ab151f9)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add tests for index lifecycle and lookup (UUID, creation validation, temp service, pending delete, deletion checks) Relevant files: server/src/test/java/org/opensearch/indices/IndicesServiceTests.java Sub-PR theme: Add tests for stats, rewrite contexts, shard cache, alias filters, and request cache eligibility Relevant files: server/src/test/java/org/opensearch/indices/IndicesServiceTests.java

⚡ Recommended focus areas for review Incorrect Assertion Order In testIndexShardCacheEntityDelegatesToShard, the test calls cacheEntity.isOpen() twice and asserts assertTrue then assertFalse. The mock is set up with when(indexShard.state()).thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED), which means the first call returns STARTED (open) and the second returns CLOSED (not open). However, ramBytesUsed() is called between the two isOpen() checks — if ramBytesUsed() also calls indexShard.state() internally, the sequence of mock return values could be consumed unexpectedly, causing the second isOpen() assertion to behave incorrectly. public void testIndexShardCacheEntityDelegatesToShard() { final IndexShard indexShard = mock(IndexShard.class); final ShardRequestCache requestCache = new ShardRequestCache(); when(indexShard.requestCache()).thenReturn(requestCache); when(indexShard.state()).thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED); final IndicesService.IndexShardCacheEntity cacheEntity = new IndicesService.IndexShardCacheEntity(indexShard); assertSame(requestCache, cacheEntity.stats()); assertSame(indexShard, cacheEntity.getCacheIdentity()); assertTrue(cacheEntity.ramBytesUsed() > 0L); assertTrue(cacheEntity.isOpen()); assertFalse(cacheEntity.isOpen()); } Incomplete Assertion In testStatusCountersAreIncludedInStatsSnapshots, the test uses RestStatus.CREATED.getStatusFamilyCode() - 1 and RestStatus.OK.getStatusFamilyCode() - 1 as array indices. If getStatusFamilyCode() returns a value like 201 or 200, subtracting 1 would give 200 or 199, which would be an out-of-bounds index for a small counter array. The intent seems to be to use the status code directly as an index, but this logic should be verified against the actual array size and indexing scheme used by DocStatusStats and SearchResponseStatusStats. public void testStatusCountersAreIncludedInStatsSnapshots() { final IndicesService indicesService = getIndicesService(); final DocStatusStats docStatusStats = new DocStatusStats(); docStatusStats.add(RestStatus.CREATED, 2L); indicesService.addDocStatusStats(docStatusStats); indicesService.getSearchResponseStatusStats().add(RestStatus.OK, 3L); assertNotNull(indicesService.stats(CommonStatsFlags.ALL)); final StatusCounterStats statusCounters = indicesService.stats(CommonStatsFlags.NONE).getStatusCounterStats(); assertEquals( 2L, statusCounters.getDocStatusStats().getDocStatusCounter()[RestStatus.CREATED.getStatusFamilyCode() - 1].longValue() ); assertEquals( 3L, statusCounters.getSearchResponseStatusStats().getSearchResponseStatusCounter()[RestStatus.OK.getStatusFamilyCode() - 1] .longValue() ); } Missing Cleanup In testIndexServiceLookupsRequireMatchingUUID and testCreateIndexRejectsExistingIndex, indicesService.createIndex(...) is called directly without cleanup (e.g., removing the created index after the test). Since getIndicesService() returns a shared node-level service, leftover indices could interfere with other tests if index names collide or if the service state is not reset between tests. public void testIndexServiceLookupsRequireMatchingUUID() throws IOException { IndicesService indicesService = getIndicesService(); IndexMetadata metadata = newIndexMetadata("lookup-test", UUIDs.randomBase64UUID()); IndexService indexService = indicesService.createIndex(metadata, Collections.emptyList(), false); assertTrue(indicesService.hasIndex(metadata.getIndex())); assertSame(indexService, indicesService.indexService(metadata.getIndex())); assertSame(indexService, indicesService.indexServiceSafe(metadata.getIndex())); Index sameNameDifferentUUID = new Index(metadata.getIndex().getName(), UUIDs.randomBase64UUID()); assertFalse(indicesService.hasIndex(sameNameDifferentUUID)); assertNull(indicesService.indexService(sameNameDifferentUUID)); expectThrows(IndexNotFoundException.class, () -> indicesService.indexServiceSafe(sameNameDifferentUUID)); } public void testCreateIndexRejectsExistingIndex() throws IOException { IndicesService indicesService = getIndicesService(); IndexMetadata metadata = newIndexMetadata("create-test", UUIDs.randomBase64UUID()); indicesService.createIndex(metadata, Collections.emptyList(), false); ResourceAlreadyExistsException duplicate = expectThrows( ResourceAlreadyExistsException.class, () -> indicesService.createIndex(metadata, Collections.emptyList(), false) ); assertThat(duplicate.getMessage(), containsString("already exists")); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ab151f9

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Stabilize sequential mock state assertions The test calls cacheEntity.isOpen() twice expecting different results, relying on when(indexShard.state()).thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED) to return different values on successive calls. However, if isOpen() internally calls state() more than once per invocation (e.g., for other checks), the stubbing order could be consumed unexpectedly, making the second assertion unreliable. Consider capturing the result of each isOpen() call into a separate variable with an explicit mock setup to make the intent clear and the test deterministic. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [766-770] assertSame(requestCache, cacheEntity.stats()); assertSame(indexShard, cacheEntity.getCacheIdentity()); assertTrue(cacheEntity.ramBytesUsed() > 0L); +when(indexShard.state()).thenReturn(IndexShardState.STARTED); assertTrue(cacheEntity.isOpen()); +when(indexShard.state()).thenReturn(IndexShardState.CLOSED); assertFalse(cacheEntity.isOpen()); Suggestion importance[1-10]: 5 __ Why: The concern about when(...).thenReturn(A, B) being consumed unexpectedly is valid if isOpen() calls state() multiple times internally. Explicitly resetting the mock between calls makes the test more deterministic and intent clearer, though this is a minor robustness improvement.	Low
	Clean up created index after test assertion The test creates a permanent index and then expects withTempIndexService to throw ResourceAlreadyExistsException. However, the created indexService is never cleaned up after the test, which may leave the node in a dirty state and interfere with other tests. Consider removing or closing the index after the assertion. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [307-328] public void testTempIndexServiceIsClosedWithoutRegistration() throws Exception { IndicesService indicesService = getIndicesService(); IndexMetadata metadata = newIndexMetadata("temp-test", UUIDs.randomBase64UUID()); AtomicBoolean consumerCalled = new AtomicBoolean(false); String indexUUID = indicesService.withTempIndexService(metadata, tempIndexService -> { consumerCalled.set(true); assertEquals(metadata.getIndex(), tempIndexService.index()); assertFalse(indicesService.hasIndex(metadata.getIndex())); assertNull(indicesService.indexService(metadata.getIndex())); return tempIndexService.indexUUID(); }); assertTrue(consumerCalled.get()); assertEquals(metadata.getIndexUUID(), indexUUID); assertFalse(indicesService.hasIndex(metadata.getIndex())); assertNull(indicesService.indexService(metadata.getIndex())); IndexService indexService = indicesService.createIndex(metadata, Collections.emptyList(), false); assertSame(indexService, indicesService.indexServiceSafe(metadata.getIndex())); expectThrows(ResourceAlreadyExistsException.class, () -> indicesService.withTempIndexService(metadata, tempIndexService -> null)); + indicesService.removeIndex(metadata.getIndex(), "test cleanup", IndexRemovalReason.NO_LONGER_ASSIGNED); } Suggestion importance[1-10]: 3 __ Why: Cleaning up test state is a good practice, but since this is an OpenSearchSingleNodeTestCase, the node is typically reset between tests, making this cleanup less critical. The improved_code also introduces IndexRemovalReason which may not be imported, making the suggestion potentially incomplete.	Low
Possible issue	Fix incorrect status counter array index calculation RestStatus.getStatusFamilyCode() returns the HTTP status family (e.g., 2 for 2xx), so subtracting 1 gives index 1 for both CREATED (201) and OK (200). Both statuses belong to the 2xx family, so both counters are stored at the same array index. This means the test may not be verifying the correct per-status slot. Use the actual status code or a dedicated index based on the status value to correctly identify the counter slot. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [734-742] assertEquals( 2L, - statusCounters.getDocStatusStats().getDocStatusCounter()[RestStatus.CREATED.getStatusFamilyCode() - 1].longValue() + statusCounters.getDocStatusStats().getDocStatusCounter()[RestStatus.CREATED.getStatus() - 200].longValue() ); assertEquals( 3L, - statusCounters.getSearchResponseStatusStats().getSearchResponseStatusCounter()[RestStatus.OK.getStatusFamilyCode() - 1] + statusCounters.getSearchResponseStatusStats().getSearchResponseStatusCounter()[RestStatus.OK.getStatus() - 200] .longValue() ); Suggestion importance[1-10]: 3 __ Why: The suggestion points out that getStatusFamilyCode() returns the HTTP family (e.g., 2 for 2xx), so both CREATED and OK map to the same index. However, the improved_code uses getStatus() - 200 which is also not necessarily the correct indexing scheme without knowing the actual array structure of getDocStatusCounter(). The suggestion may be identifying a real issue but the fix is speculative.	Low

---

Previous suggestions

Suggestions up to commit a563d6a

Category	Suggestion	Impact
General	Clarify sequential mock state assertions ordering The test calls cacheEntity.isOpen() twice expecting different results, relying on the mock returning IndexShardState.STARTED first and IndexShardState.CLOSED second. However, isOpen() may not be called in the exact order assumed, and the mock stub when(indexShard.state()).thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED) only works correctly if state() is called exactly once per isOpen() invocation. Consider storing the results of each isOpen() call in separate variables to make the ordering explicit and the test intent clearer. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [766-770] assertSame(requestCache, cacheEntity.stats()); assertSame(indexShard, cacheEntity.getCacheIdentity()); assertTrue(cacheEntity.ramBytesUsed() > 0L); -assertTrue(cacheEntity.isOpen()); -assertFalse(cacheEntity.isOpen()); +boolean firstCall = cacheEntity.isOpen(); +boolean secondCall = cacheEntity.isOpen(); +assertTrue(firstCall); +assertFalse(secondCall); Suggestion importance[1-10]: 5 __ Why: The suggestion improves test clarity by storing the results of sequential isOpen() calls in separate variables before asserting, making the ordering explicit. The mock stub when(indexShard.state()).thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED) does work correctly with Mockito's sequential stubbing, but the refactoring makes the intent clearer.	Low
	Use consistent receiver for static-like utility method call The test calls getIndicesService().statsByShard(mockIndicesService, ...) but the method under test is statsByShard which is a static-like utility that operates on the passed mockIndicesService. Using the real IndicesService instance to invoke statsByShard with a mocked service is inconsistent with the existing test pattern (which uses indicesService.statsByShard(mockIndicesService, ...)). This could mask bugs if statsByShard uses instance state. Use mockIndicesService as the receiver or be consistent with the existing test pattern. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [719-721] -final Map<Index, List<IndexShardStats>> indexStats = getIndicesService().statsByShard(mockIndicesService, CommonStatsFlags.ALL); +final Map<Index, List<IndexShardStats>> indexStats = IndicesService.statsByShard(mockIndicesService, CommonStatsFlags.ALL); assertThat(indexStats.get(index), equalTo(Collections.singletonList(expectedShardStats))); Suggestion importance[1-10]: 4 __ Why: The suggestion points out an inconsistency where getIndicesService() is used as the receiver instead of mockIndicesService or a local variable. However, statsByShard is an instance method and the existing test pattern in the file also uses a real IndicesService instance to call statsByShard with a mock, so the improved code suggesting a static call may not compile correctly.	Low
	Clean up registered index after test assertion After createIndex is called with metadata, the index is registered. The test then expects withTempIndexService to throw ResourceAlreadyExistsException. However, there is no cleanup of the created IndexService after the test, which may cause interference with other tests sharing the same IndicesService instance. Consider removing or closing the created index after the assertion to avoid test pollution. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [327] expectThrows(ResourceAlreadyExistsException.class, () -> indicesService.withTempIndexService(metadata, tempIndexService -> null)); +indicesService.removeIndex(metadata.getIndex(), IndexRemovalReason.NO_LONGER_ASSIGNED, "test cleanup"); Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about test pollution, but the improved_code references IndexRemovalReason which may not be imported or available, and the cleanup approach may not be accurate. Additionally, test isolation in OpenSearchSingleNodeTestCase typically handles cleanup between tests.	Low

Suggestions up to commit 457e2a6

Category	Suggestion	Impact
Possible issue	Fix incorrect status counter array index calculation RestStatus.CREATED.getStatusFamilyCode() returns 2 (the 2xx family code), so the index used is 1. However, DocStatusStats counters are typically indexed by the exact HTTP status code (e.g., 201), not the family code. Using the family code as an array index may access the wrong counter bucket and cause the assertion to pass vacuously or fail unexpectedly. Verify the correct indexing scheme for DocStatusStats and use the appropriate index (e.g., RestStatus.CREATED.getStatus() - 200 or a dedicated accessor). server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [734-742] assertEquals( 2L, - statusCounters.getDocStatusStats().getDocStatusCounter()[RestStatus.CREATED.getStatusFamilyCode() - 1].longValue() + statusCounters.getDocStatusStats().getDocStatusCounter()[RestStatus.CREATED.getStatus() - 200].longValue() ); assertEquals( 3L, - statusCounters.getSearchResponseStatusStats().getSearchResponseStatusCounter()[RestStatus.OK.getStatusFamilyCode() - 1] + statusCounters.getSearchResponseStatusStats().getSearchResponseStatusCounter()[RestStatus.OK.getStatus() - 200] .longValue() ); Suggestion importance[1-10]: 6 __ Why: This is a potentially valid concern - if DocStatusStats counters are indexed by exact HTTP status codes rather than family codes, using getStatusFamilyCode() - 1 would access the wrong bucket. However, the suggestion's improved_code using getStatus() - 200 is speculative without knowing the actual DocStatusStats implementation, and the existing test pattern may be intentionally using family codes if that's how the counter is designed.	Low
	Fix fragile mock state ordering for open/closed assertions The test calls cacheEntity.isOpen() twice expecting different results, relying on the mock returning IndexShardState.STARTED first and IndexShardState.CLOSED second. However, isOpen() may not be called exactly once per assertion — if the implementation calls state() multiple times internally, the stubbing order could be consumed unexpectedly. Consider using when(indexShard.state()).thenReturn(IndexShardState.STARTED).thenReturn(IndexShardState.CLOSED) and verifying the call count with verify(indexShard, times(2)).state() to make the ordering contract explicit and robust. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [766-770] assertSame(requestCache, cacheEntity.stats()); assertSame(indexShard, cacheEntity.getCacheIdentity()); assertTrue(cacheEntity.ramBytesUsed() > 0L); +when(indexShard.state()).thenReturn(IndexShardState.STARTED); assertTrue(cacheEntity.isOpen()); +when(indexShard.state()).thenReturn(IndexShardState.CLOSED); assertFalse(cacheEntity.isOpen()); Suggestion importance[1-10]: 5 __ Why: The mock is set up with thenReturn(IndexShardState.STARTED, IndexShardState.CLOSED) which already handles sequential calls correctly in Mockito. However, if isOpen() internally calls state() more than once, the ordering could be consumed unexpectedly. The suggestion to reset the mock state between assertions is a valid defensive improvement, though the current code may work correctly depending on the implementation.	Low
	Ensure statsByShard operates on the mocked service The test calls getIndicesService().statsByShard(mockIndicesService, CommonStatsFlags.ALL) — it invokes statsByShard on the real IndicesService instance but passes a fully mocked mockIndicesService as the argument. If statsByShard is a static-style method that delegates to the passed service, this may work, but if it uses this internally, the mock's iterator will never be consulted. Verify that statsByShard is designed to operate on the passed argument rather than this; if not, the call should be mockIndicesService.statsByShard(mockIndursService, CommonStatsFlags.ALL) or the mock setup is incorrect. server/src/test/java/org/opensearch/indices/IndicesServiceTests.java [719-721] -public void testStatsByShardSkipsNullShardStats() { - ... - final Map<Index, List<IndexShardStats>> indexStats = getIndicesService().statsByShard(mockIndicesService, CommonStatsFlags.ALL); +final Map<Index, List<IndexShardStats>> indexStats = IndicesService.statsByShard(mockIndicesService, CommonStatsFlags.ALL); - assertThat(indexStats.get(index), equalTo(Collections.singletonList(expectedShardStats))); -} +assertThat(indexStats.get(index), equalTo(Collections.singletonList(expectedShardStats))); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about whether statsByShard uses this or the passed argument internally. However, looking at the existing test testStatsByShardDoesNotDieFromExpectedExceptions (line 697-703), it uses the same pattern of calling getIndicesService().statsByShard(mockIndicesService, ...), suggesting this is the established pattern and likely works correctly. The improved_code changes to a static call which may not compile if statsByShard is not static.	Low

[github-actions]

Persistent review updated to latest commit a563d6a

[github-actions]

❌ Gradle check result for a563d6a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit ab151f9

[github-actions]

❌ Gradle check result for ab151f9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?