[Feat][Connector-v2][JDBC][Oracle] Add Testcontainers-based unit tests #10435
[Feat][Connector-v2][JDBC][Oracle] Add Testcontainers-based unit tests #10435zooo-code wants to merge 5 commits intoapache:devfrom
Conversation
Issue 1: Test method lacks assertionsLocation: @Test
public void testQueryNextChunkMax() throws Exception {
TablePath tablePath = TablePath.of(DATABASE, SCHEMA, TEST_TABLE);
JdbcSourceTable table =
JdbcSourceTable.builder()
.tablePath(tablePath)
.useSelectCount(false)
.skipAnalyze(false)
.build();
Object maxValue = dialect.queryNextChunkMax(connection, table, "ID", 10, 0);
}Related Context:
Problem Description: Potential Risks:
Impact Scope:
Severity: MAJOR Improvement Suggestions: @Test
public void testQueryNextChunkMax() throws Exception {
TablePath tablePath = TablePath.of(DATABASE, SCHEMA, TEST_TABLE);
JdbcSourceTable table =
JdbcSourceTable.builder()
.tablePath(tablePath)
.useSelectCount(false)
.skipAnalyze(false)
.build();
// Test first chunk
Object firstChunkMax = dialect.queryNextChunkMax(connection, table, "ID", 3, 1);
Assertions.assertNotNull(firstChunkMax);
Assertions.assertEquals(1, ((Number) firstChunkMax).intValue());
// Test second chunk
Object secondChunkMax = dialect.queryNextChunkMax(connection, table, "ID", 3, 1);
Assertions.assertNotNull(secondChunkMax);
// Should return the only ID value (1) since table only has one row
Assertions.assertEquals(1, ((Number) secondChunkMax).intValue());
}Rationale: Referencing the implementation of Issue 2: Testcontainers version is outdatedLocation: <dependency>
<groupId>org.testcontainers</groupId>
<artifactId>oracle-xe</artifactId>
<version>${testcontainer.version}</version>
<scope>test</scope>
</dependency>Related Context:
Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions:
Rationale: Although the version is old, it does not affect functionality. Suggested as a follow-up optimization item to be handled together when upgrading Testcontainers version globally. Issue 3: Debug output should use logging frameworkLocation:
System.out.println("Available databases: " + databases);
System.out.println("Tables found: " + tables);Related Context:
Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: import lombok.extern.slf4j.Slf4j;
@Slf4j
public class OracleCatalogContainerTest extends AbstractOracleContainerTest {
@Test
public void testDatabaseExists() {
List<String> databases = catalog.listDatabases();
log.info("Available databases: {}", databases);
// ...
}
@Test
public void testListTables() throws SQLException {
// ...
log.info("Tables found: {}", tables);
// ...
}
}Rationale: Using Slf4j maintains consistency with the project's logging framework, supports log level control, and avoids generating excessive noise in CI. For debug information, Issue 4: Windows platform disablement lacks documentationLocation: @DisabledOnOs(OS.WINDOWS)
public abstract class AbstractOracleContainerTest {Related Context:
Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: /**
* Base class for Oracle Testcontainers-based unit tests. Provides shared Oracle container setup and
* connection management.
*
* <p>Note: Tests are disabled on Windows due to Docker Desktop limitations with shared memory
* configuration required by Oracle container. Use WSL2 or Linux/macOS for running these tests.
*/
@DisabledOnOs(OS.WINDOWS)
public abstract class AbstractOracleContainerTest {Rationale: Documenting the reason for disablement in JavaDoc helps Windows developers understand the limitation and provides alternative solutions (WSL2). |
| @AfterAll | ||
| public static void stopContainer() throws SQLException { | ||
| if (catalog != null) { | ||
| catalog.close(); |
There was a problem hiding this comment.
Thank you for your contribution. If an exception is thrown when closing the catalog, will the following be unable to close
There was a problem hiding this comment.
Added try-catch blocks to ensure all resources are closed even if catalog.close() throws an exception.
| + quoteIdentifier(TEST_TABLE), | ||
| 128); | ||
|
|
||
| Assertions.assertNotNull(preparedStatement); |
There was a problem hiding this comment.
If the assertion here fails, preparedStatement cannot be closed
There was a problem hiding this comment.
@LiJie20190102 I've changed it to use try-with-resources pattern to ensure the PreparedStatement is always closed. Fixed in the latest commit.
3f94907 to
0c39ed5
Compare
|
I followed the pattern in #10252 PR, with a few adjustments to align with project conventions
|
adfb234 to
c0fc604
Compare
LiJie20190102
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Could you migrate the unit tests from the jdbc-e2e module to connector-jdbc later? Or could you submit them together this time
Thanks for the approval! @LiJie20190102 I'd prefer to keep this PR focused on adding the Testcontainers-based tests, as it's already been through several review iterations and is ready to merge. I can create a follow-up PR to migrate the existing unit tests from the jdbc-e2e module to connector-jdbc.
However, if you prefer merging them together in this PR, I'm happy to work on the migration now. Please let me know! |
| * quoting, SQL generation, and data sampling. | ||
| */ | ||
| @DisabledOnOs(OS.WINDOWS) | ||
| public class OracleDialectTest extends AbstractOracleContainerTest { |
There was a problem hiding this comment.
Add getInsertIntoStatement(), getUpdateStatement(), getDeleteStatement(), getRowExistsStatement(), getUpsertStatement() and other corresponding unit tests, and it is best to add FieldNamedPreparedStatement.prepareStatement().
There was a problem hiding this comment.
@chl-wxp Thanks for the review!
I've added all the requested tests as suggested.
(10 new tests)
-
DML Statements
- Covered Insert, Update, Delete, RowExists, and Upsert logic.
-
FieldNamedPreparedStatement
- Covered parameter parsing, duplicate handling, and execution.
All tests passed locally.
Thanks!
…s for Oracle connector
…framework in tests
…asses and rename test classes
c0fc604 to
374f3e9
Compare
Purpose of this pull request
Add comprehensive Testcontainers-based unit tests for the Oracle JDBC connector to improve test coverage and enable CI execution.
This PR addresses issue #10212 by introducing Docker-based unit tests that can run in CI environments without requiring a local Oracle installation.
Does this PR introduce any user-facing change?
No.
This PR only adds unit tests for the Oracle connector. No functional changes to the connector itself.
How was this patch tested?
New Tests Added
AbstractOracleContainerTest: Base class providing Oracle container setup with ARM64 supportOracleCatalogContainerTest: 9 tests for catalog operations (database/table management, complex types, primary keys)OracleDialectContainerTest: 15 tests for Oracle-specific SQL features (DUAL, ORA_HASH, identifier quoting, data sampling)Test Environment
gvenzl/oracle-free:23-slimDocker image for ARM64 / Apple Silicon compatibilityTest Results
All 570 tests in the
connector-jdbcmodule pass successfully, including 24 new Oracle tests.Local Testing
Check list
incompatible-changes.mdto describe the incompatibility caused by this PR.