snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998346563
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientITCase.java:
##########
@@ -124,45 +136,52 @@ public static void setup() throws IOException {
File udfJar =
UserClassLoaderJarTestUtils.createJarFile(
- tempFolder.newFolder("test-jar"),
+ Files.createTempDirectory(tempFolder,
"test-jar").toFile(),
"test-classloader-udf.jar",
classNameCodes);
URL udfDependency = udfJar.toURI().toURL();
String path = udfDependency.getPath();
// we need to pad the displayed "jars" tableau to have the same width
of path string
// 4 for the "jars" characters, see `set.q` test file
int paddingLen = path.length() - 4;
- historyPath = tempFolder.newFile("history").toPath();
+ historyPath = Files.createTempFile(tempFolder, "history", "");
replaceVars = new HashMap<>();
replaceVars.put("$VAR_UDF_JAR_PATH", path);
replaceVars.put("$VAR_UDF_JAR_PATH_DASH", repeat('-', paddingLen));
replaceVars.put("$VAR_UDF_JAR_PATH_SPACE", repeat(' ', paddingLen));
replaceVars.put("$VAR_PIPELINE_JARS_URL", udfDependency.toString());
- replaceVars.put(
- "$VAR_REST_PORT",
-
MINI_CLUSTER_RESOURCE.getClientConfiguration().get(PORT).toString());
- replaceVars.put(
- "$VAR_JOBMANAGER_RPC_ADDRESS",
- MINI_CLUSTER_RESOURCE.getClientConfiguration().get(ADDRESS));
+ replaceVars.put("$VAR_REST_PORT", configuration.get(PORT).toString());
+ replaceVars.put("$VAR_JOBMANAGER_RPC_ADDRESS",
configuration.get(ADDRESS));
}
- @Before
+ @BeforeEach
public void before() throws IOException {
Review Comment:
```suggestion
void before() throws IOException {
```
It could be package private
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -269,15 +269,15 @@ public void testCancelBatchResult() throws Exception {
.isEqualTo("Query terminated, received a total of 0 row" +
System.lineSeparator());
// didn't have a chance to read page
- assertThat(mockExecutor.getNumRetrieveResultPageCalls()).isEqualTo(0);
+ assertThat(mockExecutor.getNumRetrieveResultPageCalls()).isZero();
// tried to cancel query
assertThat(mockExecutor.getNumCancelCalls()).isEqualTo(1);
Review Comment:
```suggestion
assertThat(mockExecutor.getNumCancelCalls()).isOne();
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -484,7 +484,7 @@ public void testCancelStreamingResult() throws Exception {
}
Review Comment:
The line started at 464 could be simplified with
`assertThat(terminalOutput).hasToString(`
and the line started at 483 could be simplified a bit like
```java
assertThat(mockExecutor.getNumCancelCalls()).isOne();
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/context/SessionContextTest.java:
##########
@@ -131,7 +132,7 @@ public void testSetAndResetKeyInConfigOptions() {
}
@Test
- public void testSetAndResetArbitraryKey() {
+ void testSetAndResetArbitraryKey() {
Review Comment:
This test could be a bit simplified as
```java
@Test
void testSetAndResetArbitraryKey() {
// other property not in flink-conf
sessionContext.set("aa", "11");
sessionContext.set("bb", "22");
assertThat(getConfigurationMap()).containsEntry("aa",
"11").containsEntry("bb", "22");
sessionContext.reset("aa");
assertThat(getConfigurationMap()).doesNotContainKey("aa").containsEntry("bb",
"22");
sessionContext.reset("bb");
assertThat(getConfigurationMap()).doesNotContainKey("bb");
}
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/context/SessionContextTest.java:
##########
@@ -53,31 +54,31 @@
import static org.assertj.core.api.HamcrestCondition.matching;
/** Test {@link SessionContext}. */
-public class SessionContextTest {
+class SessionContextTest {
- @ClassRule public static TemporaryFolder tempFolder = new
TemporaryFolder();
+ @TempDir private static Path tempFolder;
private static File udfJar;
private SessionContext sessionContext;
- @BeforeClass
+ @BeforeAll
public static void prepare() throws Exception {
Review Comment:
```suggestion
static void prepare() throws Exception {
```
could be package private
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -183,7 +183,7 @@ public void setUp() {
}
@Test
- public void testBatchResult() {
+ void testBatchResult() {
Review Comment:
the code at line 209
```java
assertThat(terminalOutput.toString())
.isEqualTo(
```
could be simplified by usage `assertThat(terminalOutput).hasToString(`
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -426,11 +426,11 @@ public void testEmptyStreamingResult() {
+ System.lineSeparator()
Review Comment:
The line started at 419 could be simplified a bit with usage of
`assertThat(terminalOutput).hasToString(`
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java:
##########
@@ -82,7 +83,8 @@
import static org.assertj.core.api.Assertions.assertThat;
/** Tests for the {@link CliClient}. */
-public class CliClientTest extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
Using the `META-INF/services/org.junit.jupiter.api.extension.Extension` is
the better approach here
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -335,7 +335,7 @@ public void testFailedBatchResult() {
}
Review Comment:
the line above could be a bit simplified like
```java
assertThat(mockExecutor.getNumCancelCalls()).isOne();
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliResultViewTest.java:
##########
@@ -58,35 +58,35 @@
import static org.assertj.core.api.Assertions.assertThat;
/** Contains basic tests for the {@link CliResultView}. */
-public class CliResultViewTest {
+class CliResultViewTest {
@Test
public void testTableResultViewKeepJobResult() throws Exception {
Review Comment:
```suggestion
void testTableResultViewKeepJobResult() throws Exception {
```
It could be package private
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -484,7 +484,7 @@ public void testCancelStreamingResult() throws Exception {
}
@Test
- public void testFailedStreamingResult() {
+ void testFailedStreamingResult() {
final Configuration testConfig = new Configuration();
testConfig.set(EXECUTION_RESULT_MODE, ResultMode.TABLEAU);
testConfig.set(RUNTIME_MODE, RuntimeExecutionMode.STREAMING);
Review Comment:
The line started at 516 could be simplified with usage of
`assertThat(terminalOutput).hasToString(`
The line started at 532 could be a bit simplified as
```java
assertThat(mockExecutor.getNumCancelCalls()).isOne();
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -394,11 +394,11 @@ public void testStreamingResult() {
+ System.lineSeparator()
Review Comment:
The expression started at line 370 could be simplified with usage
`assertThat(terminalOutput).hasToString(`
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -300,11 +300,11 @@ public void testEmptyBatchResult() {
view.close();
assertThat(terminalOutput.toString()).isEqualTo("Empty set" +
System.lineSeparator());
Review Comment:
```suggestion
assertThat(terminalOutput).hasToString("Empty set" +
System.lineSeparator());
```
could be a bit simplified
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/context/SessionContextTest.java:
##########
@@ -53,31 +54,31 @@
import static org.assertj.core.api.HamcrestCondition.matching;
/** Test {@link SessionContext}. */
-public class SessionContextTest {
+class SessionContextTest {
- @ClassRule public static TemporaryFolder tempFolder = new
TemporaryFolder();
+ @TempDir private static Path tempFolder;
private static File udfJar;
private SessionContext sessionContext;
- @BeforeClass
+ @BeforeAll
public static void prepare() throws Exception {
udfJar =
UserClassLoaderJarTestUtils.createJarFile(
- tempFolder.newFolder("test-jar"),
+ Files.createTempDirectory(tempFolder,
"test-jar").toFile(),
"test-classloader-udf.jar",
GENERATED_LOWER_UDF_CLASS,
String.format(GENERATED_LOWER_UDF_CODE,
GENERATED_LOWER_UDF_CLASS));
}
- @Before
+ @BeforeEach
public void setup() {
Review Comment:
```suggestion
void setup() {
```
could be package private
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliTableauResultViewTest.java:
##########
@@ -269,15 +269,15 @@ public void testCancelBatchResult() throws Exception {
.isEqualTo("Query terminated, received a total of 0 row" +
System.lineSeparator());
Review Comment:
This could be a bit simplified like by usage `hasToString`
```java
assertThat(terminalOutput).hasToString(
"Query terminated, received a total of 0 row" +
System.lineSeparator());
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientITCase.java:
##########
@@ -76,8 +82,8 @@
import static org.assertj.core.api.Assertions.assertThat;
/** Test that runs every {@code xx.q} file in "resources/sql/" path as a test.
*/
-@RunWith(Parameterized.class)
-public class CliClientITCase extends AbstractTestBase {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
Using the `META-INF/services/org.junit.jupiter.api.extension.Extension` is
the better approach here
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -433,8 +427,7 @@ public void testBatchQueryExecutionMultipleTimes() throws
Exception {
}
@Test
- @Timeout(value = 90)
- public void testStopJob() throws Exception {
+ void testStopJob() throws Exception {
Review Comment:
The line in this method `assertThat(savepoint.isPresent()).isTrue();` could
be simplified as
`assertThat(savepoint).isPresent();`
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -86,7 +86,8 @@
import static org.assertj.core.api.Assertions.assertThat;
/** Contains basic tests for the {@link LocalExecutor}. */
-public class LocalExecutorITCase extends TestLogger {
+@ExtendWith(TestLoggerExtension.class)
Review Comment:
Using the `META-INF/services/org.junit.jupiter.api.extension.Extension` is
the better approach here
--
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]
