ferenc-csaky commented on code in PR #22789:
URL: https://github.com/apache/flink/pull/22789#discussion_r1511222835
##########
flink-filesystems/flink-hadoop-fs/src/test/java/org/apache/flink/runtime/fs/hdfs/HadoopFreeTests.java:
##########
Review Comment:
Use AssertJ's `assertThatThrownBy` instead of try/catch and `fail()`.
##########
flink-filesystems/flink-hadoop-fs/src/test/java/org/apache/flink/runtime/fs/hdfs/HadoopFsFactoryTest.java:
##########
@@ -46,14 +45,14 @@ public void testCreateHadoopFsWithoutConfig() throws
Exception {
}
@Test
- public void testCreateHadoopFsWithMissingAuthority() throws Exception {
+ void testCreateHadoopFsWithMissingAuthority() throws Exception {
final URI uri = URI.create("hdfs:///my/path");
HadoopFsFactory factory = new HadoopFsFactory();
try {
factory.create(uri);
- fail("should have failed with an exception");
+ Assertions.fail("should have failed with an exception");
Review Comment:
Use AssertJ's `assertThatThrownBy` instead of try/catch.
##########
flink-filesystems/flink-s3-fs-presto/src/test/java/org/apache/flink/fs/s3presto/PrestoS3FileSystemITCase.java:
##########
@@ -25,19 +25,15 @@
import org.apache.flink.testutils.s3.S3TestCredentials;
import com.amazonaws.SdkClientException;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
import java.io.IOException;
-import java.util.Arrays;
-import java.util.List;
import java.util.UUID;
import static
com.facebook.presto.hive.s3.S3ConfigurationUpdater.S3_USE_INSTANCE_CREDENTIALS;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.fail;
Review Comment:
Use AssertJ's `assertThatThrownBy` instead of try/catch `fail()`.
##########
flink-core/src/test/java/org/apache/flink/core/fs/local/LocalFileSystemRecoverableWriterTest.java:
##########
@@ -22,17 +22,18 @@
import org.apache.flink.core.fs.FileSystem;
import org.apache.flink.core.fs.Path;
-import org.junit.ClassRule;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
/** Tests for the {@link LocalRecoverableWriter}. */
public class LocalFileSystemRecoverableWriterTest extends
AbstractRecoverableWriterTest {
- @ClassRule public static final TemporaryFolder TEMP_FOLDER = new
TemporaryFolder();
+ @TempDir protected static File tempFolder;
Review Comment:
Why not `private` instead of `protected`?
##########
flink-core/src/test/java/org/apache/flink/core/fs/AbstractRecoverableWriterTest.java:
##########
@@ -303,27 +304,23 @@ public void testCommitAfterRecovery() throws Exception {
// TESTS FOR EXCEPTIONS
- @Test(expected = IOException.class)
+ @Test
public void testExceptionWritingAfterCloseForCommit() throws Exception {
final Path testDir = getBasePathForTest();
final RecoverableWriter writer = getNewFileSystemWriter();
final Path path = new Path(testDir, "part-0");
- RecoverableFsDataOutputStream stream = null;
- try {
- stream = writer.open(path);
+ try (RecoverableFsDataOutputStream stream = writer.open(path)) {
stream.write(testData1.getBytes(StandardCharsets.UTF_8));
stream.closeForCommit().getRecoverable();
- stream.write(testData2.getBytes(StandardCharsets.UTF_8));
- fail();
- } finally {
Review Comment:
I wonder if it would be better to keep the `try`/`finally` here, as
try-with-resources do not support quiet close, so it might introduce some
flakiness.
##########
flink-core/src/test/java/org/apache/flink/core/fs/local/LocalFileSystemRecoverableWriterTest.java:
##########
Review Comment:
The class could be package-private I think.
##########
flink-filesystems/flink-azure-fs-hadoop/src/test/java/org/apache/flink/fs/azurefs/AzureBlobStorageFSFactoryTest.java:
##########
@@ -19,77 +19,63 @@
package org.apache.flink.fs.azurefs;
import org.apache.flink.configuration.Configuration;
-import org.apache.flink.util.TestLogger;
import org.apache.hadoop.fs.azure.AzureException;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExpectedException;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import java.net.URI;
-import java.util.Arrays;
-import java.util.List;
-/** Tests for the AzureFSFactory. */
-@RunWith(Parameterized.class)
-public class AzureBlobStorageFSFactoryTest extends TestLogger {
-
- @Parameterized.Parameter public String scheme;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
- @Parameterized.Parameters(name = "Scheme = {0}")
- public static List<String> parameters() {
- return Arrays.asList("wasb", "wasbs");
- }
-
- @Rule public final ExpectedException exception = ExpectedException.none();
+/** Tests for the AzureFSFactory. */
+class AzureBlobStorageFSFactoryTest {
private AbstractAzureFSFactory getFactory(String scheme) {
return scheme.equals("wasb")
? new AzureBlobStorageFSFactory()
: new SecureAzureBlobStorageFSFactory();
}
- @Test
- public void testNullFsURI() throws Exception {
+ @ParameterizedTest
+ @ValueSource(strings = {"wasb", "wasbs"})
Review Comment:
I think a custom test annotation fed by a `MethodSource` could be utilized
here to return the existing factories instead of passing the schemes to all
test cases and creating a factory every time. Something like this:
```java
@ParameterizedTest(name = "Factory = {0}")
@MethodSource("getFactories")
@Retention(RetentionPolicy.RUNTIME)
private @interface TestAllFsImpl {}
@SuppressWarnings("unused")
private static Stream<AbstractAzureFSFactory> getFactories() {
return Stream.of(
new AzureBlobStorageFSFactory(),
new SecureAzureBlobStorageFSFactory());
}
@TestAllFsImpl
void testNullFsURI(AbstractAzureFSFactory factory) throws Exception {
...
}
```
And the same could be applied to `AzureDataLakeStoreGen2FSFactoryTest`.
WDYT?
--
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
