XComp commented on code in PR #20267: URL: https://github.com/apache/flink/pull/20267#discussion_r989152235
########## flink-core/src/test/java/org/apache/flink/core/fs/local/LocalFileSystemBehaviorTest.java: ########## @@ -23,26 +23,25 @@ import org.apache.flink.core.fs.FileSystemKind; import org.apache.flink.core.fs.Path; -import org.junit.Rule; -import org.junit.rules.TemporaryFolder; +import org.junit.jupiter.api.io.TempDir; /** Behavior tests for Flink's {@link LocalFileSystem}. */ -public class LocalFileSystemBehaviorTest extends FileSystemBehaviorTestSuite { +class LocalFileSystemBehaviorTest extends FileSystemBehaviorTestSuite { - @Rule public final TemporaryFolder tmp = new TemporaryFolder(); + @TempDir private java.nio.file.Path tmp; Review Comment: Using `File` here would prevent you from using a full reference of the type here ########## flink-filesystems/flink-azure-fs-hadoop/src/test/java/org/apache/flink/fs/azurefs/AzureFileSystemBehaviorITCase.java: ########## @@ -70,12 +60,30 @@ public class AzureFileSystemBehaviorITCase extends FileSystemBehaviorTestSuite { private static final String TEST_DATA_DIR = "tests-" + UUID.randomUUID(); - // Azure Blob Storage defaults to https only storage accounts. We check if http support has been - // enabled on a best effort basis and test http if so. - @Parameterized.Parameters(name = "Scheme = {0}") - public static List<String> parameters() throws IOException { - boolean httpsOnly = isHttpsTrafficOnly(); - return httpsOnly ? Arrays.asList("wasbs") : Arrays.asList("wasb", "wasbs"); + /** + * Azure Blob Storage defaults to https only storage accounts, tested in the base class. + * + * <p>This nested class repeats the tests with http support, but only if a best effort check on + * https support succeeds. + */ + static class HttpSupportAzureFileSystemBehaviorITCase extends AzureFileSystemBehaviorITCase { Review Comment: hm, I'm wondering whether it should be a separate test class instead of an inner one. But I don't have a strong opinion here. :thinking: So, I guess, it's good enough :shrug: ########## flink-filesystems/flink-azure-fs-hadoop/src/test/java/org/apache/flink/fs/azurefs/AzureFileSystemBehaviorITCase.java: ########## @@ -85,9 +93,9 @@ private static boolean isHttpsTrafficOnly() throws IOException { return true; } - Assume.assumeTrue( - "Azure storage account not configured, skipping test...", - !StringUtils.isNullOrWhitespaceOnly(ACCOUNT)); + assumeThat(ACCOUNT) + .describedAs("Azure storage account not configured, skipping test...") + .isNotBlank(); Review Comment: Could we clean it up a bit. I wouldn't expected `assumeThat` to be called within `isHttpsTrafficOnly`. ########## flink-core/src/test/java/org/apache/flink/core/fs/FileSystemBehaviorTestSuite.java: ########## @@ -82,141 +80,142 @@ public void cleanup() throws Exception { // --- file system kind @Test - public void testFileSystemKind() { - assertEquals(getFileSystemKind(), fs.getKind()); + void testFileSystemKind() { + assertThat(fs.getKind()).isEqualTo(getFileSystemKind()); } // --- access and scheme @Test - public void testPathAndScheme() throws Exception { - assertEquals(fs.getUri(), getBasePath().getFileSystem().getUri()); - assertEquals(fs.getUri().getScheme(), getBasePath().toUri().getScheme()); + void testPathAndScheme() throws Exception { + assertThat(fs.getUri()).isEqualTo(getBasePath().getFileSystem().getUri()); + assertThat(fs.getUri().getScheme()).isEqualTo(getBasePath().toUri().getScheme()); } @Test - public void testHomeAndWorkDir() { - assertEquals(fs.getUri().getScheme(), fs.getWorkingDirectory().toUri().getScheme()); - assertEquals(fs.getUri().getScheme(), fs.getHomeDirectory().toUri().getScheme()); + void testHomeAndWorkDir() { + assertThat(fs.getUri().getScheme()) + .isEqualTo(fs.getWorkingDirectory().toUri().getScheme()) + .isEqualTo(fs.getHomeDirectory().toUri().getScheme()); Review Comment: ```suggestion @Test void testHomeDirScheme() { assertThat(fs.getHomeDirectory().toUri().getScheme()).isEqualTo(fs.getUri().getScheme()); } @Test void testWorkDirScheme() { assertThat(fs.getWorkingDirectory().toUri().getScheme()).isEqualTo(fs.toUri().getScheme()); } ``` nit: `actual` and `expected` are swapped here, I guess (we're actually testing the working directory and home directory). I was wondering whether it make sense to split these two up into two separate test cases. Additionally, we could add better naming. I know that this is nitpicking. I'll leave it up to you. Splitting the test up into two should be done in a separate hotfix commit, though. ########## flink-filesystems/flink-hadoop-fs/src/test/java/org/apache/flink/runtime/fs/hdfs/HdfsBehaviorTest.java: ########## @@ -47,19 +43,17 @@ public class HdfsBehaviorTest extends FileSystemBehaviorTestSuite { // ------------------------------------------------------------------------ - @BeforeClass - public static void verifyOS() { - Assume.assumeTrue( - "HDFS cluster cannot be started on Windows without extensions.", - !OperatingSystem.isWindows()); + @BeforeAll + static void verifyOS() { + assumeThat(OperatingSystem.isWindows()) + .describedAs("HDFS cluster cannot be started on Windows without extensions.") + .isFalse(); } - @BeforeClass - public static void createHDFS() throws Exception { - final File baseDir = TMP.newFolder(); - + @BeforeAll + static void createHDFS(@TempDir java.nio.file.Path tmp) throws Exception { Review Comment: You could use `File` instead of `java.nio.file.Path`. This way, you avoid using a qualified type here and there's no need to call `toString()` further down in line 56 because there's `tmp.getAbsolutePath()`. -- 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