Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
Jiabao-Sun merged PR #24612: URL: https://github.com/apache/flink/pull/24612 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565637869
##
flink-core/src/test/java/org/apache/flink/configuration/ReadableWritableConfigurationTest.java:
##
@@ -169,7 +170,7 @@ void testGetOptionalFromObject() {
testSpec.setValue(configuration);
Optional optional = configuration.getOptional(testSpec.getOption());
-assertThat(optional.get(), equalTo(testSpec.getValue()));
+assertThat(optional.get()).isEqualTo(testSpec.getValue());
Review Comment:
Compile error with suggestion
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565642234
##
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##
@@ -42,57 +42,48 @@ class GlobalConfigurationTest {
@TempDir private File tmpDir;
@Test
-void testConfigurationWithLegacyYAML() {
+void testConfigurationWithLegacyYAML() throws FileNotFoundException {
File confFile = new File(tmpDir,
GlobalConfiguration.LEGACY_FLINK_CONF_FILENAME);
-
-try {
-try (final PrintWriter pw = new PrintWriter(confFile)) {
-
-pw.println("###"); // should be skipped
-pw.println("# Some : comments : to skip"); // should be skipped
-pw.println("###"); // should be skipped
-pw.println("mykey1: myvalue1"); // OK, simple correct case
-pw.println("mykey2 : myvalue2"); // OK, whitespace
before colon is correct
-pw.println("mykey3:myvalue3"); // SKIP, missing white space
after colon
-pw.println(" some nonsense without colon and whitespace
separator"); // SKIP
-pw.println(" : "); // SKIP
-pw.println(" "); // SKIP (silently)
-pw.println(" "); // SKIP (silently)
-pw.println("mykey4: myvalue4# some comments"); // OK, skip
comments only
-pw.println(" mykey5:myvalue5"); // OK, trim
unnecessary whitespace
-pw.println("mykey6: my: value6"); // OK, only use first ': '
as separator
-pw.println("mykey7: "); // SKIP, no value provided
-pw.println(": myvalue8"); // SKIP, no key provided
-
-pw.println("mykey9: myvalue9"); // OK
-pw.println("mykey9: myvalue10"); // OK, overwrite last value
-
-} catch (FileNotFoundException e) {
-e.printStackTrace();
-}
-
-Configuration conf =
GlobalConfiguration.loadConfiguration(tmpDir.getAbsolutePath());
-
-// all distinct keys from confFile1 + confFile2 key
-assertThat(conf.keySet()).hasSize(6);
-
-// keys 1, 2, 4, 5, 6, 7, 8 should be OK and match the expected
values
-assertThat(conf.getString("mykey1", null)).isEqualTo("myvalue1");
-assertThat(conf.getString("mykey1", null)).isEqualTo("myvalue1");
-assertThat(conf.getString("mykey2", null)).isEqualTo("myvalue2");
-assertThat(conf.getString("mykey3", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey4", null)).isEqualTo("myvalue4");
-assertThat(conf.getString("mykey5", null)).isEqualTo("myvalue5");
-assertThat(conf.getString("mykey6", null)).isEqualTo("my: value6");
-assertThat(conf.getString("mykey7", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey8", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey9", null)).isEqualTo("myvalue10");
-} finally {
-// Clear the standard yaml flag to avoid impact to other cases.
-GlobalConfiguration.setStandardYaml(true);
-confFile.delete();
-tmpDir.delete();
+try (PrintWriter pw = new PrintWriter(confFile)) {
+pw.println("###"); // should be skipped
+pw.println("# Some : comments : to skip"); // should be skipped
+pw.println("###"); // should be skipped
+pw.println("mykey1: myvalue1"); // OK, simple correct case
+pw.println("mykey2 : myvalue2"); // OK, whitespace before
colon is correct
+pw.println("mykey3:myvalue3"); // SKIP, missing white space after
colon
+pw.println(" some nonsense without colon and whitespace
separator"); // SKIP
+pw.println(" : "); // SKIP
+pw.println(" "); // SKIP (silently)
+pw.println(" "); // SKIP (silently)
+pw.println("mykey4: myvalue4# some comments"); // OK, skip
comments only
+pw.println(" mykey5:myvalue5"); // OK, trim
unnecessary whitespace
+pw.println("mykey6: my: value6"); // OK, only use first ': ' as
separator
+pw.println("mykey7: "); // SKIP, no value provided
+pw.println(": myvalue8"); // SKIP, no key provided
+
+pw.println("mykey9: myvalue9"); // OK
+pw.println("mykey9: myvalue10"); // OK, overwrite last value
}
+Configuration conf =
GlobalConfiguration.loadConfiguration(tmpDir.getAbsolutePath());
+
+// all distinct keys from confFile1 + confFile2 key
+assertThat(conf.keySet()).hasSize(6);
+
+// keys 1, 2, 4, 5, 6, 7, 8 should be OK and match the expected values
+assertThat(conf.getString("mykey1",
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565637869
##
flink-core/src/test/java/org/apache/flink/configuration/ReadableWritableConfigurationTest.java:
##
@@ -169,7 +170,7 @@ void testGetOptionalFromObject() {
testSpec.setValue(configuration);
Optional optional = configuration.getOptional(testSpec.getOption());
-assertThat(optional.get(), equalTo(testSpec.getValue()));
+assertThat(optional.get()).isEqualTo(testSpec.getValue());
Review Comment:
There is no need to clean after test completed.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
Jiabao-Sun commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565548513
##
flink-core/src/test/java/org/apache/flink/configuration/MemorySizePrettyPrintingTest.java:
##
@@ -45,13 +46,13 @@ public static Object[][] parameters() {
};
}
-@Parameterized.Parameter public MemorySize memorySize;
+@Parameter private MemorySize memorySize;
-@Parameterized.Parameter(1)
+@Parameter(value = 1)
Review Comment:
value can be ignored and expectedString can be private.
##
flink-core/src/test/java/org/apache/flink/configuration/ReadableWritableConfigurationTest.java:
##
@@ -169,7 +170,7 @@ void testGetOptionalFromObject() {
testSpec.setValue(configuration);
Optional optional = configuration.getOptional(testSpec.getOption());
-assertThat(optional.get(), equalTo(testSpec.getValue()));
+assertThat(optional.get()).isEqualTo(testSpec.getValue());
Review Comment:
```suggestion
assertThat(optional).hasValue(testSpec.getValue());
```
##
flink-core/src/test/java/org/apache/flink/configuration/GlobalConfigurationTest.java:
##
@@ -42,57 +42,48 @@ class GlobalConfigurationTest {
@TempDir private File tmpDir;
@Test
-void testConfigurationWithLegacyYAML() {
+void testConfigurationWithLegacyYAML() throws FileNotFoundException {
File confFile = new File(tmpDir,
GlobalConfiguration.LEGACY_FLINK_CONF_FILENAME);
-
-try {
-try (final PrintWriter pw = new PrintWriter(confFile)) {
-
-pw.println("###"); // should be skipped
-pw.println("# Some : comments : to skip"); // should be skipped
-pw.println("###"); // should be skipped
-pw.println("mykey1: myvalue1"); // OK, simple correct case
-pw.println("mykey2 : myvalue2"); // OK, whitespace
before colon is correct
-pw.println("mykey3:myvalue3"); // SKIP, missing white space
after colon
-pw.println(" some nonsense without colon and whitespace
separator"); // SKIP
-pw.println(" : "); // SKIP
-pw.println(" "); // SKIP (silently)
-pw.println(" "); // SKIP (silently)
-pw.println("mykey4: myvalue4# some comments"); // OK, skip
comments only
-pw.println(" mykey5:myvalue5"); // OK, trim
unnecessary whitespace
-pw.println("mykey6: my: value6"); // OK, only use first ': '
as separator
-pw.println("mykey7: "); // SKIP, no value provided
-pw.println(": myvalue8"); // SKIP, no key provided
-
-pw.println("mykey9: myvalue9"); // OK
-pw.println("mykey9: myvalue10"); // OK, overwrite last value
-
-} catch (FileNotFoundException e) {
-e.printStackTrace();
-}
-
-Configuration conf =
GlobalConfiguration.loadConfiguration(tmpDir.getAbsolutePath());
-
-// all distinct keys from confFile1 + confFile2 key
-assertThat(conf.keySet()).hasSize(6);
-
-// keys 1, 2, 4, 5, 6, 7, 8 should be OK and match the expected
values
-assertThat(conf.getString("mykey1", null)).isEqualTo("myvalue1");
-assertThat(conf.getString("mykey1", null)).isEqualTo("myvalue1");
-assertThat(conf.getString("mykey2", null)).isEqualTo("myvalue2");
-assertThat(conf.getString("mykey3", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey4", null)).isEqualTo("myvalue4");
-assertThat(conf.getString("mykey5", null)).isEqualTo("myvalue5");
-assertThat(conf.getString("mykey6", null)).isEqualTo("my: value6");
-assertThat(conf.getString("mykey7", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey8", "null")).isEqualTo("null");
-assertThat(conf.getString("mykey9", null)).isEqualTo("myvalue10");
-} finally {
-// Clear the standard yaml flag to avoid impact to other cases.
-GlobalConfiguration.setStandardYaml(true);
-confFile.delete();
-tmpDir.delete();
+try (PrintWriter pw = new PrintWriter(confFile)) {
+pw.println("###"); // should be skipped
+pw.println("# Some : comments : to skip"); // should be skipped
+pw.println("###"); // should be skipped
+pw.println("mykey1: myvalue1"); // OK, simple correct case
+pw.println("mykey2 : myvalue2"); // OK, whitespace before
colon is correct
+pw.println("mykey3:myvalue3"); // SKIP, missing white space after
colon
+pw.println(" some nonsense without colon and whitespace
separator"); // SKIP
+pw.println(" : ");
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
Jiabao-Sun commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565204190
##
flink-core/src/test/java/org/apache/flink/configuration/ConfigurationConversionsTest.java:
##
@@ -52,7 +51,9 @@ public class ConfigurationConversionsTest {
private Configuration pc;
-@Before
+@Parameter private TestSpec testSpec;
+
+@BeforeEach
public void init() {
Review Comment:
The visibility of class and methods can be package default.
##
flink-core/src/test/java/org/apache/flink/configuration/FilesystemSchemeConfigTest.java:
##
@@ -21,83 +21,75 @@
import org.apache.flink.core.fs.FileSystem;
import org.apache.flink.core.fs.UnsupportedFileSystemSchemeException;
import org.apache.flink.core.fs.local.LocalFileSystem;
-import org.apache.flink.util.TestLogger;
-import org.junit.After;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
-import java.io.IOException;
+import java.io.File;
import java.net.URI;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
/** Tests for the configuration of the default file system scheme. */
-public class FilesystemSchemeConfigTest extends TestLogger {
+public class FilesystemSchemeConfigTest {
-@Rule public final TemporaryFolder tempFolder = new TemporaryFolder();
+@TempDir public File tempFolder;
Review Comment:
private
##
flink-core/src/test/java/org/apache/flink/configuration/ConfigurationParsingInvalidFormatsTest.java:
##
@@ -64,35 +66,35 @@ public static Object[][] getSpecs() {
};
}
-@Parameterized.Parameter public ConfigOption option;
+@Parameter public ConfigOption option;
Review Comment:
visibility can be 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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
1996fanrui commented on code in PR #24612:
URL: https://github.com/apache/flink/pull/24612#discussion_r1565149231
##
flink-core/src/test/java/org/apache/flink/configuration/FilesystemSchemeConfigTest.java:
##
@@ -21,83 +21,79 @@
import org.apache.flink.core.fs.FileSystem;
import org.apache.flink.core.fs.UnsupportedFileSystemSchemeException;
import org.apache.flink.core.fs.local.LocalFileSystem;
-import org.apache.flink.util.TestLogger;
-import org.junit.After;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import java.io.File;
import java.io.IOException;
import java.net.URI;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
/** Tests for the configuration of the default file system scheme. */
-public class FilesystemSchemeConfigTest extends TestLogger {
+public class FilesystemSchemeConfigTest {
-@Rule public final TemporaryFolder tempFolder = new TemporaryFolder();
+@TempDir public File tempFolder;
-@After
-public void clearFsSettings() throws IOException {
+@AfterEach
+void clearFsSettings() throws IOException {
FileSystem.initialize(new Configuration());
}
//
@Test
-public void testDefaultsToLocal() throws Exception {
-URI justPath = new URI(tempFolder.newFile().toURI().getPath());
-assertNull(justPath.getScheme());
+void testDefaultsToLocal() throws Exception {
+URI justPath = new URI(File.createTempFile("junit", null,
tempFolder).toURI().getPath());
+assertThat(justPath.getScheme()).isNull();
FileSystem fs = FileSystem.get(justPath);
-assertEquals("file", fs.getUri().getScheme());
+assertThat(fs.getUri().getScheme()).isEqualTo("file");
}
@Test
-public void testExplicitlySetToLocal() throws Exception {
+void testExplicitlySetToLocal() throws Exception {
final Configuration conf = new Configuration();
conf.set(CoreOptions.DEFAULT_FILESYSTEM_SCHEME,
LocalFileSystem.getLocalFsURI().toString());
FileSystem.initialize(conf);
-URI justPath = new URI(tempFolder.newFile().toURI().getPath());
-assertNull(justPath.getScheme());
+URI justPath = new URI(File.createTempFile("junit", null,
tempFolder).toURI().getPath());
+assertThat(justPath.getScheme()).isNull();
FileSystem fs = FileSystem.get(justPath);
-assertEquals("file", fs.getUri().getScheme());
+assertThat(fs.getUri().getScheme()).isEqualTo("file");
}
@Test
-public void testExplicitlySetToOther() throws Exception {
+void testExplicitlySetToOther() throws Exception {
final Configuration conf = new Configuration();
conf.set(CoreOptions.DEFAULT_FILESYSTEM_SCHEME,
"otherFS://localhost:1234/");
FileSystem.initialize(conf);
-URI justPath = new URI(tempFolder.newFile().toURI().getPath());
-assertNull(justPath.getScheme());
+URI justPath = new URI(File.createTempFile("junit", null,
tempFolder).toURI().getPath());
+assertThat(justPath.getScheme()).isNull();
try {
FileSystem.get(justPath);
fail("should have failed with an exception");
} catch (UnsupportedFileSystemSchemeException e) {
-assertTrue(e.getMessage().contains("otherFS"));
+assertThat(e.getMessage()).contains("otherFS");
Review Comment:
It's better to use `assertThatThrownBy` instead.
##
flink-core/src/test/java/org/apache/flink/configuration/MemorySizeTest.java:
##
@@ -20,243 +20,232 @@
import org.apache.flink.core.testutils.CommonTestUtils;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
import java.io.IOException;
import static org.apache.flink.configuration.MemorySize.MemoryUnit.MEGA_BYTES;
-import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static
org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
/** Tests for the {@link MemorySize} class. */
-public class MemorySizeTest {
+class MemorySizeTest {
@Test
-public void testUnitConversion() {
+void testUnitConversion() {
final MemorySize zero =
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 commented on PR #24612: URL: https://github.com/apache/flink/pull/24612#issuecomment-2044061547 @Jiabao-Sun @1996fanrui PTAL -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
flinkbot commented on PR #24612: URL: https://github.com/apache/flink/pull/24612#issuecomment-2036688897 ## CI report: * 94ff81023ed7f0fe9e9a2afd45ba73bd0006c134 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 commented on PR #24612: URL: https://github.com/apache/flink/pull/24612#issuecomment-2033951840 @Jiabao-Sun PTAL -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-25537][JUnit5 Migration] Module: flink-core with,Package: Configuration [flink]
GOODBOY008 opened a new pull request, #24612: URL: https://github.com/apache/flink/pull/24612 Changes: - Module: flink-core with,Package: Configuration migrate 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
