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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
