DanielCarter-stack commented on PR #10435:
URL: https://github.com/apache/seatunnel/pull/10435#issuecomment-3832926343
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10435", "part": 1,
"total": 1} -->
## Issue 1: Test method lacks assertions
**Location**: `OracleDialectContainerTest.java:172-183`
```java
@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**:
- Reference: `DuckDBDialectTest.java:167-173` has complete assertions for
the same method
- Method under test: `OracleDialect.queryNextChunkMax()`
(OracleDialect.java:263-310)
**Problem Description**:
This test method calls `queryNextChunkMax()` but does not perform any
assertion verification on the return value. Referencing the same test in
`DuckDBDialectTest`, the returned maximum value should be verified as expected.
The current implementation will "pass" even if the method returns null or
throws an exception (unless the exception directly causes failure).
**Potential Risks**:
- Cannot verify the logic correctness of `queryNextChunkMax()`
- Cannot catch regression issues (e.g., future modifications breaking this
method)
- False high test coverage (method is called but not verified)
**Impact Scope**:
- Direct impact: `OracleDialect.queryNextChunkMax()` method
- Indirect impact: Oracle data source reading functionality that depends on
chunk sharding
- Affected area: Oracle Connector's data sharding logic
**Severity**: MAJOR
**Improvement Suggestions**:
```java
@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 `DuckDBDialectTest`, the
return value should be verified as expected. Since only one row of data (ID=1)
was inserted in the test table, the chunk sharding should correctly handle this
case.
---
## Issue 2: Testcontainers version is outdated
**Location**: `pom.xml:431-439`
```xml
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>oracle-xe</artifactId>
<version>${testcontainer.version}</version>
<scope>test</scope>
</dependency>
```
**Related Context**:
- Parent POM definition: `testcontainer.version = 1.17.6` (pom.xml:141)
- Other modules in the project: connector-databend-e2e uses 1.20.2,
connector-hudi-e2e uses 1.19.1
**Problem Description**:
Testcontainers 1.17.6 was released in 2022 and is relatively old. Although
this version works, there are the following issues:
1. Lacks new features and bug fixes
2. May have known security issues
3. Inconsistent with versions used in other modules of the project (e.g.,
databend 1.20.2)
**Potential Risks**:
- Possible unfixed bugs affecting test stability
- Compatibility issues with newer Docker/Java versions
- Security vulnerabilities (although test dependencies don't affect
production, CI environment is still affected)
**Impact Scope**:
- Direct impact: Oracle tests in the connector-jdbc module
- Indirect impact: Test execution in CI environment
- Affected area: Test dependencies of a single module
**Severity**: MINOR
**Improvement Suggestions**:
Consider upgrading Testcontainers to version 1.19.x or 1.20.x to align with
other modules in the project. However, this requires:
1. Upgrading the `testcontainer.version` property in the parent POM
2. Verifying compatibility of all modules using Testcontainers
3. Handling in a separate issue, should not block the current PR
**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 framework
**Location**:
- `OracleCatalogContainerTest.java:52`
- `OracleCatalogContainerTest.java:180`
```java
System.out.println("Available databases: " + databases);
System.out.println("Tables found: " + tables);
```
**Related Context**:
- Production code uses Slf4j + Logback (see OracleDialect.java:39 uses
`@Slf4j`)
- Test code should also follow unified standards
**Problem Description**:
Test code uses `System.out.println` for debug output instead of using a
logging framework. This practice in CI environments may lead to:
1. Inconsistent log output format
2. Unable to control output through log levels
3. Confused output during concurrent testing
**Potential Risks**:
- Reduced CI log readability
- Unable to disable debug output through configuration
- Inconsistent with project logging standards
**Impact Scope**:
- Direct impact: Log output of OracleCatalogContainerTest
- Indirect impact: CI log readability
- Affected area: Test code quality
**Severity**: MINOR
**Improvement Suggestions**:
```java
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, `log.debug()` should be used instead of
`log.info()`, or removed after confirming test stability.
---
## Issue 4: Windows platform disablement lacks documentation
**Location**: `AbstractOracleContainerTest.java:40`
```java
@DisabledOnOs(OS.WINDOWS)
public abstract class AbstractOracleContainerTest {
```
**Related Context**:
- PR description mentions "Disabled on Windows for CI compatibility"
- Other test classes do not use this annotation (e.g., DuckDBDialectTest)
**Problem Description**:
The test uses `@DisabledOnOs(OS.WINDOWS)` to disable the Windows platform,
but does not explain the reason in JavaDoc or code comments. This may lead to:
1. Confusion among Windows developers about why the test is skipped
2. Unclear whether it's a Docker Desktop for Windows limitation or other
reasons
**Potential Risks**:
- Windows developers may mistakenly think there's an issue with the test
- Cannot track whether there are plans to support Windows in the future
**Impact Scope**:
- Direct impact: Developers on Windows platform
- Indirect impact: Test documentation completeness
- Affected area: Cross-platform test support
**Severity**: MINOR
**Improvement Suggestions**:
```java
/**
* 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).
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
