Repository navigation
MLE-33565 Resolve the 5 remaining disabled tests - #1984
hmadala001 wants to merge 7 commits into
Conversation
Stabilised: - TestOpticOnViews.testgroupByUnion now uses order-independent assertions - BulkIOInputCallerTest.bulkInputEndpointTestWithAnyDocument now uses its own collections, cleans up beforehand, and asserts on the union of written inputs rather than on how batches are distributed across call contexts Retired: - ClientApiFunctionalTest.TestE2ENumberOfConcurrentUsers: tests the app server concurrent-request-limit feature, not the Java Client; flaky on Jenkins - DocumentMetadataHandleTest.testMetadataPropertiesExtraction (and test.bin): tests a v1/documents feature and causes OOM in CI - SetMaxBatchesTest.testWithQuery: setMaxBatches with a query is a known limitation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the test-only changes that re-enable two tests and remove three disabled tests.
No open issues 🎉
What changed in this PR
Resolves five disabled tests in the Java Client’s core and functional test suites by stabilizing two and retiring three. No production code changes are included.
Changes:
- Re-enables the Optic union test with order-independent assertions.
- Re-enables the bulk-input test with isolated collections, cleanup, and assertions that every input is written exactly once.
- Removes three disabled tests covering server behavior or a known limitation, plus an unused fixture and helper.
| File | Description |
|---|---|
marklogic-client-api/src/test/resources/test.bin |
Removes the retired metadata test’s binary fixture. |
marklogic-client-api/src/test/java/com/marklogic/client/test/io/DocumentMetadataHandleTest.java |
Removes the disabled metadata-extraction test and related imports. |
marklogic-client-api/src/test/java/com/marklogic/client/test/dataservices/BulkIOInputCallerTest.java |
Isolates and re-enables the bulk-input test with distribution-independent assertions. |
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/fastfunctest/TestOpticOnViews.java |
Re-enables the union test and replaces order-sensitive row checks. |
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/fastfunctest/datamovement/SetMaxBatchesTest.java |
Removes the disabled query-based test while retaining iterator coverage. |
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/fastfunctest/ClientApiFunctionalTest.java |
Removes the disabled concurrent-request-limit test and its configuration helper. |
Review effort: Balanced
| } | ||
|
|
||
| @AfterAll | ||
| public static void cleanup(){ |
There was a problem hiding this comment.
I would rework this test class to extend AbstractClientTest which will clean the database before the test starts instead of after. That's a better practice because if the database is cleaned afterwards and the test fails, it's much harder to debug since the data is all gone. Extending that class also avoids duplication of deletion code like this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure, though that's actually not necessary either. I know now the dataservices tests do a lot of that - loading modules and then deleting them - but they could all be setup via the test-app as part of mlDeploy, and then the tests wouldn't need all the additional code. Not important to change, just an FYI.
Addresses review feedback: AbstractClientTest cleans the database before each test instead of after, so data from a failing test is still available for debugging, and it removes the duplicated document-deletion code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Validate Copyright Headers check requires modified files to carry the current year. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
testMetadataPropertiesExtraction was removed, but its one unique check, adding a custom property to a binary document whose properties were extracted and reading both back, is now part of BinaryDocumentTest.testReadWrite. That test already runs the metadata extraction in CI (with the same ML 11 and ARM conditions), so no additional converter invocation is introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the test instead of removing it. It verifies that the client surfaces the 403 returned by an app server with concurrent-request-limit=1 as a FailedRequestException. It passed consistently on repeated local runs; the original Jenkins-only failures were never reproduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The results list was a plain ArrayList that onUrisReady appended to from 4 threads, so concurrent adds could be lost (e.g. 10 results instead of 20). Use a synchronized list. Failed 5/12 runs before, 0/20 after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Nice work stabilizing these tests and migrating the custom-property metadata assertions into BinaryDocumentTest! Verified that no functional test coverage is lost for the retired tests (DocumentMetadataHandleTest.testMetadataPropertiesExtraction is covered in BinaryDocumentTest.testReadWrite, and QueryBatcher.setMaxBatches with a query is covered in QueryBatcherTest.maxUrisTestWithQueryTask and QueryBatcherJobReportTest.testJobReportSetMaxBatches). One issue worth addressing in ClientApiFunctionalTest.TestE2ENumberOfConcurrentUsers now that it is re-enabled: Thread-safety of msgEx in TestE2ENumberOfConcurrentUsers (ClientApiFunctionalTest.java line 309): StringBuilder msgEx = new StringBuilder(); is mutated concurrently by threads w1, w2, and w3 inside the catch (FailedRequestException ex) block without synchronization: java When two threads hit the concurrent request limit at the same time, unsynchronized StringBuilder.append(...) calls can race and corrupt the buffer (or throw ArrayIndexOutOfBoundsException inside a worker thread), which can cause intermittent assertion failures on Jenkins. Switching StringBuilder to StringBuffer (and moving the assertTrue(...) out of the finally block so finally only resets modifyConcurrentUsersOnHttpServer(serverName, 0)) will make this thread-safe: java Minor: The PR description still lists ClientApiFunctionalTest.TestE2ENumberOfConcurrentUsers under Retired, though commit a0e55d1 re-enabled it. |
addressed this |
Use StringBuffer for msgEx, which is appended to by concurrent worker threads, and move the assertion out of the finally block so it only resets the concurrent-request-limit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stabilised:
Retired: