You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/07/05 10:54:48 UTC
[GitHub] [flink] zhoulii opened a new pull request, #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
zhoulii opened a new pull request, #20168:
URL: https://github.com/apache/flink/pull/20168
## What is the purpose of the change
[JUnit5 Migration] Module: flink-sql-client
## Verifying this change
This change is already covered by existing tests.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented)
--
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
[GitHub] [flink] zhoulii commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1283459186
@flinkbot run azure
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998862192
##########
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:
done
--
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
[GitHub] [flink] XComp commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
XComp commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1271241763
@zhoulii are you able to work on this considering @snuyanzin 's review comments?
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r968213127
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/SqlClientTest.java:
##########
@@ -38,36 +37,35 @@
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.TimeUnit;
import static org.apache.flink.configuration.ConfigConstants.ENV_FLINK_CONF_DIR;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Tests for {@link SqlClient}. */
-public class SqlClientTest {
+@Timeout(value = 1000)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 1, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 1000, unit = TimeUnit.MILLISECONDS)
```
--
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
[GitHub] [flink] zhoulii commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1281741638
Thanks for reviewing, I will rebase the change and address the comments.
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r968211878
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java:
##########
@@ -231,8 +234,9 @@ public void testIllegalStatementInInitFile() throws Exception {
assertThat(cliClient.executeInitialization(content)).isFalse();
}
- @Test(timeout = 10000)
- public void testCancelExecutionInNonInteractiveMode() throws Exception {
+ @Test
+ @Timeout(10000)
Review Comment:
By Default JUnit4 works with millis while JUnit 5 works with seconds...
that's why I would suggest to specify timeunits since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 10, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
```
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998861965
##########
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:
done
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998345986
##########
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)
+public class CliClientITCase {
Review Comment:
```suggestion
class CliClientITCase {
```
It could be package private
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r997903874
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/utils/SqlParserHelper.java:
##########
@@ -26,7 +26,7 @@
import org.apache.flink.table.catalog.hive.HiveTestUtils;
import org.apache.flink.table.delegation.Parser;
-/** An utility class that provides pre-prepared tables and sql parser. */
+/** A utility class that provides pre-prepared tables and sql parser. */
Review Comment:
"utility" begins with a vowel, it does not pronounce a vowel, but a consonant. Here is a funny website [a-or-an.com](https://www.a-or-an.com/a_an/utility), If you are not sure whether to use a or an, you can refer to it .
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998862068
##########
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:
done
--
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
[GitHub] [flink] zhuzhurk commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1281737718
Thanks for opening this PR! @zhoulii
Now the Flink master is ready to merge changes for Flink 1.17 and we can continue with this migration work.
Would you rebase the change to the lastest master?
--
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
[GitHub] [flink] zhuzhurk commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhuzhurk commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1284980784
Thanks for working on this! @zhoulii
And thanks for the thorough review! @snuyanzin
Merging.
--
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
[GitHub] [flink] zhuzhurk closed pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhuzhurk closed pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
URL: https://github.com/apache/flink/pull/20168
--
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
[GitHub] [flink] zhoulii commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1284809301
Hi @zhuzhurk, I have rebased the change to the latest master, the ci passed now.
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r968208180
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -265,8 +274,9 @@ public void testStreamQueryExecutionTable() throws Exception {
executeStreamQueryTable(replaceVars, configMap, query, expectedResults);
}
- @Test(timeout = 90_000L)
- public void testStreamQueryExecutionTableMultipleTimes() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -240,8 +248,9 @@ public void testStreamQueryExecutionChangelogMultipleTimes() throws Exception {
}
}
- @Test(timeout = 90_000L)
- public void testStreamQueryExecutionTable() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -191,8 +198,9 @@ public void testStreamQueryExecutionChangelog() throws Exception {
}
}
- @Test(timeout = 90_000L)
- public void testStreamQueryExecutionChangelogMultipleTimes() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -144,8 +150,9 @@ public void testCompleteStatement() {
executor.closeSession(sessionId);
}
- @Test(timeout = 90_000L)
- public void testStreamQueryExecutionChangelog() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r968207924
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -358,8 +370,9 @@ public void testBatchQueryExecution() throws Exception {
}
}
- @Test(timeout = 90_000L)
- public void testBatchQueryExecutionMultipleTimes() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient (code will be more readable) to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -312,8 +323,9 @@ public void testStreamQueryExecutionLimitedTable() throws Exception {
executeStreamQueryTable(replaceVars, configMap, query, expectedResults);
}
- @Test(timeout = 90_000L)
- public void testBatchQueryExecution() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -291,8 +301,9 @@ public void testStreamQueryExecutionTableMultipleTimes() throws Exception {
}
}
- @Test(timeout = 90_000L)
- public void testStreamQueryExecutionLimitedTable() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
Since JUnit 5 supports specification of timeunits it would be much more convenient to use them like
```java
@Timeout(value = 90, unit = TimeUnit.SECONDS)
```
or
```java
@Timeout(value = 90, unit = TimeUnit.MILLISECONDS)
```
--
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
[GitHub] [flink] XComp commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
XComp commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1281823045
Thanks for your contribution @zhoulii. Just a minor comment about the timeouts: Keep in mind that we actually want to remove timeouts from JUnit tests (see [coding guidelines](https://flink.apache.org/contributing/code-style-and-quality-common.html#avoid-timeouts-in-junit-tests)) to benefit from the thread dump printout generated by our CI tools. So it might be better to remove them instead of migrating them to JUnit5
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998861783
##########
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)
+public class CliClientITCase {
Review Comment:
done
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r998861628
##########
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:
done
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r968210053
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/utils/SqlParserHelper.java:
##########
@@ -26,7 +26,7 @@
import org.apache.flink.table.catalog.hive.HiveTestUtils;
import org.apache.flink.table.delegation.Parser;
-/** An utility class that provides pre-prepared tables and sql parser. */
+/** A utility class that provides pre-prepared tables and sql parser. */
Review Comment:
As as i know it should be `An` since it stays before the word starting with a vowel letter....
Or did I miss anything?
--
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
[GitHub] [flink] snuyanzin commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1243493165
Thanks for your contribution
I left some comments
I fear you need to rebase it since there are conflicts...
--
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
[GitHub] [flink] zhoulii commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on code in PR #20168:
URL: https://github.com/apache/flink/pull/20168#discussion_r997908191
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/local/LocalExecutorITCase.java:
##########
@@ -358,8 +370,9 @@ public void testBatchQueryExecution() throws Exception {
}
}
- @Test(timeout = 90_000L)
- public void testBatchQueryExecutionMultipleTimes() throws Exception {
+ @Test
+ @Timeout(90)
Review Comment:
As @XComp suggested:
> Keep in mind that we actually want to remove timeouts from JUnit tests (see [coding guidelines](https://flink.apache.org/contributing/code-style-and-quality-common.html#avoid-timeouts-in-junit-tests)) to benefit from the thread dump printout generated by our CI tools. So it might be better to remove them instead of migrating them to JUnit5
We'd better remove timeouts instead of migrating them to JUnit5.
--
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
[GitHub] [flink] zhoulii commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
zhoulii commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1283663775
@flinkbot run azure
--
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
[GitHub] [flink] snuyanzin commented on a diff in pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20168: [FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20168:
URL: https://github.com/apache/flink/pull/20168#issuecomment-1174931588
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b89034b434aae5ed9f3c1776491e946ef28faadd",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b89034b434aae5ed9f3c1776491e946ef28faadd",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b89034b434aae5ed9f3c1776491e946ef28faadd UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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