DanielCarter-stack commented on PR #10573:
URL: https://github.com/apache/seatunnel/pull/10573#issuecomment-4014823971
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10573", "part": 1,
"total": 1} -->
### Issue 1: Conditional judgment in URL conversion logic always evaluates
to false
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/sqlserver/container/AbstractSqlServerContainerTest.java:90`
**Related Context**:
- Parent class: None (independent base class)
- Subclasses: `SqlServerCatalogTest.java`,
`SqlServerDialectContainerTest.java`
- Caller 1: `createSqlServerCatalog()` (line 100-110)
- Caller 2: `SqlServerCatalogTest.testCatalogOperations()` (line 84)
- Caller 3: `SqlServerCatalogTest.testCatalogSaveMode()` (line 155)
**Problem Description**:
The conditional judgment `if (!baseUrl.contains("/"))` on line 90 always
evaluates to false, because `baseUrl` at this point is
`jdbc:sqlserver://localhost:1433`, which already contains `://`. This causes
the database concatenation logic on line 91 to never execute.
Although this bug does not cause actual test failures in the current
implementation (because the database part in JdbcUrlUtil's regular expression
is optional), this is dead code and will mislead future maintainers.
**Potential Risks**:
- **Risk 1**: Reduced code readability. Future maintainers may be confused
about why a branch that never executes was added
- **Risk 2**: If someone mistakenly thinks this logic is useful and copies
this code elsewhere, it may introduce real problems
- **Risk 3**: If JdbcUrlUtil's regular expression is modified in the future
to require a database part, this bug will cause test failures
**Impact Scope**:
- **Direct Impact**: `AbstractSqlServerContainerTest.getJdbcUrlInfo()` method
- **Indirect Impact**: All test classes that inherit this base class
(`SqlServerCatalogTest`, `SqlServerDialectContainerTest`)
- **Impact Area**: Single Connector (SQL Server JDBC Connector tests)
**Severity**: MINOR
**Improvement Suggestion**:
```java
protected JdbcUrlUtil.UrlInfo getJdbcUrlInfo() {
String jdbcUrl = getJdbcUrl();
String databaseName = "master";
if (jdbcUrl.startsWith("jdbc:sqlserver://")) {
String baseUrl = jdbcUrl;
if (jdbcUrl.contains(";")) {
baseUrl = jdbcUrl.substring(0, jdbcUrl.indexOf(";"));
}
// Check if there is a database path after host:port (excluding the
:// part)
String afterProtocol = baseUrl.substring(baseUrl.indexOf("://") + 3);
if (!afterProtocol.contains("/")) {
baseUrl = baseUrl + "/" + databaseName;
}
return JdbcUrlUtil.getUrlInfo(baseUrl);
}
return JdbcUrlUtil.getUrlInfo(jdbcUrl);
}
```
**Rationale**:
- The fixed logic correctly checks whether `/` exists after `host:port`
- Maintains the original intent (if URL has no database, add default
database)
- Since the database part in JdbcUrlUtil's current regular expression is
optional, this modification does not affect the current behavior of tests
- But the code logic is now correct, providing a better foundation for
future modifications
---
### Issue 2: Test method is too long, violating single responsibility
principle
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/sqlserver/container/SqlServerCatalogTest.java:145-288`
**Related Context**:
- Belonging Class: `SqlServerCatalogTest extends
AbstractSqlServerContainerTest`
- Test Method Name: `testCatalogSaveMode()`
- Called APIs: `catalog.createTable()`, `catalog.tableExists()`,
`catalog.isExistsData()`, `catalog.truncateTable()`, `catalog.dropTable()`
**Problem Description**:
The `testCatalogSaveMode()` method is 143 lines long (145-288), testing
multiple independent functions:
1. Create source table with special character comments (lines 160-180)
2. Verify source table comments (lines 190-202)
3. Create target table using SaveMode (line 208)
4. Verify target table comments (lines 215-233)
5. Verify data existence (lines 236-255)
6. Test TRUNCATE operation (lines 258-263)
7. Test DROP operation (lines 266-269)
This violates the single responsibility principle of unit testing, making it:
- Unclear test intent (method name only mentions "SaveMode", but actually
tests the entire CRUD lifecycle)
- Difficult to locate failure causes (if an assertion fails, careful
checking is needed to determine which scenario)
- Difficult to maintain (if one scenario needs modification in the future,
it may affect other scenarios)
**Potential Risks**:
- **Risk 1**: Reduced test maintainability. When tests fail, more time is
needed to locate problems
- **Risk 2**: Unclear test coverage. Difficult to see whether each function
point is independently tested
- **Risk 3**: Increased test execution time. Failure of one method causes
the entire method to stop executing, and subsequent scenarios cannot be verified
**Impact Scope**:
- **Direct Impact**: `SqlServerCatalogTest.testCatalogSaveMode()` method
- **Indirect Impact**: Test report readability
- **Impact Area**: Single Connector (SQL Server JDBC Connector tests)
**Severity**: MINOR
**Improvement Suggestion**:
Split `testCatalogSaveMode()` into multiple independent test methods:
```java
@Test
public void testCreateTableWithComplexComment() throws SQLException {
String schema = "dbo";
String databaseName = "master";
String testTableName = "test_complex_comment_table";
TablePath sourceTablePath = TablePath.of(databaseName, schema,
testTableName);
SqlServerCatalog catalog = new SqlServerCatalog(
"sqlserver", getUsername(), getPassword(), getJdbcUrlInfo(), schema,
null);
catalog.open();
try {
String createTableSQL = "..."; // Create table with special
character comments
executeSql(createTableSQL);
CatalogTable catalogTable = catalog.getTable(sourceTablePath);
String actualComment =
catalogTable.getTableSchema().getColumns().stream()
.filter(col -> "complex_comment_col".equals(col.getName()))
.findFirst()
.map(Column::getComment)
.orElse(null);
assertEquals("\"#¥%……&*（）;;',,.\\.``````// '@Special
comment'\\\\'\"", actualComment);
} finally {
executeSql("DROP TABLE IF EXISTS " + sourceTablePath.getFullName());
catalog.close();
}
}
@Test
public void testCreateTableWithSaveModePreservesComments() throws
SQLException {
// Test whether createTable() preserves comments
// ...
}
@Test
public void testTruncateTable() throws SQLException {
// Test truncateTable() operation
// ...
}
@Test
public void testDropTable() throws SQLException {
// Test dropTable() operation
// ...
}
```
**Rationale**:
- Each test method focuses on one function point, conforming to the single
responsibility principle
- If a test fails, problems can be quickly located
- Test reports are clearer
- Can use `@MethodOrderer` to control execution order (if needed)
- Improves code maintainability and readability
---
### Issue 3: Error handling is not user-friendly, lacks Docker availability
check
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/sqlserver/container/AbstractSqlServerContainerTest.java:48-54`
**Related Context**:
- Parent class: None
- Subclasses: `SqlServerCatalogTest`, `SqlServerDialectContainerTest`
- Dependency: `org.testcontainers.containers.MSSQLServerContainer`
**Problem Description**:
The container startup logic in the `@BeforeAll` method lacks error handling.
If Docker is not installed locally or the Docker daemon is not running,
`MSSQL_CONTAINER.start()` will throw an exception, causing test failure. The
error message is similar to:
```
org.testcontainers.containers.ContainerLaunchException: Container startup
failed
Caused by: java.lang.IllegalStateException: Cannot connect to the Docker
daemon
```
This is not user-friendly for developers because:
1. The error message does not clearly indicate this is a Docker issue
2. No prompt on how to solve the problem (install Docker or skip tests)
3. In CI environments, it may be misreported as test failure
**Potential Risks**:
- **Risk 1**: Poor developer experience. When running tests for the first
time, if Docker installation is forgotten, obscure error messages will be seen
- **Risk 2**: CI/CD false positives. If CI environment temporarily lacks
Docker, test failures may be mistakenly considered code issues
- **Risk 3**: Reduced test portability. Tests cannot run in environments
without Docker (such as some development machines)
**Impact Scope**:
- **Direct Impact**: All test classes that inherit
`AbstractSqlServerContainerTest`
- **Indirect Impact**: CI/CD processes
- **Impact Area**: Single Connector (SQL Server JDBC Connector tests)
**Severity**: MINOR
**Improvement Suggestion**:
```java
@BeforeAll
static void startContainer() {
try {
MSSQL_CONTAINER =
new
MSSQLServerContainer<>(DockerImageName.parse(MSSQL_IMAGE))
.acceptLicense();
MSSQL_CONTAINER.start();
} catch (Exception e) {
log.error("Failed to start SQL Server container. Please ensure:", e);
log.error("1. Docker is installed and running (docker ps)");
log.error("2. You have permission to access Docker (user in docker
group)");
log.error("3. Sufficient memory is available (SQL Server requires
~2GB)");
throw new org.junit.jupiter.api.AssumptionViolatedException(
"Docker not available, skipping SQL Server integration
tests", e);
}
}
```
**Rationale**:
- Using `AssumptionViolatedException` instead of ordinary exceptions, the
test framework will mark it as "skipped" rather than "failed"
- Provides clear error messages and solutions
- Maintains test robustness
- Conforms to JUnit 5 best practices (using Assumptions to handle
environment dependencies)
---
### Issue 4: 25 instances of System.out.println used, should be replaced
with logging
**Location**: Multiple locations in multiple test files
**Related Context**:
- `SqlServerCatalogTest.java`: 25 instances of `System.out.println`
- `SqlServerDialectContainerTest.java`: 3 instances of `System.out.println`
**Problem Description**:
Test code extensively uses `System.out.println()` instead of logging
frameworks, for example:
```java
System.out.println("SQL Server container is running at: " + getJdbcUrl());
System.out.println("All indexes correctly identified by catalog");
System.out.println("Data round-trip successful:");
```
This violates logging best practices:
1. Unable to control log levels (DEBUG, INFO, WARN, ERROR)
2. Unable to output to log files for centralized analysis
3. May produce unnecessary output in production environment testing
4. Inconsistent with the style of using `log.info()` elsewhere in the project
**Potential Risks**:
- **Risk 1**: Chaotic log management. Unable to disable these outputs
through configuration
- **Risk 2**: Performance impact. Even when debug information is not needed,
these outputs will still be executed
- **Risk 3**: Incomplete logging. Test logs cannot be centrally managed
**Impact Scope**:
- **Direct Impact**: All SQL Server test classes
- **Indirect Impact**: Log file clarity
- **Impact Area**: Single Connector
**Severity**: MINOR
**Improvement Suggestion**:
Replace `System.out.println` with `log.info` or `log.debug`:
```java
// Before replacement
System.out.println("SQL Server container is running at: " + getJdbcUrl());
// After replacement
log.info("SQL Server container is running at: {}", getJdbcUrl());
// Before replacement
System.out.println("All indexes correctly identified by catalog");
// After replacement
log.info("All indexes correctly identified by catalog");
// For debug information, use debug level
log.debug("Found {} constraint keys", constraintKeys.size());
```
**Rationale**:
- Consistent with logging style elsewhere in the project (e.g., `@Slf4j` is
used in `SqlServerCatalog`)
- Can control log output through configuration
- Facilitates collection and analysis of test logs in CI/CD
- Conforms to Java logging best practices
--
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]
