[GitHub] [flink-connector-jdbc] wanglijie95 commented on a diff in pull request #12: [FLINK-27940] Migrate tests to junit5

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org