Enhance robustness of InternalOpenSearchSink tests through scenario-driven coverage, refactoring, and regression prevention by pCastq · Pull Request #6146 · opensearch-project/security

pCastq · 2026-05-16T00:48:19Z

Summary

Adds integration and unit tests covering previously untested branches in InternalOpenSearchSink,
and refactors test classes to eliminate duplication using the Template Method pattern.
This improves overall robustness and helps prevent regressions.

Problem & Solution

Existing State

The codebase contains InternalOpenSearchSinkIntegrationTestAuditAlias, an integration test
that covers the metadata.hasAlias() == true branch with a real cluster configured with a write alias.

Coverage Gap

The complementary path—where the sink must create a concrete index with automatic date-based naming—
was not tested. Additionally, cluster-level failure scenarios (race conditions, acknowledged=false,
generic exceptions) could not be reliably reproduced on a single-node embedded cluster.

Solution

Three coordinated changes eliminate duplication and complete coverage:

New integration test: InternalOpenSearchSinkIntegrationTestConcreteIndex
exercises index creation on a plain cluster without aliases, covering the default path where
indices follow the security-auditlog-YYYY.MM.dd pattern.
Extract commonality: AbstractInternalOpenSearchSinkIntegrationTest
holds the shared test methods (testPersistsAuditEventsToTarget, testAuditDocumentContainsMandatoryFields)
that are valid regardless of cluster configuration. Both concrete-index and alias variants
inherit and reuse these tests through method overrides (cluster(), auditTarget()),
eliminating 68 lines of duplicate code.
Unit test hard-to-reproduce scenarios: InternalOpenSearchSinkTest
uses Mockito to cover branches that cannot be reliably triggered on an embedded cluster:
- Race condition: two nodes concurrently creating the same index → ResourceAlreadyExistsException must be treated as success
- Cluster failures: network, authorization, or I/O errors → must be absorbed and return false
- acknowledged=false: cluster manager creates the index but times out on shard propagation → must return false gracefully

Changes

New Test Classes

InternalOpenSearchSinkIntegrationTestConcreteIndex:
covers the index creation path with a plain, unaliased cluster; verifies that indices
are created with the default date-based pattern (security-auditlog-YYYY.MM.dd)
InternalOpenSearchSinkTest:
unit tests with Mockito covering non-reproducible cluster scenarios:
race conditions (ResourceAlreadyExistsException), generic cluster failures,
and unacknowledged responses

Refactored Test Classes

AbstractInternalOpenSearchSinkIntegrationTest:
extracted shared integration tests that work against both concrete-index and alias variants;
subclasses override cluster() and auditTarget() to inject their configuration.
Eliminates duplication; each variant now contributes only its specialized behavior.
InternalOpenSearchSinkIntegrationTestAuditAlias:
refactored to extend the abstract base; removed 3 duplicate methods
(countAuditDocs, generateAuditEvent, testAuditDocumentsViaAliasContainMandatoryFields);
now focuses solely on its unique test: testRecognizesAuditTargetAsWriteAlias

Testing

All existing tests continue to pass. New tests provide complete branch coverage
for InternalOpenSearchSink.createIndexIfAbsent().

Issues Resolved

Closes #6145

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

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.

…Sink Signed-off-by: Pietro Paolo Castagna <PietroPaolo.Castagna@gmail.com>

github-actions · 2026-05-16T00:49:29Z

PR Reviewer Guide 🔍

(Review updated until commit `07b2132`)

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: Refactor integration tests with shared base class and add concrete index variant Relevant files: src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java src/integrationTest/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkIntegrationTestAuditAlias.java src/integrationTest/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkIntegrationTestConcreteIndex.java Sub-PR theme: Add unit tests for exception and race-condition branches in InternalOpenSearchSink Relevant files: src/test/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkTest.java
⚡ Recommended focus areas for review Shared State Risk The `testCreatesAuditIndexAutomatically()` test calls `countAuditDocs(client)` before generating an event to capture a baseline, but the inherited `testPersistsAuditEventsToTarget()` and `testAuditDocumentContainsMandatoryFields()` tests also write to the same `security-auditlog-` index pattern. Since JUnit does not guarantee test execution order, if `testCreatesAuditIndexAutomatically()` runs after the other tests, the index already exists and the "first write creates the index" code path is not actually exercised. The test would still pass (the index exists), but the intended code path (both `hasAlias()` and `hasIndex()` returning false) would be skipped silently. @Test public void testCreatesAuditIndexAutomatically() { try (Client client = CLUSTER.getInternalNodeClient()) { long before = countAuditDocs(client); generateAuditEvent("_cluster/health"); await().atMost(10, SECONDS).pollInterval(200, MILLISECONDS).untilAsserted(() -> { refreshAuditTarget(client); assertThat("At least one new audit event must be generated", countAuditDocs(client), greaterThan(before)); }); await().atMost(10, SECONDS).pollInterval(200, MILLISECONDS).untilAsserted(() -> { GetIndexResponse response = client.admin() .indices() .getIndex(new GetIndexRequest().indices(AUDIT_INDEX_PREFIX + "")) .actionGet(); assertThat("At least one audit index must exist", response.indices().length, greaterThan(0)); assertThat( "All audit indices must follow date-based pattern security-auditlog-YYYY.MM.dd", Arrays.stream(response.indices()).allMatch(name -> name.matches(AUDIT_INDEX_PREFIX + "\\d{4}\\.\\d{2}\\.\\d{2}$")), is(true) ); }); } } Missing Refresh Before Count In `testAuditDocumentContainsMandatoryFields()`, after the `await()` loop confirms at least one new document exists, a `SearchRequest` is issued immediately to fetch the document for field validation. However, there is no explicit refresh between the `await()` completion and the final search. The `await()` loop calls `refreshAuditTarget()` internally, but the final search outside the loop could theoretically miss the document if a new segment was written between the last refresh in the loop and the final search. This is a low-probability race but could cause intermittent test failures. SearchResponse response = client.search( new SearchRequest(auditTarget()).source( new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(1).sort("@timestamp", SortOrder.DESC) ) ).actionGet();

github-actions · 2026-05-16T00:49:50Z

PR Code Suggestions ✨

Latest suggestions up to 07b2132

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Tolerate missing indices during polling When the audit target uses a wildcard pattern (e.g. `security-auditlog-`) and no matching index exists yet, both `search` and `prepareRefresh` will throw `IndexNotFoundException` unless `allowNoIndices`/`ignoreUnavailable` are set. This will cause `testPersistsAuditEventsToTarget` to fail intermittently when the test runs before any index is created. Configure `IndicesOptions.lenientExpandOpen()` (or equivalent) on both requests to gracefully handle the "no index yet" state during polling. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [75-87] /* Counts all audit documents reachable through {@link #auditTarget()}. / long countAuditDocs(Client client) { - return Objects.requireNonNull( - client.search(new SearchRequest(auditTarget()).source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0))) - .actionGet() - .getHits() - .getTotalHits() - ).value(); + SearchRequest request = new SearchRequest(auditTarget()) + .indicesOptions(IndicesOptions.lenientExpandOpen()) + .source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0)); + return Objects.requireNonNull(client.search(request).actionGet().getHits().getTotalHits()).value(); } /* Refreshes the audit target so newly indexed documents become searchable. */ void refreshAuditTarget(Client client) { - client.admin().indices().prepareRefresh(auditTarget()).get(); + client.admin().indices().prepareRefresh(auditTarget()) + .setIndicesOptions(IndicesOptions.lenientExpandOpen()).get(); } Suggestion importance[1-10]: 7 __ Why: Valid concern: when the audit index does not yet exist, the wildcard search and refresh may throw `IndexNotFoundException`, causing flaky polling in `testPersistsAuditEventsToTarget`. Adding `IndicesOptions.lenientExpandOpen()` is a reasonable robustness improvement.	Medium
General	Close Mockito mocks to prevent leaks `LuceneTestCase` enforces strict thread/resource leak checks and assumes JUnit lifecycle conventions that may conflict with `@Before`/`MockitoAnnotations.openMocks` (e.g., the returned `AutoCloseable` is leaked, which can fail leak detectors and leave Mockito state between tests). Either close the mocks in an `@After` method or extend a plain test base (or none) to avoid spurious leak-detection failures. src/test/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkTest.java [65] public class InternalOpenSearchSinkTest extends LuceneTestCase { + private AutoCloseable mocks; + + @Before + public void setupSink() { + mocks = MockitoAnnotations.openMocks(this); + // ... existing setup ... + } + + @After + public void tearDown() throws Exception { + mocks.close(); + super.tearDown(); + } + Suggestion importance[1-10]: 5 __ Why: Closing the `AutoCloseable` returned by `MockitoAnnotations.openMocks` is good practice to avoid resource leaks, especially when extending `LuceneTestCase` which has strict leak detection. Moderate impact on test reliability.	Low

Previous suggestions

Suggestions up to commit 20b49f8

Category	Suggestion	Impact
Possible issue	Avoid IndexNotFoundException on initial count When the audit target is a wildcard pattern like `security-auditlog-` and no matching index exists yet, the search request will fail with `IndexNotFoundException`. Wrap the search with `IndicesOptions.lenientExpandOpen()` (or equivalent) so the initial `countAuditDocs` call before any event is generated returns 0 instead of throwing. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [75-82] /* Counts all audit documents reachable through {@link #auditTarget()}. / long countAuditDocs(Client client) { - return Objects.requireNonNull( - client.search(new SearchRequest(auditTarget()).source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0))) - .actionGet() - .getHits() - .getTotalHits() - ).value(); + SearchRequest request = new SearchRequest(auditTarget()).source( + new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0) + ); + request.indicesOptions(org.opensearch.action.support.IndicesOptions.lenientExpandOpen()); + return Objects.requireNonNull(client.search(request).actionGet().getHits().getTotalHits()).value(); } Suggestion importance[1-10]: 6 __ Why: Valid concern: when `auditTarget()` returns the wildcard `security-auditlog-` and no index exists yet, the initial `countAuditDocs` call in `testPersistsAuditEventsToTarget` could throw `IndexNotFoundException`. Wildcards typically expand leniently by default, but making it explicit improves test robustness.	Low
General	Use lenient options when refreshing target `prepareRefresh` against a wildcard pattern may throw `IndexNotFoundException` if no audit index exists yet (e.g., before the first event is persisted). Configure lenient indices options so polling assertions don't fail spuriously while the index is still being created. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [85-87] /** Refreshes the audit target so newly indexed documents become searchable. */ void refreshAuditTarget(Client client) { - client.admin().indices().prepareRefresh(auditTarget()).get(); + client.admin().indices().prepareRefresh(auditTarget()) + .setIndicesOptions(org.opensearch.action.support.IndicesOptions.lenientExpandOpen()) + .get(); } Suggestion importance[1-10]: 6 __ Why: Refreshing a wildcard pattern before any matching index exists can fail; using lenient indices options would make the polling assertions more reliable in the concrete-index variant where indices are created on first write.	Low

Suggestions up to commit 09c4343

Category	Suggestion	Impact
General	Filter audit search to the generated event The search uses `matchAllQuery()` and sorts by `@timestamp` desc, returning the most recent doc across all events—possibly from another test sharing the cluster, not necessarily the one generated by `_cluster/health`. Filter by `audit_rest_request_path` to ensure assertions target the document this test generated. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [158-162] @Test public void testAuditDocumentContainsMandatoryFields() { try (Client client = cluster().getInternalNodeClient()) { long before = countAuditDocs(client); generateAuditEvent("_cluster/health"); await().atMost(10, SECONDS).pollInterval(200, MILLISECONDS).untilAsserted(() -> { refreshAuditTarget(client); assertThat("Test must generate at least one new event", countAuditDocs(client) - before, greaterThan(0L)); }); SearchResponse response = client.search( new SearchRequest(auditTarget()).source( - new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(1).sort("@timestamp", SortOrder.DESC) + new SearchSourceBuilder() + .query(QueryBuilders.termQuery("audit_rest_request_path", "/_cluster/health")) + .size(1) + .sort("@timestamp", SortOrder.DESC) ) ).actionGet(); Suggestion importance[1-10]: 7 __ Why: Valid concern: sorting by `@timestamp` desc on `matchAllQuery()` may return a document from a different test or event, making assertions less reliable. Filtering by `audit_rest_request_path` improves test correctness.	Medium
	Avoid wildcard static imports Wildcard imports from `org.hamcrest.Matchers` are generally discouraged in this codebase (other files use explicit imports) and can mask name collisions. Replace with explicit imports of the specific matchers used (`greaterThan`, `greaterThanOrEqualTo`, `is`). src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [30] -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.is; Suggestion importance[1-10]: 3 __ Why: Minor style improvement; explicit imports are preferred for consistency but the wildcard import is functionally correct.	Low
	Document or replace null constructor arguments Passing `null` for several constructor arguments is brittle—if `InternalOpenSearchSink`'s constructor (or any code reachable from `createIndexIfAbsent`) starts dereferencing those parameters, all three tests will fail with `NullPointerException` and obscure the actual branch under test. Consider passing minimally valid stubs or documenting which arguments are intentionally unused for these tests. src/test/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkTest.java [99-108] +// NOTE: null arguments are unused by createIndexIfAbsent under test; +// revisit if the constructor or method begins to dereference them. sink = new InternalOpenSearchSink( "test-sink", Settings.EMPTY, null, null, client, threadPool, null, clusterService ); Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment without functional changes; the null arguments are intentional for the unit test scope.	Low

Suggestions up to commit f34cc61

Category	Suggestion	Impact
General	Verify REST request succeeds The `restClient.get(path)` call may fail silently if the request returns an error status code, potentially causing flaky test failures. Verify the response status or catch exceptions to ensure the audit event was actually generated before the test proceeds. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [88-92] void generateAuditEvent(String path) { try (TestRestClient restClient = cluster().getRestClient(cluster().getAdminCertificate())) { - restClient.get(path); + TestRestClient.HttpResponse response = restClient.get(path); + response.assertStatusCode(200); } } Suggestion importance[1-10]: 7 __ Why: Verifying the response status ensures the audit event was actually generated, preventing flaky test failures. This is important for test reliability, though the impact is moderate since the tests use `await()` to handle timing issues.	Medium
	Close mock resources after tests The `MockitoAnnotations.openMocks(this)` call returns an `AutoCloseable` resource that should be closed after each test to prevent resource leaks. Store the returned `AutoCloseable` in a field and close it in an `@After` method to ensure proper cleanup. src/test/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkTest.java [90-109] +private AutoCloseable mocks; + @Before public void setupSink() { - MockitoAnnotations.openMocks(this); + mocks = MockitoAnnotations.openMocks(this); when(clusterService.state()).thenReturn(clusterState); when(clusterState.metadata()).thenReturn(metadata); when(metadata.hasAlias(TEST_INDEX)).thenReturn(false); when(metadata.hasIndex(TEST_INDEX)).thenReturn(false); when(client.admin()).thenReturn(adminClient); when(adminClient.indices()).thenReturn(indicesAdminClient); sink = new InternalOpenSearchSink( "test-sink", Settings.EMPTY, null, null, client, threadPool, null, clusterService ); } +@After +public void tearDown() throws Exception { + if (mocks != null) { + mocks.close(); + } +} + Suggestion importance[1-10]: 6 __ Why: Closing the `AutoCloseable` returned by `MockitoAnnotations.openMocks()` prevents potential resource leaks. While this is good practice, the impact is limited since test JVMs are typically short-lived and Mockito's resource usage is minimal.	Low
	Add descriptive null-check error message The `Objects.requireNonNull()` call will throw `NullPointerException` if `getTotalHits()` returns `null`, but this provides no context about which audit target failed. Add a descriptive error message to the `requireNonNull()` call to aid debugging when the search response is malformed. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [73-80] long countAuditDocs(Client client) { return Objects.requireNonNull( client.search(new SearchRequest(auditTarget()).source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0))) .actionGet() .getHits() - .getTotalHits() + .getTotalHits(), + "getTotalHits() returned null for audit target: " + auditTarget() ).value(); } Suggestion importance[1-10]: 4 __ Why: Adding a descriptive error message to `Objects.requireNonNull()` improves debugging when `getTotalHits()` returns `null`. However, this is a minor enhancement since such failures are rare in well-formed search responses and the stack trace would still identify the location.	Low

Suggestions up to commit b0ed4d2

Category	Suggestion	Impact
General	Strengthen date pattern validation The regex pattern `\d{4}\.\d{2}\.\d{2}$` accepts invalid dates like `9999.99.99`. Consider validating that the date components represent actual calendar dates (e.g., month 01-12, day 01-31) to prevent false positives when the index naming deviates from expected format. src/integrationTest/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkIntegrationTestConcreteIndex.java [106-110] assertThat( "All audit indices must follow date-based pattern security-auditlog-YYYY.MM.dd", - Arrays.stream(response.indices()).allMatch(name -> name.matches(AUDIT_INDEX_PREFIX + "\\d{4}\\.\\d{2}\\.\\d{2}$")), + Arrays.stream(response.indices()).allMatch(name -> + name.matches(AUDIT_INDEX_PREFIX + "\\d{4}\\.(0[1-9]\|1[0-2])\\.(0[1-9]\|[12]\\d\|3[01])$") + ), is(true) ); Suggestion importance[1-10]: 6 __ Why: The improved regex validates month (01-12) and day (01-31) ranges, preventing false positives like `9999.99.99`. This strengthens the test assertion, though the current pattern is likely sufficient for detecting major naming issues in practice.	Low
General	Add descriptive error message The `Objects.requireNonNull()` call will throw `NullPointerException` if `getTotalHits()` returns `null`, but this provides no context about which component failed. Consider adding a descriptive error message to the `requireNonNull()` call to aid debugging when the search response structure is unexpected. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [73-80] long countAuditDocs(Client client) { return Objects.requireNonNull( client.search(new SearchRequest(auditTarget()).source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0))) .actionGet() .getHits() - .getTotalHits() + .getTotalHits(), + "getTotalHits() returned null for audit target: " + auditTarget() ).value(); } Suggestion importance[1-10]: 5 __ Why: Adding a descriptive error message to `Objects.requireNonNull()` improves debugging when `getTotalHits()` returns `null`. However, this is a minor enhancement since such failures are rare in integration tests and the stack trace would still identify the location.	Low

Suggestions up to commit f345baf

Category	Suggestion	Impact
General	Verify response status in audit event generation The method silently ignores any exceptions thrown by `restClient.get(path)`, including authentication failures or network errors. This can lead to flaky tests where audit events are not generated but the test continues without detection. Verify the response status or propagate exceptions to ensure event generation succeeded. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [88-92] void generateAuditEvent(String path) { try (TestRestClient restClient = cluster().getRestClient(cluster().getAdminCertificate())) { - restClient.get(path); + TestRestClient.HttpResponse response = restClient.get(path); + if (response.getStatusCode() >= 400) { + throw new IllegalStateException("Failed to generate audit event for path: " + path + + ", status: " + response.getStatusCode()); + } } } Suggestion importance[1-10]: 7 __ Why: Valid concern about silent failures in test helper methods. Verifying the response status would make tests more robust and failures easier to diagnose. The improved code properly checks for error status codes and provides clear error messages.	Medium
General	Document null constructor parameters with comments Passing multiple `null` arguments to the constructor makes the test fragile and unclear about which dependencies are actually required. If the constructor validates these parameters or uses them in `createIndexIfAbsent()`, the test may fail unexpectedly. Use mock objects or verify that `null` is acceptable for these specific parameters. src/test/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSinkTest.java [90-99] sink = new InternalOpenSearchSink( "test-sink", Settings.EMPTY, - null, - null, + null, // auditLogImpl - not used in createIndexIfAbsent + null, // fallbackSink - not used in createIndexIfAbsent client, threadPool, - null, + null, // auditConfig - not used in createIndexIfAbsent clusterService ); Suggestion importance[1-10]: 4 __ Why: While adding comments to clarify `null` parameters improves code readability, this is a minor documentation enhancement. The test is already working correctly, and the suggestion doesn't address any functional issue or bug.	Low
Possible issue	Add explicit null handling for total hits The `Objects.requireNonNull()` call will throw `NullPointerException` if `getTotalHits()` returns `null`, which can occur in certain OpenSearch versions or edge cases. This will cause test failures with unclear error messages. Add explicit null handling with a descriptive error message or return a default value. src/integrationTest/java/org/opensearch/security/auditlog/sink/AbstractInternalOpenSearchSinkIntegrationTest.java [73-80] long countAuditDocs(Client client) { - return Objects.requireNonNull( - client.search( - new SearchRequest(auditTarget()) - .source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0)) - ).actionGet().getHits().getTotalHits() - ).value(); + var totalHits = client.search( + new SearchRequest(auditTarget()) + .source(new SearchSourceBuilder().query(QueryBuilders.matchAllQuery()).size(0)) + ).actionGet().getHits().getTotalHits(); + + if (totalHits == null) { + throw new IllegalStateException("Search response returned null total hits for audit target: " + auditTarget()); + } + return totalHits.value(); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that `getTotalHits()` could return `null`, and the improved code provides better error handling with a descriptive message. However, `Objects.requireNonNull()` already throws `NullPointerException` with the object reference, so this is more of a code quality improvement than a critical fix.	Low

…riven coverage, refactoring, and regression prevention: - Add InternalOpenSearchSinkIntegrationTestConcreteIndex: covers the index creation path with a plain concrete date-based index (no alias) - Extract shared tests into AbstractInternalOpenSearchSinkIntegrationTest using Template Method pattern: testPersistsAuditEventsToTarget and testAuditDocumentContainsMandatoryFields now run against both concrete-index and alias variants without duplication - Refactor InternalOpenSearchSinkIntegrationTestAuditAlias: now extends the abstract base, removing 3 duplicate methods (countAuditDocs, generateAuditEvent, testAuditDocumentsViaAliasContainMandatoryFields); retains only the specialized test testRecognizesAuditTargetAsWriteAlias - Add InternalOpenSearchSinkTest unit tests: covers race condition (ResourceAlreadyExistsException), generic cluster failures, and acknowledged=false scenarios that cannot be tested reliably on single-node embedded cluster - Fix resource leak in testCreatesAuditIndexAutomatically Closes opensearch-project#6145 Signed-off-by: Pietro Paolo Castagna <PietroPaolo.Castagna@gmail.com>

github-actions · 2026-05-16T01:05:14Z

Persistent review updated to latest commit f345baf

Signed-off-by: Pietro Paolo Castagna <PietroPaolo.Castagna@gmail.com>

github-actions · 2026-05-21T14:56:53Z

Persistent review updated to latest commit b0ed4d2

cwperks · 2026-06-10T12:48:48Z

+ *   <li>{@link InternalOpenSearchSinkIntegrationTestAuditAlias}</li>
+ * </ul>
+ */
+@RunWith(MockitoJUnitRunner.class)


Can we extend LuceneTestCase instead of using MockitoJUnitRunner here? I've recently been trying to make this repo adhere with the testingConventions check from OpenSearch core. See https://github.com/opensearch-project/security/pull/6205/changes for some examples on how to adapt it.

Thanks for sharing this. I’ve looked into it and read a bit about LuceneTestCase.
I’ve made the changes accordingly, hope everything looks good now.

Thanks a lot @cwperks !

…oject testing conventions

github-actions · 2026-06-19T00:24:56Z

Persistent review updated to latest commit f34cc61

github-actions · 2026-06-23T22:44:07Z

Persistent review updated to latest commit 09c4343

DarshitChanpura

@pCastq Changes look good overall. Left a couple of comments.

For code-hygiene, could you run:

$ ./gradlew spotlessApply

$ ./gradlew checkstyleIntegrationTest  -- ensure this passes locally

DarshitChanpura · 2026-06-23T22:44:34Z

+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.*;


imports will fail checkStyle scans. Please expand it to individual imports.

DarshitChanpura · 2026-06-23T22:53:07Z

+        try (Client client = cluster().getInternalNodeClient()) {
+            long before = countAuditDocs(client);
+
+            generateAuditEvent("_cluster/health");


can we add couple more events and test with "atleast" convention here.

github-actions · 2026-06-24T17:27:59Z

Persistent review updated to latest commit 20b49f8

…ckstyle fixes

github-actions · 2026-06-24T17:37:54Z

Persistent review updated to latest commit 07b2132

test(auditlog): add unit and integration tests for InternalOpenSearch…

6bf3ea4

…Sink Signed-off-by: Pietro Paolo Castagna <PietroPaolo.Castagna@gmail.com>

pCastq requested review from DarshitChanpura, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners May 16, 2026 00:48

pCastq force-pushed the test/improve-sink-test-coverage-and-structure branch from e23204b to f345baf Compare May 16, 2026 01:04

spotlessApply

b0ed4d2

Signed-off-by: Pietro Paolo Castagna <PietroPaolo.Castagna@gmail.com>

cwperks reviewed Jun 10, 2026

View reviewed changes

Migrate InternalOpenSearchSinkTest to LuceneTestCase to align with pr…

f34cc61

…oject testing conventions

pCastq requested a review from Rishav9852Kumar as a code owner June 19, 2026 00:23

pCastq requested a review from cwperks June 19, 2026 00:31

Merge branch 'main' into test/improve-sink-test-coverage-and-structure

09c4343

DarshitChanpura reviewed Jun 23, 2026

View reviewed changes

Refactor imports, add audit event to test, and apply spotless and che…

07b2132

…ckstyle fixes

pCastq force-pushed the test/improve-sink-test-coverage-and-structure branch from 20b49f8 to 07b2132 Compare June 24, 2026 17:36

Uh oh!

Conversation

pCastq commented May 16, 2026

Summary

Problem & Solution

Existing State

Coverage Gap

Solution

Changes

New Test Classes

Refactored Test Classes

Testing

Issues Resolved

Check List

Uh oh!

github-actions Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 07b2132)

Uh oh!

github-actions Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 16, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

cwperks Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

pCastq Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 16, 2026 •

edited

Loading

(Review updated until commit `07b2132`)

github-actions Bot commented May 16, 2026 •

edited

Loading