perf(database): Cache and reuse IDatabaseConnection across tests - #801
Merged
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (68)
📝 WalkthroughWalkthroughAdds opt-in liveness-validated connection caching, shared test-case lifecycle connections, suppressed failure handling, locale- and charset-stable processing, export and operation fixes, restored tests, and updated 3.4.0 release documentation. ChangesDbUnit 3.4.0 behavior and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeffjensen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java (1)
183-203: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid closing connections returned by
CachingConnectionProviderbefore lifecycle completion.
lookupFeatureValue()closesgetConnection()in itsfinally, andcleanupData()callscloseReusableConnection()in itsfinally. SinceCachingConnectionProvider.getConnection()returns the same cached connection untilclose()is invoked, these paths make an otherwise shared test-method connection unavailable for every closing user of that connection for the rest of the test. If this class may be used with a sharedCachingConnectionProvider, the per-call lookup should keep that connection open, andcleanupData()should only close a connection it is responsible for releasing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java` around lines 183 - 203, Update lookupFeatureValue to avoid closing the connection returned by getConnection(), allowing shared CachingConnectionProvider connections to remain available through the test lifecycle. In cleanupData, only invoke closeReusableConnection() when this class owns the connection release; preserve cleanup behavior for connections that are not shared.
🧹 Nitpick comments (1)
src/test/java/org/dbunit/database/CachingConnectionProviderTest.java (1)
260-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
.as()fail message.Unlike virtually every other assertion in this file, this one has no
.as(...)description.- assertThatThrownBy(() -> provider.getConnection(factory)).isInstanceOf(SQLException.class); + assertThatThrownBy(() -> provider.getConnection(factory)) + .as("The first attempt's factory failure must surface as-is.") + .isInstanceOf(SQLException.class);As per coding guidelines, "For tests, prefer adding
.as()with a fail message ending with a period."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/database/CachingConnectionProviderTest.java` around lines 260 - 276, Add an AssertJ `.as(...)` description ending with a period to the first `assertThatThrownBy` assertion in `testGetConnection_afterFactoryThrowsOnFirstCall_retriesOnNextCall`, clearly describing the expected SQLException from the failed initial factory call.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/changes/changes.xml`:
- Line 16: Shorten the description attribute of the 3.3.1-SNAPSHOT release entry
to a brief summary of the changes, removing the detailed test-by-test changelog;
retain those details in the existing action entries.
In `@src/main/java/org/dbunit/database/CachingConnectionProvider.java`:
- Around line 217-226: Update CachingConnectionProvider.toString() to
synchronize access to connection, matching the synchronization used by the
class’s other connection-accessing methods. Preserve the existing string format
and validationTimeoutSeconds output.
In `@src/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.java`:
- Around line 78-92: Complete the public JavaDoc in
PropertiesBasedJdbcDatabaseTester.java:78-92 by making the connectionProvider
`@param` description a capitalized, complete sentence ending with a period. In
InMemoryJndiContextFactory.java:55-85, update every `@param` and `@return`
description to use complete, capitalized sentences ending with periods. Add
JavaDoc to the public getInitialContext() method in
InMemoryJndiContextFactory.java:94-96, documenting its behavior and return
value.
In `@src/test/java/org/dbunit/database/InMemoryJndiContextFactory.java`:
- Around line 61-66: Update InMemoryJndiContextFactory around bind to add a
clear/unbind API that removes bindings and their corresponding lookup counters
from BINDINGS and LOOKUP_COUNTS. Invoke this cleanup API from the JNDI test
teardown so each test releases its bound H2 DataSource while preserving existing
binding and lookup behavior.
In `@src/test/java/org/dbunit/DataSourceDatabaseTesterTest.java`:
- Around line 36-42: Rename the test class and file DataSourceDatabaseTesterTest
to DataSourceDatabaseTesterIT, and rename JndiDatabaseTesterTest and its file to
JndiDatabaseTesterIT, preserving their existing test behavior so both database
integration tests are picked up by Failsafe.
---
Outside diff comments:
In `@src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java`:
- Around line 183-203: Update lookupFeatureValue to avoid closing the connection
returned by getConnection(), allowing shared CachingConnectionProvider
connections to remain available through the test lifecycle. In cleanupData, only
invoke closeReusableConnection() when this class owns the connection release;
preserve cleanup behavior for connections that are not shared.
---
Nitpick comments:
In `@src/test/java/org/dbunit/database/CachingConnectionProviderTest.java`:
- Around line 260-276: Add an AssertJ `.as(...)` description ending with a
period to the first `assertThatThrownBy` assertion in
`testGetConnection_afterFactoryThrowsOnFirstCall_retriesOnNextCall`, clearly
describing the expected SQLException from the failed initial factory call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61830764-41b8-424b-9376-34be33152781
📒 Files selected for processing (21)
src/changes/changes.xmlsrc/main/java/org/dbunit/DataSourceDatabaseTester.javasrc/main/java/org/dbunit/DefaultDatabaseTester.javasrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/JdbcDatabaseTester.javasrc/main/java/org/dbunit/JndiDatabaseTester.javasrc/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.javasrc/main/java/org/dbunit/database/CachingConnectionProvider.javasrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DataSourceDatabaseTesterTest.javasrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultDatabaseTesterTest.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/JdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/JndiDatabaseTesterTest.javasrc/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/database/CachingConnectionProviderIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderTest.javasrc/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
from
July 23, 2026 13:47
29471ee to
78353fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java`:
- Around line 227-273: Update closeReusableConnection() so it clears connection
only when closeConnectionAfterTest is true and the connection is actually
closed; when closing is skipped, retain the existing reference for subsequent
lifecycle steps and avoid reacquiring a fresh connection. Preserve the current
null guard and exception-safe cleanup behavior for the closing path.
In `@src/test/java/org/dbunit/database/CachingConnectionProviderIT.java`:
- Around line 54-66: Update the setUp and tearDown lifecycle methods so teardown
remains safe when setup fails before provider assignment. Initialize provider
before the fallible profile lookup and driver loading in setUp, or guard
provider.close() in tearDown against null while preserving normal cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b9b98d9-c8a9-46a1-a2d2-612eedbf42da
📒 Files selected for processing (21)
src/changes/changes.xmlsrc/main/java/org/dbunit/DataSourceDatabaseTester.javasrc/main/java/org/dbunit/DefaultDatabaseTester.javasrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/JdbcDatabaseTester.javasrc/main/java/org/dbunit/JndiDatabaseTester.javasrc/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.javasrc/main/java/org/dbunit/database/CachingConnectionProvider.javasrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DataSourceDatabaseTesterIT.javasrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultDatabaseTesterTest.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/JdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/JndiDatabaseTesterIT.javasrc/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/database/CachingConnectionProviderIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderTest.javasrc/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
🚧 Files skipped from review as they are similar to previous changes (14)
- src/test/java/org/dbunit/JdbcDatabaseTesterTest.java
- src/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.java
- src/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
- src/site/fml/faq.fml
- src/test/java/org/dbunit/DefaultDatabaseTesterTest.java
- src/main/java/org/dbunit/DefaultDatabaseTester.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.java
- src/main/java/org/dbunit/DataSourceDatabaseTester.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.java
- src/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.java
- src/main/java/org/dbunit/database/CachingConnectionProvider.java
- src/main/java/org/dbunit/JndiDatabaseTester.java
- src/changes/changes.xml
- src/test/java/org/dbunit/database/CachingConnectionProviderTest.java
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
from
July 23, 2026 15:44
78353fa to
d0702fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.java`:
- Around line 78-92: Update the `@since` tag on the new connectionProvider
constructor documentation in PropertiesBasedJdbcDatabaseTester from 3.3.1 to
3.4.0, matching the release that first introduces this overload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d3e71fe-20ef-43a8-a4f6-429738b9a589
📒 Files selected for processing (23)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/DataSourceDatabaseTester.javasrc/main/java/org/dbunit/DefaultDatabaseTester.javasrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/JdbcDatabaseTester.javasrc/main/java/org/dbunit/JndiDatabaseTester.javasrc/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.javasrc/main/java/org/dbunit/database/CachingConnectionProvider.javasrc/site/asciidoc/index.adocsrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DataSourceDatabaseTesterIT.javasrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultDatabaseTesterTest.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/JdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/JndiDatabaseTesterIT.javasrc/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/database/CachingConnectionProviderIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderTest.javasrc/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
🚧 Files skipped from review as they are similar to previous changes (18)
- src/test/java/org/dbunit/JdbcDatabaseTesterTest.java
- src/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.java
- src/test/java/org/dbunit/DataSourceDatabaseTesterIT.java
- src/main/java/org/dbunit/JdbcDatabaseTester.java
- src/changes/changes.xml
- src/main/java/org/dbunit/database/CachingConnectionProvider.java
- src/test/java/org/dbunit/JndiDatabaseTesterIT.java
- src/test/java/org/dbunit/DefaultDatabaseTesterTest.java
- src/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
- src/site/fml/faq.fml
- src/main/java/org/dbunit/JndiDatabaseTester.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.java
- src/test/java/org/dbunit/database/CachingConnectionProviderTest.java
- src/main/java/org/dbunit/DataSourceDatabaseTester.java
- src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java
- src/main/java/org/dbunit/DefaultDatabaseTester.java
- src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
2 times, most recently
from
July 23, 2026 17:26
443d1e7 to
44dd13f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java`:
- Around line 228-245: Update lookupFeatureValue() and cleanupData() so
closeReusableConnection() failures do not replace an exception from the primary
operation: capture the primary throwable, attempt cleanup separately, and attach
any cleanup failure with addSuppressed before rethrowing. Preserve normal return
behavior and ensure cleanup-only failures still propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 557cabf0-759d-4fb6-8c05-4ca415f48c4a
📒 Files selected for processing (10)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/site/asciidoc/index.adocsrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/database/MockDatabaseConnection.java
🚧 Files skipped from review as they are similar to previous changes (7)
- pom.xml
- src/site/asciidoc/index.adoc
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.java
- src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java
- src/site/fml/faq.fml
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java
DatabaseTestCase.tearDown() run after a test body has already failed can throw its own exception, which (via normal Java finally/catch semantics in the caller) replaces the original test failure with no trace of it. This was reported in 2004 (SourceForge bugs#80, migrated as GitHub #141) and closed invalid at the time because it was considered unfixable given JUnit's lifecycle control at the time. Add an overload, tearDown(Throwable testFailure), for callers that already have a test failure in flight: a tear-down failure is attached to it via Throwable.addSuppressed() instead of replacing it, and the original testFailure is always what propagates. The existing no-arg tearDown() is unchanged; the shared operation-execution logic is factored into a private runTearDownOperation() helper. DatabaseTestCase implements InvocationInterceptor but overrides none of its methods, so it does not actually run setUp()/tearDown() automatically around @test methods today; the real JUnit lifecycle wiring happens in subclasses like AbstractDatabaseIT via their own @BeforeEach/@AfterEach overrides. This fix targets the manual setUp()/tearDown() invocation path (as already used by DatabaseTestCaseIT's other test and by DefaultPrepAndExpectedTestCase) and leaves that unrelated, higher-risk interceptor question alone. Refs: 141 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R58qYD5rnoCWoyXAZe6Xbq
testWithBadQuerySet_withBothIdAndRefid_throwsBuildException asserted that a queryset with both id and refid attributes throws a BuildException. QuerySet.setId()/setRefid() still contain that validation, but Ant's attribute-binding no longer routes the id attribute through the task's setId() (id is a framework-reserved attribute consumed for project reference registration), so the refid-vs-id conflict check in setRefid() never sees a non-null id and never fires; the target now runs to completion. Rewrite the test to assert today's actual behavior (resolves the refid reference, does not throw) and rename to match, verified by running it against hsqldb. Refs: 797 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R58qYD5rnoCWoyXAZe6Xbq
InsertOperationIT.testExecuteNullAsNone referenced DatabaseConfig.FEATURE_NULL_AS_NONE, which does not exist anywhere in production code under any name - an apparently never-implemented feature, confirmed by a full-codebase search. The NO_VALUE-omission behavior it targeted is already covered by InsertOperationTest.testExecute_withNoValueFields_omitsNoValueColumnsFromInsertSql. Implementing the feature to make the test meaningful would be new production functionality out of scope for restoring existing test coverage. DatabaseDataSetIT.testGetTableNamesAndCaseSensitive was two lines cut off mid-statement with no coherent assertion, probing raw DatabaseMetaData introspection that dbunit's own case-sensitivity handling (FEATURE_CASE_SENSITIVE_TABLE_NAMES, config-flag-driven) does not use. Nothing restorable there either; that behavior is already exercised throughout this same file via the convertString() helper. Refs: 797 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R58qYD5rnoCWoyXAZe6Xbq
xtestOidDataType, xtestUUidDataType, and xtestDomainDataTypes kept the stale JUnit3/4 x-prefix disable convention, which has no effect under JUnit Jupiter (the methods still ran, still carrying @test). Each body was wrapped in try { ... } catch (Exception e) { assertEquals("...no exception", ""+e); }: AssertionError is not an Exception, so the real AssertJ assertions inside the try already propagated normally on failure, but any other exception (e.g. a JDBC or type-conversion failure from the CLEAN_INSERT under test) was caught and replaced with a fixed nonsense comparison, discarding the real exception, type, and stack trace. Rename all three to the test* convention and delete the try/catch, letting the underlying operation's own failures propagate normally. Verified against a Dockerized postgres:16.3 via ./mvnw clean verify -Ppostgresql-16. Refs: 797 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R58qYD5rnoCWoyXAZe6Xbq
Update the release-level description to summarize all ten changes in this un-skip/strengthen-tests effort, matching the running-summary convention used by prior releases. Refs: 797 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R58qYD5rnoCWoyXAZe6Xbq
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
2 times, most recently
from
July 24, 2026 03:51
f048c54 to
944afb2
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/dbunit/database/CachingConnectionProviderIT.java (1)
118-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an AssertJ description to the connection-identity assertion.
Line 120 lacks an
.as()message, reducing failure diagnostics.Suggested change
- assertThat(second).isNotSameAs(first); + assertThat(second) + .as("Closing the provider must create a new connection.") + .isNotSameAs(first);As per coding guidelines, tests should prefer
.as()with a fail message ending with a period.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/database/CachingConnectionProviderIT.java` around lines 118 - 120, Add an AssertJ `.as()` description to the `assertThat(second).isNotSameAs(first)` assertion in the connection provider test, using a clear failure message that ends with a period.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/java/org/dbunit/database/CachingConnectionProviderIT.java`:
- Around line 118-120: Add an AssertJ `.as()` description to the
`assertThat(second).isNotSameAs(first)` assertion in the connection provider
test, using a clear failure message that ends with a period.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5162b9f6-03d3-482e-b744-a32abdc4e31c
📒 Files selected for processing (32)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/DataSourceDatabaseTester.javasrc/main/java/org/dbunit/DatabaseTestCase.javasrc/main/java/org/dbunit/DefaultDatabaseTester.javasrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/JdbcDatabaseTester.javasrc/main/java/org/dbunit/JndiDatabaseTester.javasrc/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.javasrc/main/java/org/dbunit/database/CachingConnectionProvider.javasrc/site/asciidoc/index.adocsrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DataSourceDatabaseTesterIT.javasrc/test/java/org/dbunit/DatabaseTestCaseIT.javasrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultDatabaseTesterTest.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/JdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/JndiDatabaseTesterIT.javasrc/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/ant/DbUnitTaskIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderTest.javasrc/test/java/org/dbunit/database/DatabaseDataSetIT.javasrc/test/java/org/dbunit/database/InMemoryJndiContextFactory.javasrc/test/java/org/dbunit/database/MockDatabaseConnection.javasrc/test/java/org/dbunit/ext/postgresql/PostgresSQLOidIT.javasrc/test/java/org/dbunit/ext/postgresql/PostgresqlUuidIT.javasrc/test/java/org/dbunit/ext/postgresql/SQLHelperDomainPostgreSQLIT.javasrc/test/java/org/dbunit/operation/InsertOperationIT.java
💤 Files with no reviewable changes (2)
- src/test/java/org/dbunit/operation/InsertOperationIT.java
- src/test/java/org/dbunit/database/DatabaseDataSetIT.java
🚧 Files skipped from review as they are similar to previous changes (21)
- pom.xml
- src/site/asciidoc/index.adoc
- src/test/java/org/dbunit/DefaultDatabaseTesterTest.java
- src/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.java
- src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java
- src/test/java/org/dbunit/JndiDatabaseTesterIT.java
- src/site/fml/faq.fml
- src/main/java/org/dbunit/database/CachingConnectionProvider.java
- src/test/java/org/dbunit/DataSourceDatabaseTesterIT.java
- src/main/java/org/dbunit/JdbcDatabaseTester.java
- src/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
- src/test/java/org/dbunit/database/CachingConnectionProviderTest.java
- src/main/java/org/dbunit/DefaultDatabaseTester.java
- src/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.java
- src/changes/changes.xml
- src/test/java/org/dbunit/JdbcDatabaseTesterTest.java
- src/main/java/org/dbunit/JndiDatabaseTester.java
- src/main/java/org/dbunit/DataSourceDatabaseTester.java
- src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java
Member
Author
|
Addressed the assertThat(second)
.as("Closing the provider must create a new connection.")
.isNotSameAs(first);This is in a Addressed via Claude Code. |
…on reuse #461 asked for a DatabaseConnectionProvider to pool/cache connections across test methods, to avoid dbUnit re-fetching table metadata on every test. That was closed as not-planned: physical JDBC connection pooling is already solved by pointing DataSourceDatabaseTester at an external pool. But investigating it surfaced a narrower, real gap: dbUnit's own metadata cache (DatabaseDataSet, scoped to one IDatabaseConnection instance) is thrown away far more often than necessary, independent of physical pooling - every concrete IDatabaseTester builds a brand new IDatabaseConnection on every getConnection() call. Add CachingConnectionProvider, caching a single IDatabaseConnection (and therefore its metadata cache) across calls. A Callable supplies the connection-creation logic; it is only invoked on the first call and again whenever Connection#isValid() (or isClosed()) shows the previously cached connection is no longer alive, so a transient DB blip gets a transparent reconnect instead of silently reusing - or permanently failing on - a dead connection. Access is synchronized so concurrent callers cannot race each other into creating more than one connection, though the returned connection itself is not thread-safe for concurrent use. Wire it into JdbcDatabaseTester, DataSourceDatabaseTester, JndiDatabaseTester, PropertiesBasedJdbcDatabaseTester, and DefaultDatabaseTester via new, additive constructor overloads. Existing constructors are unchanged and remain the default (always-fresh) behavior; the new capability is entirely opt-in: share one CachingConnectionProvider instance across the tester instances built for each test method (e.g. a static field on a common test base class) to get reuse across a run, and pair it with IOperationListener#NO_OP_OPERATION_LISTENER (or an equivalent non-closing listener) - otherwise the default listener closes the connection after every onSetup()/onTearDown() call and the cache never has anything alive left to reuse. This makes the existing manual DefaultDatabaseTester + NO_OP_OPERATION_LISTENER reuse pattern - previously a dumb holder with no liveness check, documented only in the FAQ - safe to opt into without hand-wiring a static connection field. DatabaseTestCase/DBTestCase subclasses that only plug in an IDatabaseTester add no connection lifecycle logic of their own, so they benefit automatically by overriding newDatabaseTester() to pass a shared provider through. Update the FAQ's performance and keep-connection-open entries accordingly. Verified with new unit tests (real H2 in-memory connections, Mockito for the liveness-check edge cases, and a JDK-only InitialContextFactory test double for JndiDatabaseTester, since the project has no JNDI test dependency) and new integration tests against hsqldb, h2, derby, and postgresql (via Docker). Refs: 799 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2ESpWxqWKhHGRWPzjHsAm
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
from
July 24, 2026 04:39
944afb2 to
5f0d150
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/dataset/LowerCaseTableMetaData.java`:
- Line 95: Update the column-name expectations in LowerCaseTableMetaDataTest to
call toLowerCase with Locale.ENGLISH, matching the locale used by
LowerCaseTableMetaData and preserving stable results under Turkish locale
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e652bb40-8387-49d8-a7f7-c799fc40202f
📒 Files selected for processing (38)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/DataSourceDatabaseTester.javasrc/main/java/org/dbunit/DefaultDatabaseTester.javasrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/JdbcDatabaseTester.javasrc/main/java/org/dbunit/JndiDatabaseTester.javasrc/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.javasrc/main/java/org/dbunit/assertion/DbUnitAssertBase.javasrc/main/java/org/dbunit/database/CachingConnectionProvider.javasrc/main/java/org/dbunit/database/search/TablesDependencyHelper.javasrc/main/java/org/dbunit/dataset/AbstractTableMetaData.javasrc/main/java/org/dbunit/dataset/CaseInsensitiveDataSet.javasrc/main/java/org/dbunit/dataset/LowerCaseDataSet.javasrc/main/java/org/dbunit/dataset/LowerCaseTableMetaData.javasrc/main/java/org/dbunit/dataset/datatype/BytesDataType.javasrc/main/java/org/dbunit/dataset/filter/PatternMatcher.javasrc/main/java/org/dbunit/ext/oracle/OracleConnection.javasrc/main/java/org/dbunit/ext/oracle/OracleSdoGeometryDataType.javasrc/main/java/org/dbunit/operation/TruncateTableOperation.javasrc/site/asciidoc/index.adocsrc/site/fml/faq.fmlsrc/test/java/org/dbunit/DataSourceDatabaseTesterIT.javasrc/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.javasrc/test/java/org/dbunit/DefaultDatabaseTesterTest.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseDiIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/JdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/JndiDatabaseTesterIT.javasrc/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.javasrc/test/java/org/dbunit/assertion/DbUnitAssertBaseTest.javasrc/test/java/org/dbunit/database/CachingConnectionProviderIT.javasrc/test/java/org/dbunit/database/CachingConnectionProviderTest.javasrc/test/java/org/dbunit/database/InMemoryJndiContextFactory.javasrc/test/java/org/dbunit/database/MockDatabaseConnection.javasrc/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.javasrc/test/java/org/dbunit/dataset/filter/PatternMatcherTest.java
🚧 Files skipped from review as they are similar to previous changes (22)
- src/site/asciidoc/index.adoc
- src/test/java/org/dbunit/DefaultDatabaseTesterTest.java
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseExtIT.java
- pom.xml
- src/main/java/org/dbunit/PropertiesBasedJdbcDatabaseTester.java
- src/test/java/org/dbunit/JdbcDatabaseTesterTest.java
- src/site/fml/faq.fml
- src/test/java/org/dbunit/database/MockDatabaseConnection.java
- src/main/java/org/dbunit/DefaultDatabaseTester.java
- src/test/java/org/dbunit/database/InMemoryJndiContextFactory.java
- src/test/java/org/dbunit/database/CachingConnectionProviderTest.java
- src/main/java/org/dbunit/database/CachingConnectionProvider.java
- src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java
- src/test/java/org/dbunit/PropertiesBasedJdbcDatabaseTesterTest.java
- src/test/java/org/dbunit/DataSourceDatabaseTesterIT.java
- src/test/java/org/dbunit/JndiDatabaseTesterIT.java
- src/main/java/org/dbunit/JdbcDatabaseTester.java
- src/main/java/org/dbunit/DataSourceDatabaseTester.java
- src/main/java/org/dbunit/JndiDatabaseTester.java
- src/changes/changes.xml
- src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java
- src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/dbunit/util/SQLHelper.java (1)
605-609: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the updated JavaDoc.
The topic and parameter descriptions are not complete sentences with terminal periods.
As per coding guidelines, JavaDoc topic, parameter, and return descriptions must be complete sentences that start with a capital letter and end with a period.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/dbunit/util/SQLHelper.java` around lines 605 - 609, Complete the JavaDoc for the unequal-value logging method by capitalizing the topic and each parameter description and adding terminal periods to every description. Update only the documentation associated with the method and preserve its existing meaning.Source: Coding guidelines
🧹 Nitpick comments (2)
src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java (1)
172-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd descriptive AssertJ failure messages.
src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java#L172-L173: Replace"output"with a specific sentence ending in a period.src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java#L195-L196: Describe the expected<none/>omission in a sentence ending in a period.src/test/java/org/dbunit/dataset/yaml/YmlWriterTest.java#L154-L155: Describe the expected YAML key omission in a sentence ending in a period.src/test/java/org/dbunit/util/QualifiedTableNameTest.java#L136-L136: Add an.as()message ending in a period.src/test/java/org/dbunit/util/QualifiedTableNameTest.java#L145-L145: Add an.as()message ending in a period.src/test/java/org/dbunit/util/QualifiedTableNameTest.java#L154-L154: Add an.as()message ending in a period.src/test/java/org/dbunit/util/QualifiedTableNameTest.java#L167-L167: Add an.as()message ending in a period.src/test/java/org/dbunit/util/SQLHelperTest.java#L216-L216: Use a sentence ending in a period.src/test/java/org/dbunit/util/SQLHelperTest.java#L241-L242: Use a sentence ending in a period.As per coding guidelines, tests should prefer adding
.as()with a fail message ending with a period.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java` around lines 172 - 173, Update AssertJ assertions in FlatXmlWriterTest.java lines 172-173 and 195-196, YmlWriterTest.java lines 154-155, QualifiedTableNameTest.java lines 136, 145, 154, and 167, and SQLHelperTest.java lines 216 and 241-242 to use descriptive failure messages instead of generic or missing messages; describe the expected output, <none/> omission, YAML key omission, or qualified-name/SQL behavior as applicable, and ensure every message is a complete sentence ending with a period.Source: Coding guidelines
src/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.java (1)
271-299: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize the default-locale mutation.
Locale.setDefault(...)changes JVM-global state. If test parallelism is enabled, unrelated tests can observetr-TRbetween the mutation and restoration, causing nondeterministic failures. Serialize this test/class through the test framework or disable parallel execution for it, and verify the project’s test configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.java` around lines 271 - 299, Serialize the locale-sensitive test using the project’s test-framework mechanism, or disable parallel execution for its containing class, so the Locale.setDefault mutation in the relevant BytesDataTypeTest test cannot overlap with other tests. Verify the project’s existing test configuration and apply the narrowest supported setting while preserving the test’s restoration logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/dataset/datatype/BytesDataType.java`:
- Around line 68-80: Update the JavaDoc for the stream-reading method so the
`@param`, `@return`, and `@throws` descriptions begin with capital letters while
retaining their existing terminating periods; leave the documented behavior and
ownership details unchanged.
In `@src/main/java/org/dbunit/util/SQLHelper.java`:
- Line 617: Update the surrounding change-detection guard in SQLHelper to use
!Objects.equals(oldValue, newValue), so null-to-value and value-to-null changes
reach the existing logger.info call. Add a regression test covering oldValue
null with a non-null newValue while preserving the current logging behavior for
equal values.
In `@src/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.java`:
- Line 223: Update the AssertJ description in the test assertion using
.as("typecast") so the failure message ends with a period, while preserving the
existing assertion behavior.
---
Outside diff comments:
In `@src/main/java/org/dbunit/util/SQLHelper.java`:
- Around line 605-609: Complete the JavaDoc for the unequal-value logging method
by capitalizing the topic and each parameter description and adding terminal
periods to every description. Update only the documentation associated with the
method and preserve its existing meaning.
---
Nitpick comments:
In `@src/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.java`:
- Around line 271-299: Serialize the locale-sensitive test using the project’s
test-framework mechanism, or disable parallel execution for its containing
class, so the Locale.setDefault mutation in the relevant BytesDataTypeTest test
cannot overlap with other tests. Verify the project’s existing test
configuration and apply the narrowest supported setting while preserving the
test’s restoration logic.
In `@src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java`:
- Around line 172-173: Update AssertJ assertions in FlatXmlWriterTest.java lines
172-173 and 195-196, YmlWriterTest.java lines 154-155,
QualifiedTableNameTest.java lines 136, 145, 154, and 167, and SQLHelperTest.java
lines 216 and 241-242 to use descriptive failure messages instead of generic or
missing messages; describe the expected output, <none/> omission, YAML key
omission, or qualified-name/SQL behavior as applicable, and ensure every message
is a complete sentence ending with a period.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec18cdae-9953-4bd2-991b-5ae292a1f369
📒 Files selected for processing (13)
src/changes/changes.xmlsrc/main/java/org/dbunit/ant/Export.javasrc/main/java/org/dbunit/dataset/datatype/BytesDataType.javasrc/main/java/org/dbunit/dataset/xml/FlatXmlWriter.javasrc/main/java/org/dbunit/dataset/yaml/YamlWriter.javasrc/main/java/org/dbunit/util/QualifiedTableName.javasrc/main/java/org/dbunit/util/SQLHelper.javasrc/test/java/org/dbunit/ant/ExportTest.javasrc/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.javasrc/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.javasrc/test/java/org/dbunit/dataset/yaml/YmlWriterTest.javasrc/test/java/org/dbunit/util/QualifiedTableNameTest.javasrc/test/java/org/dbunit/util/SQLHelperTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/changes/changes.xml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/dataset/xml/FlatDtdDataSet.java`:
- Around line 89-94: Update the JavaDoc topic sentence for the dataset-writing
method in src/main/java/org/dbunit/dataset/xml/FlatDtdDataSet.java lines 89-94
from “Write...” to “Writes...” and ensure it remains a complete, capitalized
sentence ending with a period. Apply the same wording correction in
src/main/java/org/dbunit/dataset/yaml/YamlDataSet.java lines 93-96.
In `@src/test/java/org/dbunit/database/DatabaseDataSetTest.java`:
- Around line 89-90: Update the AssertJ description in the assertion using
metaData.getTableName() so the .as() message ends with a period, preserving the
existing assertion and expected value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d6b8c35-ec32-40d0-bae5-dd4a8fee4704
📒 Files selected for processing (22)
pom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/database/DatabaseDataSet.javasrc/main/java/org/dbunit/dataset/datatype/AbstractDataType.javasrc/main/java/org/dbunit/dataset/datatype/BigIntegerDataType.javasrc/main/java/org/dbunit/dataset/datatype/BooleanDataType.javasrc/main/java/org/dbunit/dataset/datatype/BytesDataType.javasrc/main/java/org/dbunit/dataset/datatype/DateDataType.javasrc/main/java/org/dbunit/dataset/datatype/DoubleDataType.javasrc/main/java/org/dbunit/dataset/datatype/FloatDataType.javasrc/main/java/org/dbunit/dataset/datatype/IntegerDataType.javasrc/main/java/org/dbunit/dataset/datatype/LongDataType.javasrc/main/java/org/dbunit/dataset/datatype/NumberDataType.javasrc/main/java/org/dbunit/dataset/datatype/StringDataType.javasrc/main/java/org/dbunit/dataset/datatype/TimeDataType.javasrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/main/java/org/dbunit/dataset/xml/FlatDtdDataSet.javasrc/main/java/org/dbunit/dataset/yaml/YamlDataSet.javasrc/main/java/org/dbunit/ext/oracle/OracleXMLTypeDataType.javasrc/test/java/org/dbunit/database/DatabaseDataSetTest.javasrc/test/java/org/dbunit/dataset/xml/FlatDtdDataSetIT.javasrc/test/java/org/dbunit/dataset/yaml/YmlDataSetTest.java
💤 Files with no reviewable changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java
- src/changes/changes.xml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/dbunit/util/FileHelper.java (1)
116-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the public method’s parameter JavaDoc.
Use capitalized, complete descriptions ending with periods for
srcFileanddestFile. As per coding guidelines, “Write JavaDoc comments for all public Java classes and methods; use complete sentences with capitalization and periods for topic text, parameters, and return descriptions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/dbunit/util/FileHelper.java` at line 116, Update the JavaDoc for the public copyFile(File srcFile, File destFile) method to add complete, capitalized parameter descriptions for srcFile and destFile, with each description ending in a period.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/org/dbunit/util/FileHelper.java`:
- Line 116: Update the JavaDoc for the public copyFile(File srcFile, File
destFile) method to add complete, capitalized parameter descriptions for srcFile
and destFile, with each description ending in a period.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba9a1069-ad60-456f-9f67-efa4c99194e9
📒 Files selected for processing (8)
src/changes/changes.xmlsrc/main/java/org/dbunit/database/DatabaseDataSet.javasrc/main/java/org/dbunit/dataset/csv/CsvParserImpl.javasrc/main/java/org/dbunit/dataset/stream/StreamingIterator.javasrc/main/java/org/dbunit/util/FileHelper.javasrc/test/java/org/dbunit/dataset/csv/CsvParserTest.javasrc/test/java/org/dbunit/dataset/stream/StreamingIteratorTest.javasrc/test/java/org/dbunit/util/FileHelperTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/dbunit/database/DatabaseDataSet.java
- src/changes/changes.xml
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
from
July 25, 2026 13:16
b35127d to
8f7292c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java (1)
406-441: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest name doesn't match what's exercised.
testRunTest_withNonDefaultTearDown_reusesOneConnectionAcrossLifecyclenever callsrunTest(); it drivesconfigureTest(),preTest(), andpostTest()individually (lines 423, 429-430). Per the naming convention,<MethodName>should reflect the method(s) actually under test.✏️ Suggested rename
- void testRunTest_withNonDefaultTearDown_reusesOneConnectionAcrossLifecycle() + void testConfigureTestPreTestPostTest_withNonDefaultTearDown_reusesOneConnectionAcrossLifecycle()As per path instructions, "Name test methods
test<MethodName>_<StartingStateConditions>_<AssertedOutcome>."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java` around lines 406 - 441, Rename testRunTest_withNonDefaultTearDown_reusesOneConnectionAcrossLifecycle to reflect the lifecycle methods it actually exercises—configureTest, preTest, and postTest—while preserving the existing starting-condition and asserted-outcome portions of the name.Source: Path instructions
🧹 Nitpick comments (1)
src/test/java/org/dbunit/dataset/LowerCaseTableMetaDataTest.java (1)
25-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the locale-sensitive behavior explicitly.
These expected-value changes are locale-stable, but the tests never change the default locale. A regression to
toLowerCase()could still pass in an English CI environment. Add a Turkish-locale case using a name containingI(such asSTRING_COLUMN), with safe restoration/serialization of the default locale.As per coding guidelines, changed behavior should be covered by unit tests.
Also applies to: 42-42, 72-72, 109-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/dataset/LowerCaseTableMetaDataTest.java` around lines 25 - 26, Add a unit-test case in LowerCaseTableMetaDataTest that temporarily sets the default locale to Turkish and verifies lowercasing a name containing uppercase I, such as STRING_COLUMN, produces the locale-stable expected result. Safely restore the original default locale and serialize or otherwise isolate the locale mutation, covering the affected test paths without changing production behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/operation/RefreshOperation.java`:
- Around line 134-143: Update the cleanup block in RefreshOperation’s execution
flow so failures from updateRowOperation.close() or the refresh operation remain
the primary exception; ensure insertRowOperation.close() still runs, but attach
any later close failure as a suppressed exception rather than allowing it to
replace the original cause, using try-with-resources or equivalent explicit
handling.
In `@src/test/java/org/dbunit/operation/InsertOperationIT.java`:
- Around line 360-369: Update the AssertJ description strings in the affected
assertions within InsertOperationIT so each .as() message ends with a period,
including the multiline row 2 STATUS description; preserve the assertion logic
and wording otherwise.
---
Outside diff comments:
In `@src/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.java`:
- Around line 406-441: Rename
testRunTest_withNonDefaultTearDown_reusesOneConnectionAcrossLifecycle to reflect
the lifecycle methods it actually exercises—configureTest, preTest, and
postTest—while preserving the existing starting-condition and asserted-outcome
portions of the name.
---
Nitpick comments:
In `@src/test/java/org/dbunit/dataset/LowerCaseTableMetaDataTest.java`:
- Around line 25-26: Add a unit-test case in LowerCaseTableMetaDataTest that
temporarily sets the default locale to Turkish and verifies lowercasing a name
containing uppercase I, such as STRING_COLUMN, produces the locale-stable
expected result. Safely restore the original default locale and serialize or
otherwise isolate the locale mutation, covering the affected test paths without
changing production behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95ad73f3-787c-4ff6-9a79-65751752433f
📒 Files selected for processing (73)
CLAUDE.mdpom.xmlsrc/changes/changes.xmlsrc/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.javasrc/main/java/org/dbunit/ant/Export.javasrc/main/java/org/dbunit/assertion/DbUnitAssertBase.javasrc/main/java/org/dbunit/database/DatabaseDataSet.javasrc/main/java/org/dbunit/database/search/TablesDependencyHelper.javasrc/main/java/org/dbunit/dataset/AbstractTableMetaData.javasrc/main/java/org/dbunit/dataset/CaseInsensitiveDataSet.javasrc/main/java/org/dbunit/dataset/LowerCaseDataSet.javasrc/main/java/org/dbunit/dataset/LowerCaseTableMetaData.javasrc/main/java/org/dbunit/dataset/csv/CsvParserImpl.javasrc/main/java/org/dbunit/dataset/csv/CsvProducer.javasrc/main/java/org/dbunit/dataset/datatype/AbstractDataType.javasrc/main/java/org/dbunit/dataset/datatype/BigIntegerDataType.javasrc/main/java/org/dbunit/dataset/datatype/BooleanDataType.javasrc/main/java/org/dbunit/dataset/datatype/BytesDataType.javasrc/main/java/org/dbunit/dataset/datatype/DateDataType.javasrc/main/java/org/dbunit/dataset/datatype/DoubleDataType.javasrc/main/java/org/dbunit/dataset/datatype/FloatDataType.javasrc/main/java/org/dbunit/dataset/datatype/IntegerDataType.javasrc/main/java/org/dbunit/dataset/datatype/LongDataType.javasrc/main/java/org/dbunit/dataset/datatype/NumberDataType.javasrc/main/java/org/dbunit/dataset/datatype/StringDataType.javasrc/main/java/org/dbunit/dataset/datatype/TimeDataType.javasrc/main/java/org/dbunit/dataset/datatype/TimestampDataType.javasrc/main/java/org/dbunit/dataset/excel/XlsTable.javasrc/main/java/org/dbunit/dataset/filter/PatternMatcher.javasrc/main/java/org/dbunit/dataset/stream/StreamingIterator.javasrc/main/java/org/dbunit/dataset/xml/FlatDtdDataSet.javasrc/main/java/org/dbunit/dataset/xml/FlatXmlWriter.javasrc/main/java/org/dbunit/dataset/yaml/YamlDataSet.javasrc/main/java/org/dbunit/dataset/yaml/YamlWriter.javasrc/main/java/org/dbunit/ext/oracle/OracleConnection.javasrc/main/java/org/dbunit/ext/oracle/OracleSdoGeometryDataType.javasrc/main/java/org/dbunit/ext/oracle/OracleXMLTypeDataType.javasrc/main/java/org/dbunit/operation/InsertOperation.javasrc/main/java/org/dbunit/operation/RefreshOperation.javasrc/main/java/org/dbunit/operation/TransactionOperation.javasrc/main/java/org/dbunit/operation/TruncateTableOperation.javasrc/main/java/org/dbunit/util/Base64.javasrc/main/java/org/dbunit/util/FileHelper.javasrc/main/java/org/dbunit/util/QualifiedTableName.javasrc/main/java/org/dbunit/util/SQLHelper.javasrc/main/java/org/dbunit/util/xml/XmlWriter.javasrc/test/java/org/dbunit/DefaultPrepAndExpectedTestCaseTest.javasrc/test/java/org/dbunit/ant/ExportTest.javasrc/test/java/org/dbunit/assertion/DbUnitAssertBaseTest.javasrc/test/java/org/dbunit/database/DatabaseDataSetTest.javasrc/test/java/org/dbunit/dataset/AbstractDataSetTest.javasrc/test/java/org/dbunit/dataset/LowerCaseTableMetaDataTest.javasrc/test/java/org/dbunit/dataset/csv/CsvParserTest.javasrc/test/java/org/dbunit/dataset/csv/CsvProducerTest.javasrc/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.javasrc/test/java/org/dbunit/dataset/datatype/TimestampDataTypeTest.javasrc/test/java/org/dbunit/dataset/excel/XlsTableTest.javasrc/test/java/org/dbunit/dataset/filter/PatternMatcherTest.javasrc/test/java/org/dbunit/dataset/stream/StreamingIteratorTest.javasrc/test/java/org/dbunit/dataset/xml/FlatDtdDataSetIT.javasrc/test/java/org/dbunit/dataset/xml/FlatXmlDataSetTest.javasrc/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.javasrc/test/java/org/dbunit/dataset/yaml/YmlDataSetTest.javasrc/test/java/org/dbunit/dataset/yaml/YmlWriterTest.javasrc/test/java/org/dbunit/operation/InsertOperationIT.javasrc/test/java/org/dbunit/operation/InsertOperationTest.javasrc/test/java/org/dbunit/operation/TransactionOperationTest.javasrc/test/java/org/dbunit/util/Base64Test.javasrc/test/java/org/dbunit/util/FileHelperTest.javasrc/test/java/org/dbunit/util/QualifiedTableNameTest.javasrc/test/java/org/dbunit/util/SQLHelperTest.javasrc/test/java/org/dbunit/util/xml/XmlWriterTest.javasrc/test/resources/sql/hypersonic.sql
💤 Files with no reviewable changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (51)
- src/main/java/org/dbunit/dataset/yaml/YamlWriter.java
- src/main/java/org/dbunit/operation/TransactionOperation.java
- src/main/java/org/dbunit/assertion/DbUnitAssertBase.java
- CLAUDE.md
- src/main/java/org/dbunit/dataset/xml/FlatXmlWriter.java
- src/test/java/org/dbunit/dataset/yaml/YmlDataSetTest.java
- src/main/java/org/dbunit/ext/oracle/OracleConnection.java
- src/test/java/org/dbunit/assertion/DbUnitAssertBaseTest.java
- src/main/java/org/dbunit/database/DatabaseDataSet.java
- src/test/java/org/dbunit/util/SQLHelperTest.java
- src/test/java/org/dbunit/dataset/AbstractDataSetTest.java
- src/test/java/org/dbunit/dataset/filter/PatternMatcherTest.java
- src/main/java/org/dbunit/dataset/csv/CsvParserImpl.java
- src/main/java/org/dbunit/dataset/LowerCaseDataSet.java
- src/main/java/org/dbunit/dataset/xml/FlatDtdDataSet.java
- src/test/resources/sql/hypersonic.sql
- src/test/java/org/dbunit/dataset/xml/FlatXmlDataSetTest.java
- src/main/java/org/dbunit/dataset/datatype/BigIntegerDataType.java
- src/test/java/org/dbunit/util/QualifiedTableNameTest.java
- src/main/java/org/dbunit/util/QualifiedTableName.java
- src/main/java/org/dbunit/dataset/datatype/DateDataType.java
- src/main/java/org/dbunit/dataset/CaseInsensitiveDataSet.java
- src/main/java/org/dbunit/dataset/datatype/AbstractDataType.java
- src/main/java/org/dbunit/dataset/excel/XlsTable.java
- src/main/java/org/dbunit/dataset/filter/PatternMatcher.java
- src/main/java/org/dbunit/operation/TruncateTableOperation.java
- src/main/java/org/dbunit/ant/Export.java
- src/main/java/org/dbunit/operation/InsertOperation.java
- src/test/java/org/dbunit/dataset/xml/FlatDtdDataSetIT.java
- src/main/java/org/dbunit/util/xml/XmlWriter.java
- src/main/java/org/dbunit/dataset/datatype/FloatDataType.java
- src/test/java/org/dbunit/operation/TransactionOperationTest.java
- src/main/java/org/dbunit/dataset/LowerCaseTableMetaData.java
- src/main/java/org/dbunit/util/SQLHelper.java
- src/main/java/org/dbunit/dataset/datatype/StringDataType.java
- src/test/java/org/dbunit/operation/InsertOperationTest.java
- src/main/java/org/dbunit/dataset/datatype/NumberDataType.java
- src/test/java/org/dbunit/dataset/xml/FlatXmlWriterTest.java
- src/test/java/org/dbunit/util/xml/XmlWriterTest.java
- src/main/java/org/dbunit/dataset/datatype/TimestampDataType.java
- src/test/java/org/dbunit/dataset/stream/StreamingIteratorTest.java
- src/test/java/org/dbunit/dataset/yaml/YmlWriterTest.java
- src/test/java/org/dbunit/dataset/excel/XlsTableTest.java
- src/test/java/org/dbunit/dataset/csv/CsvParserTest.java
- src/main/java/org/dbunit/ext/oracle/OracleSdoGeometryDataType.java
- src/main/java/org/dbunit/ext/oracle/OracleXMLTypeDataType.java
- src/main/java/org/dbunit/dataset/datatype/TimeDataType.java
- src/changes/changes.xml
- src/test/java/org/dbunit/dataset/datatype/BytesDataTypeTest.java
- src/main/java/org/dbunit/dataset/datatype/BytesDataType.java
- src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java
…acters Two ways XmlWriter.escapeXml() produced an export that no XML parser could re-read: (1) Control characters 0x00-0x08, 0x0B, 0x0C, 0x0E-0x1F were written raw - convertCharacterToEntity() only intercepted characters > 0x7f in its default branch, and raw control characters are not permitted in XML 1.0 at all. (2) escapeXml() iterated char-by-char, so a supplementary character (emoji, CJK-extension - valid XML, range 0x10000-0x10FFFF) was seen as two surrogate halves, each individually failing isValidXmlChar(), and was emitted as two numeric entities referencing forbidden surrogate code points - also a hard parse error on re-import. Rework escapeXml() to iterate by code point (String.codePointAt() / Character.charCount()) and check isValidXmlChar() first: a valid supplementary code point passes through raw (no markup character is outside the BMP); a valid BMP code point still routes through convertCharacterToEntity() unchanged, so existing subclass overrides keep taking effect; a code point that XML 1.0 cannot represent at all (control chars other than tab/LF/CR, 0xFFFE/0xFFFF, unpaired surrogates - isValidXmlChar() already correctly rejects all of these, it just was not being consulted for chars <= 0x7f or for supplementary code points) is substituted with U+FFFD and logged once per escapeXml() call, naming the replaced code points - never emitting them raw or as numeric entities, both of which are unparseable. convertCharacterToEntity() itself is untouched (signature and body), since escapeXml() now filters out-of-range code points before ever calling it. BEHAVIOR CHANGE: exports containing XML-unrepresentable control characters change from broken output to lossy-but-valid output with a warning; exports containing supplementary characters change from broken to correct. Refs: 811 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
…e math The zone-offset block encoded the timestamp as BigInteger.valueOf(tsTime / 1000 * 1000) - integer division truncates toward zero, but Timestamp.getTime() for a pre-epoch fractional-second instant is floor(seconds)*1000 + nanos/1e6, so for negative times the truncation landed on the wrong whole-second boundary and the encoded value was off by a full second (the nanos, always positive, were then double-counted). Separately, if the offset subtraction drove the encoded value negative, divideAndRemainder(ONE_BILLION) yielded a negative remainder and ts.setNanos(negative) threw IllegalArgumentException. Replace the truncating division with Math.floorDiv(tsTime, 1000L) * 1000L. After divideAndRemainder, normalize a negative remainder by subtracting one from the quotient and adding ONE_BILLION to the remainder (standard floor-mod normalization) before constructing the Timestamp. Scope is pre-epoch timestamps with explicit timezone suffixes - currently wrong or crashing; post-epoch cases are unaffected since floorDiv and truncating division agree for non-negative values. Refs: 812 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
execute() caught DatabaseUnitException, SQLException, and RuntimeException for rollback - but not Error. When an Error (e.g. AssertionError from a listener, OutOfMemoryError) propagated, the finally still ran setAutoCommit(true), which per JDBC commits the in-flight transaction - silently committing partial operation results and defeating the class's entire purpose. Add a fourth catch (Error e) alongside the three existing catches, rolling back and rethrowing exactly like its siblings. setAutoCommit(true) stays in the finally. Add a new Mockito-based TransactionOperationTest covering: an Error from the delegate rolls back without committing and rethrows unchanged; the success path commits then restores auto-commit, in that order, without ever rolling back; the pre-existing SQLException path still rolls back and rethrows. Additionally: the three pre-existing catch blocks (DatabaseUnitException, SQLException, RuntimeException) had the identical failure-masking bug this commit fixed for Error - each called rollback() unguarded, so a SQLException thrown by rollback() replaced the exception that triggered it instead of surfacing both. Extract a private static handleException(Connection, Throwable) helper that all four catch blocks now call: it attempts rollback and, if that itself throws SQLException, attaches the failure to the original exception via addSuppressed() instead of letting it replace it. This removes the duplicated try/catch that was previously inlined only in the Error block. Add regression tests mirroring the existing Error/rollback-failure case for DatabaseUnitException, SQLException, and RuntimeException: each asserts the original exception is still what's thrown and the rollback failure is attached as suppressed. Refs: 813 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
(1) getRowCount() returned _sheet.getLastRowNum(), a 0-based index: correct by coincidence for header-plus-contiguous-data sheets (the header-row offset and the last-index-vs-count offset cancel out), but -1 for a sheet with no rows at all - a negative row count that then leaks into row-count assertions. Clamp to Math.max(_sheet.getLastRowNum(), 0) so both header-only and truly-empty sheets report 0. (2) getValue() called _sheet.getRow(row + 1).getCell(...) directly; POI returns null for a physically-missing row (a gap row inside the sheet, e.g. after Sheet.removeRow()), producing a raw NullPointerException instead of treating the row's cells as empty, consistent with the existing cell == null -> null branch. Add three tests building POI workbooks in-memory: an empty sheet and a header-only sheet both report zero rows, and a sheet with a physically removed row returns null for that row's cells while still reading the row after the gap correctly. Refs: 814 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
YamlDataSet.write(IDataSet, OutputStream) and FlatDtdDataSet.write(IDataSet, OutputStream) wrapped output in new OutputStreamWriter(out) with the platform default charset, while their corresponding readers decode UTF-8: SnakeYAML reads streams as UTF-8 per the YAML spec, and the DTD InputStream constructor's SAX parsing assumes UTF-8 absent an encoding declaration. On a platform whose default charset is not UTF-8 (e.g. Windows/cp1252), a YAML or DTD export containing non-ASCII text could not be re-read by dbUnit itself. OracleXMLTypeDataType.getSqlValue()/setSqlValue() have the same class of bug in their internal byte[]/String conversions (used for the Base64 representation dbUnit stores XMLTYPE content as): getBytes() and new String(bytes) both used the platform default charset with no explicit pairing between the two methods. Pinned both to UTF-8; the standard JDBC SQLXML.getString()/setString() calls that bracket these conversions already handle the actual XML document encoding safely and are unchanged. BEHAVIOR CHANGE: on a platform whose default charset is not UTF-8, the bytes written by these three writers change to what their readers already expect - a correction, not a regression. CSV read/write remains deliberately excluded: it is symmetric on one machine, so pinning UTF-8 there would break users whose existing fixture files are in the platform charset - a major-version change with a migration note, unlike these three writers whose current output is already broken cross-platform today. Add a YamlDataSet round-trip test for a non-ASCII value through a ByteArrayOutputStream/ByteArrayInputStream, and a FlatDtdDataSetIT test asserting the raw output bytes of a non-ASCII table name decode correctly as UTF-8. Refs: 815 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
The 3.3.0-to-snapshot japicmp reportSet was left commented out with a "enable when new snapshot version set" note. The project version was bumped to 3.4.0-SNAPSHOT in 84c350d, so that condition now holds; without enabling it, `mvn site` only regenerated the stale 3.2.0-to-3.3.0 comparison (of two already-released versions) and produced no report at all for this branch's actual changes, making the plan's end-of-Tier-2/end-of-plan japicmp checkpoints unable to check anything. Verified the resulting target/site/3.3.0-to-snapshot.html report: Access modifier filter is PROTECTED (public/protected surface only, matching this project's own compatibility policy), and every change across the whole report is METHOD_ADDED_TO_PUBLIC_CLASS - purely additive, non-breaking. No incompatibilities found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
…et.getTableMetaData getTableMetaData first proves the table exists (_tableMap.containsTable(tableName) - populated by initialize() from a getTables() enumeration - throwing NoSuchTableException otherwise), then constructed new DatabaseTableMetaData(storedTableName, _connection, true, ...) whose validate=true runs metadataHandler.tableExists(...) - another getTables() metadata round trip per table, per dataset, to re-prove a fact just established. On a full-database export of N tables that is N redundant metadata queries; on remote databases these dominate. Pass validate=false at this single call site. No change to DatabaseTableMetaData itself or the other construction sites. Add a new DatabaseDataSetTest using a real in-memory H2 database (via InMemoryDatabaseConnection) with a Mockito spy wrapping the real IMetadataHandler, avoiding hand-mocked JDBC ResultSet/DatabaseMetaData chains: after getTableNames() + getTableMetaData(), getTables() was called exactly once and tableExists() never. A second test confirms an unknown table still throws NoSuchTableException from the containsTable() check. Refs: 816 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013kCYUs49FaK5yQeym9QXVm
* Guard setSqlValue's 3-argument "setSqlValue(value={}, column={},
statement={}) - start" trace log across the datatype package behind
if (logger.isDebugEnabled()): SLF4J's varargs overload allocates an
Object[] array at the call site regardless of log level once 3 or
more arguments are involved.
* Leave typeCast/compare/compareNonNulls/getSqlValue's 0-2 argument
logger.debug calls unguarded: SLF4J's fixed-arity overloads take
those arguments directly with no array allocation, and the only
primitive ever passed - a single int column index - autoboxes into
a cache-hit Integer for any realistically-sized table, so a guard
would not pay for its own branch there.
* Same reasoning applies to BytesDataType's typeCast branch-selection
diagnostic logs (extended-syntax command dispatch, URI/file/Base64
fallback detection): 0-1 arguments each, left unguarded.
Refs: 820
Refs: 278
… threads * Declare _asyncException volatile so the write on the producer thread (handleException) is guaranteed visible to the consumer thread's read (resolveException); interruption alone establishes no happens-before edge. * Document the existing daemon-thread lifecycle: an abandoned iterator leaves the producer thread parked on the bounded channel until JVM exit. Documentation only, no behavior change. Refs: 821
* In the multi-line continuation loop, a successful parse now always breaks out of the loop, whether or not it met the expected column count, matching the single-line short-row behavior at the method's tail. Only the IllegalStateException path (unterminated quote) continues accumulating, and it already reads a fresh line each time. * Previously, the stale shouldProceed flag from an earlier legitimate continuation let a later successful-but-short parse re-loop and re-append the same already-parsed line instead of reading the next one, duplicating data until the column count accidentally matched or the offending-line message was garbled. Refs: 822
DatabaseDataSet.initialize compared config.getProperty(...) against Boolean.TRUE by reference instead of calling getFeature, unlike every other feature check in the codebase. DatabaseConfig stores a caller-supplied Boolean instance as-is, so a non-canonical instance (deserialization, legacy new Boolean(true)) would silently disable the feature here while reading as enabled everywhere else. No practical way to construct that mismatch without deprecated constructors, so this relies on the existing DatabaseDataSetTest suite for regression; treated as a consistency fix rather than a reproducible defect. Refs: 823
Rewrite copyFile with try-with-resources over both the source and destination FileInputStream/FileOutputStream (closing the streams closes their channels). Previously the source channel was opened before the try, so a failure opening the destination (e.g. writing to a directory) leaked the source stream; and in the finally block, srcChannel.close() throwing skipped dstChannel.close() entirely. Added FileHelperTest, which did not previously exist despite the plan listing it as already present. Refs: 824
…ifecycle * lookupFeatureValue no longer closes the connection it lazily acquired on the success path; only the catch path still closes a connection it acquired itself, so a failing standalone call does not leak. cleanupData already closes the shared connection on every completion path of runTest, including the failure path via postTest(false). * Previously configureTest's case-sensitivity lookup closed its own connection immediately, and setupData then reacquired a fresh one for the rest of the lifecycle - 2 connections per test where 1 suffices. Across configureTest+setupData+verifyData+cleanupData, getConnection() now runs once instead of twice. * As a side effect, cleanupData's own fallback feature lookup (used when configureTest was not called) now also leaves its connection open for a non-NONE tear-down operation to reuse, instead of that operation reacquiring a second one. * Documented in the class and configureTest JavaDoc that a standalone configureTest call now leaves the connection open until cleanupData, matching the existing setupData/verifyData behavior. Refs: 825
…shOperation * Delete UpdateRowOperation's never-assigned PreparedStatement _countStatement field (private inner class, no API surface); it was a leftover copy of the sibling RowExistOperation's actually-used field of the same name. * Nest execute's cleanup finally block so a failure closing updateRowOperation no longer skips closing insertRowOperation. No behavior change on the happy path. No new test: the field was unreferenced everywhere, and the row operations are created internally within execute() with no seam to inject a throwing close() from a unit test. Covered by the existing RefreshOperationIT suite. Refs: 826
* Log the bad-input-character diagnostic via logger.warn instead of System.err.println. * Delete four e.printStackTrace() calls that duplicated an adjacent logger.error(exception) call already reporting the same exception, removing the redundant stderr output. * decode()'s null-return contract on invalid input is unchanged. Added Base64Test (none existed) covering the one behavior change: a WARN log record for bad input instead of console output. Refs: 827
Delete the private setEncoding(String) overload: setWriter(Writer, String) converts its encoding argument via Charset.forName(encoding) and delegates to setWriter(Writer, Charset) directly, so the String overload of setEncoding was never reachable. Even if it were called, its encoding-name matching only recognized "UTF8", never the common "UTF-8" spelling. Left the public main/test1/test2 demo methods in place: removing them is a japicmp-visible public API removal for a low-value cleanup, and main() itself calls test1()/test2(), so removing only two of the three would also require rewriting main(). Took the plan's own sanctioned fallback of leaving all three untouched. No new test: the method was unreachable, so nothing could have exercised it. Refs: 828
* Derive the table name with lastIndexOf(".csv") instead of
indexOf(".csv"). A table named "a.csvx" exports to "a.csvx.csv";
reading it back truncated at the first ".csv" occurrence, so it
read back as table "a" instead of "a.csvx".
* Null-safe the NULL-token comparison by flipping it to
CsvDataSetWriter.NULL.equals(row[col]), avoiding a
NullPointerException if row[col] is itself null.
Refs: 829
Delete slf4jVersion (1.7.25) and snakeYamlVersion (2.2): stale, unreferenced casing/value variants of the actually-used slf4jApiVersion (2.0.18) and snakeyamlVersion (2.6). Confirmed no references anywhere in the repo before deleting. Verified with a clean install and by diffing `mvn dependency:list` output before and after the change: byte-identical. Refs: 830
The two closing braces at the end of the method were indented one level too deep, misleadingly reading as closing the inner if and a phantom block instead of the isDebugEnabled() block and the method body. Brace count was already balanced; cosmetic only, no behavior change. Same bug as its sibling logInfoIfValueChanged (fixed in 804); left out of that fix's scope since it wasn't the commit under review, tracked separately here instead. Refs: 832 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6ZWMnFpE8at7F6Vi3grBv
* Replace the duplicated Locale.getDefault()/setDefault()/try-finally boilerplate in 13 test classes with a @TurkishDefaultLocale annotation backed by a JUnit 5 extension (TurkishLocaleExtension). * Flagged by CodeRabbit's review of PR #801: the JVM-wide default locale mutation was hand-rolled in every affected test and would race under parallel test execution. Refs: 833
setFormat(String) has always accepted "csv" via isSupportedFormat(), but its IllegalArgumentException message listed only 'flat'(default), 'xml', 'dtd', 'xls', and 'yml' - omitting the one format this area's other bugs (issue 808) are specifically about, and misleading anyone who hit the exception into thinking csv wasn't supported. Refs: 808 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqQTzFd5qsXiMTpFsReD12
jeffjensen
force-pushed
the
800-defaultprepandexpected
branch
from
July 27, 2026 17:24
e021bd1 to
b7be25f
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
jeffjensen
added a commit
that referenced
this pull request
Jul 27, 2026
* Replace the duplicated Locale.getDefault()/setDefault()/try-finally boilerplate in 13 test classes with a @TurkishDefaultLocale annotation backed by a JUnit 5 extension (TurkishLocaleExtension). * Flagged by CodeRabbit's review of PR #801: the JVM-wide default locale mutation was hand-rolled in every affected test and would race under parallel test execution. Refs: 833
4 tasks
This was referenced Aug 15, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
CachingConnectionProviderfor connection reuse across testers.closeConnectionAfterTest(default: close).