zentol commented on code in PR #18986:
URL: https://github.com/apache/flink/pull/18986#discussion_r862659744


##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/CliFrontendRunWithYarnTest.java:
##########
@@ -43,22 +44,20 @@
  *
  * @see org.apache.flink.client.cli.CliFrontendRunTest
  */
-public class CliFrontendRunWithYarnTest extends CliFrontendTestBase {
-
-    @Rule public TemporaryFolder tmp = new TemporaryFolder();
+class CliFrontendRunWithYarnTest extends CliFrontendTestBase {

Review Comment:
   We should be able to remove this inheritance completely since we use none of 
the inherited methods.
   



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/UtilsTest.java:
##########
@@ -55,36 +52,36 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.Assertions.assertThat;
 
 /** Tests for various utilities. */
-public class UtilsTest extends TestLogger {
+class UtilsTest {
     private static final Logger LOG = LoggerFactory.getLogger(UtilsTest.class);
 
-    @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    @TempDir File temporaryFolder;
 
     @Test
-    public void testUberjarLocator() {
+    void testUberjarLocator() {
         File dir = TestUtils.findFile("..", new 
TestUtils.RootDirFilenameFilter());
-        Assert.assertNotNull(dir);
-        Assert.assertTrue(dir.getName().endsWith(".jar"));
+        assertThat(dir).isNotNull();
+        assertThat(dir.getName()).endsWith(".jar");
         dir = dir.getParentFile().getParentFile(); // from uberjar to lib to 
root
-        Assert.assertTrue(dir.exists());
-        Assert.assertTrue(dir.isDirectory());
-        List<String> files = Arrays.asList(dir.list());
-        Assert.assertTrue(files.contains("lib"));
-        Assert.assertTrue(files.contains("bin"));
-        Assert.assertTrue(files.contains("conf"));
+        assertThat(dir.exists()).isTrue();
+        assertThat(dir.isDirectory()).isTrue();
+        List<String> files = Arrays.asList(Objects.requireNonNull(dir.list()));
+        assertThat(files).contains("lib");
+        assertThat(files).contains("bin");
+        assertThat(files).contains("conf");

Review Comment:
   ```suggestion
           assertThat(dir.list()).contains("lib", "bin", "conf");
   ```



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java:
##########
@@ -445,39 +444,51 @@ private static Map<String, String> getFlinkConfig(final 
String host, final int p
      * With an error message, we can help users identifying the issue)
      */
     @Test
-    public void testNonexistingQueueWARNmessage() throws Exception {
+    void testNonexistingQueueWARNmessage() throws Exception {
         runTest(
                 () -> {
                     LOG.info("Starting testNonexistingQueueWARNmessage()");
-                    try {
-                        runWithArgs(
-                                new String[] {
-                                    "-j",
-                                    flinkUberjar.getAbsolutePath(),
-                                    "-t",
-                                    flinkLibFolder.getAbsolutePath(),
-                                    "-jm",
-                                    "768m",
-                                    "-tm",
-                                    "1024m",
-                                    "-qu",
-                                    "doesntExist"
-                                },
-                                "to unknown queue: doesntExist",
-                                null,
-                                RunTypes.YARN_SESSION,
-                                1);
-                    } catch (Exception e) {
-                        assertTrue(
-                                ExceptionUtils.findThrowableWithMessage(
-                                                e, "to unknown queue: 
doesntExist")
-                                        .isPresent());
-                    }
-                    assertThat(
-                            yarTestLoggerResource.getMessages(),
-                            hasItem(
-                                    containsString(
-                                            "The specified queue 'doesntExist' 
does not exist. Available queues")));
+                    assertThatThrownBy(
+                                    () ->
+                                            runWithArgs(
+                                                    new String[] {
+                                                        "-j",
+                                                        
flinkUberjar.getAbsolutePath(),
+                                                        "-t",
+                                                        
flinkLibFolder.getAbsolutePath(),
+                                                        "-jm",
+                                                        "768m",
+                                                        "-tm",
+                                                        "1024m",
+                                                        "-qu",
+                                                        "doesntExist"
+                                                    },
+                                                    "to unknown queue: 
doesntExist",
+                                                    null,
+                                                    RunTypes.YARN_SESSION,
+                                                    1))
+                            .isInstanceOf(Exception.class)
+                            .getRootCause()
+                            .hasMessageContaining("to unknown queue: 
doesntExist");
+
+                    assertThat(yarLoggerAuditingExtension.getMessages())
+                            .satisfies(
+                                    new Condition<Object>() {
+                                        @Override
+                                        public boolean matches(Object strList) 
{
+                                            return !Objects.isNull(strList)
+                                                    && ((List<String>) strList)
+                                                            .stream()
+                                                                    .anyMatch(
+                                                                            s 
->
+                                                                               
     !Objects.isNull(
+                                                                               
                     s)
+                                                                               
             && s
+                                                                               
                     .contains(
+                                                                               
                             "The specified queue 'doesntExist' does not exist. 
Available queues"));
+                                        }
+                                    })
+                            .hasSizeGreaterThan(0);

Review Comment:
   ```suggestion
                               .anyMatch(s -> assertThat(s).contains("The 
specified queue 'doesntExist' does not exist. Available queues"));
   ```



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionFIFOSecuredITCase.java:
##########
@@ -156,9 +154,10 @@ public void testDetachedModeSecureWithPreInstallKeytab() 
throws Exception {
                 });
     }
 
-    @Test(timeout = 60000) // timeout after a minute.
+    @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
+    @Test // timeout after a minute.

Review Comment:
   remove comment



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java:
##########
@@ -759,30 +755,24 @@ private static void start(
             YarnConfiguration conf, String principal, String keytab, boolean 
withDFS) {
         // set the home directory to a temp directory. Flink on YARN is using 
the home dir to
         // distribute the file
-        File homeDir = null;
-        try {
-            homeDir = tmp.newFolder();
-        } catch (IOException e) {
-            e.printStackTrace();
-            Assert.fail(e.getMessage());
-        }
+        File homeDir = tmp;
         System.setProperty("user.home", homeDir.getAbsolutePath());
         String uberjarStartLoc = "..";
-        LOG.info("Trying to locate uberjar in {}", new 
File(uberjarStartLoc).getAbsolutePath());
+        log.info("Trying to locate uberjar in {}", new 
File(uberjarStartLoc).getAbsolutePath());
         flinkUberjar = TestUtils.findFile(uberjarStartLoc, new 
TestUtils.RootDirFilenameFilter());
-        Assert.assertNotNull("Flink uberjar not found", flinkUberjar);
+        assertThat(flinkUberjar).isNotNull();
         String flinkDistRootDir = flinkUberjar.getParentFile().getParent();
         flinkLibFolder = flinkUberjar.getParentFile(); // the uberjar is 
located in lib/
-        Assert.assertNotNull("Flink flinkLibFolder not found", flinkLibFolder);
-        Assert.assertTrue("lib folder not found", flinkLibFolder.exists());
-        Assert.assertTrue("lib folder not found", 
flinkLibFolder.isDirectory());
+        assertThat(flinkLibFolder).isNotNull();

Review Comment:
   shuld be unnecessary



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNHighAvailabilityITCase.java:
##########
@@ -90,19 +92,13 @@
 import java.util.stream.Collectors;
 
 import static org.apache.flink.util.Preconditions.checkState;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.empty;
-import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.notNullValue;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assume.assumeTrue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 /** Tests that verify correct HA behavior. */
-public class YARNHighAvailabilityITCase extends YarnTestBase {
+class YARNHighAvailabilityITCase extends YarnTestBase {
 
-    @ClassRule public static final TemporaryFolder FOLDER = new 
TemporaryFolder();
+    private static final Logger log = 
LoggerFactory.getLogger(YARNHighAvailabilityITCase.class);

Review Comment:
   ```suggestion
       private static final Logger LOG = 
LoggerFactory.getLogger(YARNHighAvailabilityITCase.class);
   ```



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java:
##########
@@ -445,39 +444,51 @@ private static Map<String, String> getFlinkConfig(final 
String host, final int p
      * With an error message, we can help users identifying the issue)
      */
     @Test
-    public void testNonexistingQueueWARNmessage() throws Exception {
+    void testNonexistingQueueWARNmessage() throws Exception {
         runTest(
                 () -> {
                     LOG.info("Starting testNonexistingQueueWARNmessage()");
-                    try {
-                        runWithArgs(
-                                new String[] {
-                                    "-j",
-                                    flinkUberjar.getAbsolutePath(),
-                                    "-t",
-                                    flinkLibFolder.getAbsolutePath(),
-                                    "-jm",
-                                    "768m",
-                                    "-tm",
-                                    "1024m",
-                                    "-qu",
-                                    "doesntExist"
-                                },
-                                "to unknown queue: doesntExist",
-                                null,
-                                RunTypes.YARN_SESSION,
-                                1);
-                    } catch (Exception e) {
-                        assertTrue(
-                                ExceptionUtils.findThrowableWithMessage(
-                                                e, "to unknown queue: 
doesntExist")
-                                        .isPresent());
-                    }
-                    assertThat(
-                            yarTestLoggerResource.getMessages(),
-                            hasItem(
-                                    containsString(
-                                            "The specified queue 'doesntExist' 
does not exist. Available queues")));
+                    assertThatThrownBy(
+                                    () ->
+                                            runWithArgs(
+                                                    new String[] {
+                                                        "-j",
+                                                        
flinkUberjar.getAbsolutePath(),
+                                                        "-t",
+                                                        
flinkLibFolder.getAbsolutePath(),
+                                                        "-jm",
+                                                        "768m",
+                                                        "-tm",
+                                                        "1024m",
+                                                        "-qu",
+                                                        "doesntExist"
+                                                    },
+                                                    "to unknown queue: 
doesntExist",
+                                                    null,
+                                                    RunTypes.YARN_SESSION,
+                                                    1))
+                            .isInstanceOf(Exception.class)
+                            .getRootCause()
+                            .hasMessageContaining("to unknown queue: 
doesntExist");

Review Comment:
   this is a bit different. Previously we accepted the exception at any place 
in the cause chain. Could use satifies(FlinkAssertions...) instead



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnConfigurationITCase.java:
##########
@@ -199,23 +196,23 @@ public void testFlinkContainerMemory() throws Exception {
                             // system page size or jvm
                             // implementation therefore we use 15% threshold 
here.
                             assertThat(
-                                    (double)
-                                                    taskManagerInfo
-                                                            
.getHardwareDescription()
-                                                            .getSizeOfJvmHeap()
-                                            / (double) expectedHeapSizeBytes,
-                                    is(closeTo(1.0, 0.15)));
+                                            (double)
+                                                            taskManagerInfo
+                                                                    
.getHardwareDescription()
+                                                                    
.getSizeOfJvmHeap()
+                                                    / (double) 
expectedHeapSizeBytes)
+                                    .isCloseTo(1.0, 
Percentage.withPercentage(15));

Review Comment:
   ```suggestion
                                       .isCloseTo(1.0, Offset.offset(0.15));
   ```



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java:
##########
@@ -1070,25 +1059,18 @@ protected void runWithArgs(
             // this lets the test fail.
             throw new RuntimeException("Runner failed", 
runner.getRunnerError());
         }
-        Assert.assertTrue(
-                "During the timeout period of "
-                        + startTimeoutSeconds
-                        + " seconds the "
-                        + "expected string \""
-                        + terminateAfterString
-                        + "\" did not show up.",

Review Comment:
   I think this message is still useful and shouldn't be lost.



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java:
##########
@@ -108,8 +106,9 @@
  *
  * <p>The test is not thread-safe. Parallel execution of tests is not possible!
  */
-public abstract class YarnTestBase extends TestLogger {
-    private static final Logger LOG = 
LoggerFactory.getLogger(YarnTestBase.class);
+public abstract class YarnTestBase {
+
+    private static final Logger log = 
LoggerFactory.getLogger(YarnTestBase.class);

Review Comment:
   ```suggestion
       private static final Logger LOG = 
LoggerFactory.getLogger(YarnTestBase.class);
   ```



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java:
##########
@@ -473,11 +472,8 @@ private static void writeHDFSSiteConfigXML(Configuration 
coreSite, File targetFo
     public static void ensureNoProhibitedStringInLogFiles(
             final String[] prohibited, final Pattern[] whitelisted) {
         File cwd = new File("target/" + 
YARN_CONFIGURATION.get(TEST_CLUSTER_NAME_KEY));
-        Assert.assertTrue(
-                "Expecting directory " + cwd.getAbsolutePath() + " to exist", 
cwd.exists());
-        Assert.assertTrue(
-                "Expecting directory " + cwd.getAbsolutePath() + " to be a 
directory",
-                cwd.isDirectory());
+        assertThat(cwd).exists();
+        assertThat(cwd).isDirectory();

Review Comment:
   Oh wow there actually are file-specific assertions. We could use these also 
in the other tests bit more, no?



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionFIFOSecuredITCase.java:
##########
@@ -156,9 +154,10 @@ public void testDetachedModeSecureWithPreInstallKeytab() 
throws Exception {
                 });
     }
 
-    @Test(timeout = 60000) // timeout after a minute.
+    @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)

Review Comment:
   can be simplified by using a different timeunit



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNHighAvailabilityITCase.java:
##########
@@ -154,16 +146,17 @@ private void initJobGraph() throws IOException {
      * Tests that Yarn will restart a killed {@link 
YarnSessionClusterEntrypoint} which will then
      * resume a persisted {@link JobGraph}.
      */
-    @Test(timeout = 1_000 * 60 * 30)
-    public void testKillYarnSessionClusterEntrypoint() throws Exception {
+    @Timeout(value = 1_000 * 60 * 30, unit = TimeUnit.MILLISECONDS)
+    @Test
+    void testKillYarnSessionClusterEntrypoint() throws Exception {
         runTest(
                 () -> {
                     assumeTrue(

Review Comment:
   could use assertj assumeThat



##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java:
##########
@@ -111,15 +108,15 @@ public class YARNSessionCapacitySchedulerITCase extends 
YarnTestBase {
     /** Toggles checking for prohibited strings in logs after the test has 
run. */
     private boolean checkForProhibitedLogContents = true;
 
-    @Rule
-    public final TestLoggerResource cliTestLoggerResource =
-            new TestLoggerResource(CliFrontend.class, Level.INFO);
+    @RegisterExtension
+    final LoggerAuditingExtension cliLoggerAuditingExtension =

Review Comment:
   extensions 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

Reply via email to