MLE-26420 First test using marklogic-junit5 #1863
Conversation
rjrudin
requested review from
BillFarber,
anu3990 and
stevebio
as code owners
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR introduces initial integration with marklogic-junit5 for improved test data management and adds an experimental WriteBatcher template pattern. The main purpose is to establish a proper test cleanup mechanism that wipes data from the database before tests run, replacing the previous pattern of manual deletion in individual test classes.
Key changes:
- Introduces
AbstractClientTestbase class that automatically deletes test documents before each test run using marklogic-junit5 - Makes
DataMovementManagerimplementCloseablefor better resource management - Adds
IncrementalWriteTestdemonstrating both the new test base class and an experimental WriteBatcher template pattern
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| marklogic-client-api/src/test/java/com/marklogic/client/test/AbstractClientTest.java | New base class providing automatic test data cleanup via marklogic-junit5 integration |
| marklogic-client-api/src/test/java/com/marklogic/client/test/rows/AbstractOpticUpdateTest.java | Updated to extend AbstractClientTest and removed manual URI deletion code |
| marklogic-client-api/src/test/java/com/marklogic/client/test/datamovement/IncrementalWriteTest.java | New test demonstrating AbstractClientTest usage and experimental WriteBatcher template pattern |
| marklogic-client-api/src/main/java/com/marklogic/client/datamovement/DataMovementManager.java | Made Closeable with default close() implementation for try-with-resources support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void test() { | ||
| AtomicInteger writtenCount = new AtomicInteger(); | ||
|
|
||
| WriteBatcherTemplate template = new WriteBatcherTemplate(Common.newClient()); |
There was a problem hiding this comment.
The WriteBatcherTemplate instance is created but never explicitly closed, which could lead to resource leaks since it holds a DatabaseClient. Consider using try-with-resources or explicitly closing the client after use.
| @Override | ||
| protected final String getJavascriptForDeletingDocumentsBeforeTestRuns() { | ||
| return """ | ||
| declareUpdate(); |
There was a problem hiding this comment.
The excluded collections 'test-data' and 'temporal-collection' should be documented to explain why they are preserved during test cleanup, as this filtering logic is critical for test data management.
| declareUpdate(); | |
| declareUpdate(); | |
| // Preserve documents in the 'test-data' and 'temporal-collection' collections because they are | |
| // created as part of deploying the test application (including temporal configuration) and must | |
| // persist across test runs. |
This doesn't test incremental writes yet, but it will soon. Just want to get a "real" test in place that wipes data from the database before it starts. Also toying with a template approach for WriteBatcher, so made DataMovementManager Closeable which is a non-breaking change.
rjrudin
force-pushed
the
feature/26420-real-test
branch
from
a37b359 to
d457c72
Compare
This doesn't test incremental writes yet, but it will soon. Just want to get a "real" test in place that wipes data from the database before it starts. Also toying with a template approach for WriteBatcher, so made DataMovementManager Closeable which is a non-breaking change.