wanglijie95 commented on code in PR #12:
URL:
https://github.com/apache/flink-connector-jdbc/pull/12#discussion_r1055463333
##########
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/catalog/MySqlCatalogTestBase.java:
##########
@@ -140,10 +140,18 @@ public static void beforeAll() throws SQLException {
}
}
- @AfterClass
- public static void cleanup() {
+ @AfterAll
+ static void cleanup() {
for (MySQLContainer<?> container : MYSQL_CONTAINERS.values()) {
container.stop();
}
}
+
Review Comment:
These 2 methods seem useless
##########
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/dialect/sqlserver/SqlServerTableSinkITCase.java:
##########
@@ -81,9 +86,8 @@ public class SqlServerTableSinkITCase extends
AbstractTestBase {
public static final String OUTPUT_TABLE5 = "checkpointTable";
public static final String USER_TABLE = "USER_TABLE";
- @BeforeClass
- public static void beforeAll() throws ClassNotFoundException, SQLException
{
- container.start();
+ @BeforeAll
+ static void beforeAll() throws ClassNotFoundException, SQLException {
Review Comment:
Why remove `container.start` ?
##########
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/xa/JdbcXaSinkMigrationTest.java:
##########
@@ -77,7 +79,8 @@ public JdbcXaSinkMigrationTest(FlinkVersion readVersion) {
private final FlinkVersion readVersion;
- @Test
+ @TestTemplate
+ @Disabled
Review Comment:
It would be better to add comments to explain it.
##########
flink-connector-jdbc/src/test/java/org/apache/flink/connector/jdbc/dialect/sqlserver/SqlServerPreparedStatementTest.java:
##########
@@ -26,7 +26,7 @@
import static org.assertj.core.api.Assertions.assertThat;
/** Tests for {@link SqlServerPreparedStatementTest}. */
-class SqlServerPreparedStatementTest {
+public class SqlServerPreparedStatementTest {
Review Comment:
[JUnit 5 Migration
Guide](https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU)
recommends omitting the public modifier for them(classes, test methods, and
lifecycle methods) unless there is a technical reason.
I think it's ok to leave it as is, and we should remove the `public`
modifier of other test classes.
--
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]
