zentol commented on code in PR #18986: URL: https://github.com/apache/flink/pull/18986#discussion_r863668139
########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -173,10 +172,10 @@ public abstract class YarnTestBase extends TestLogger { }; // Temp directory which is deleted after the unit test. - @ClassRule public static TemporaryFolder tmp = new TemporaryFolder(); + @TempDir public static File tmp; Review Comment: ```suggestion @TempDir protected static File tmp; ``` ? ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -869,7 +856,7 @@ private static void setMiniDFSCluster(File targetTestClassesFolder) throws Excep } /** Default @BeforeClass impl. Overwrite this for passing a different configuration */ - @BeforeClass + @BeforeAll public static void setup() throws Exception { Review Comment: ```suggestion static void setup() throws Exception { ``` ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -276,7 +275,7 @@ public void setupYarnClient() { } /** Sleep a bit between the tests (we are re-using the YARN cluster for the tests). */ - @After + @AfterEach public void shutdownYarnClient() { Review Comment: ```suggestion void shutdownYarnClient() { ``` ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -264,7 +263,7 @@ public static void populateYarnSecureConfigurations( conf.set("hadoop.security.auth_to_local", "RULE:[1:$1] RULE:[2:$1]"); } - @Before + @BeforeEach public void setupYarnClient() { Review Comment: ```suggestion void setupYarnClient() { ``` ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java: ########## @@ -445,39 +443,45 @@ 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) + .satisfies( + new Condition<Throwable>() { + @Override + public boolean matches(Throwable value) { + return ExceptionUtils.findThrowableWithMessage( + value, "to unknown queue: doesntExist") + .isPresent(); + } + }); Review Comment: ```suggestion .satisfies(anyCauseMatches("to unknown queue: doesntExist")); ``` ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java: ########## @@ -445,39 +443,45 @@ 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) + .satisfies( + new Condition<Throwable>() { + @Override + public boolean matches(Throwable value) { + return ExceptionUtils.findThrowableWithMessage( + value, "to unknown queue: doesntExist") + .isPresent(); + } + }); + + assertThat(yarLoggerAuditingExtension.getMessages()) + .anyMatch( + s -> + s.contains( + "The specified queue 'doesntExist' does not exist. Available queues")); Review Comment: ```suggestion .anySatisfies( s -> assertThat(s).contains( "The specified queue 'doesntExist' does not exist. Available queues")); ``` This will give a better error message. ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNHighAvailabilityITCase.java: ########## @@ -154,16 +146,19 @@ 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 = 60 * 30) Review Comment: ```suggestion @Timeout(value = 30, unit = TimeUnit.MINUTES) ``` This is what I had in mind. ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -1176,7 +1164,7 @@ public Throwable getRunnerError() { // -------------------------- Tear down -------------------------- // - @AfterClass + @AfterAll public static void teardown() throws Exception { Review Comment: ```suggestion static void teardown() throws Exception { ``` ########## 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 + private final LoggerAuditingExtension cliLoggerAuditingExtension = + new LoggerAuditingExtension(CliFrontend.class, Level.INFO); - @Rule - public final TestLoggerResource yarTestLoggerResource = - new TestLoggerResource(YarnClusterDescriptor.class, Level.WARN); + @RegisterExtension + private final LoggerAuditingExtension yarLoggerAuditingExtension = + new LoggerAuditingExtension(YarnClusterDescriptor.class, Level.WARN); - @BeforeClass + @BeforeAll public static void setup() throws Exception { Review Comment: ```suggestion static void setup() throws Exception { ``` ########## flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java: ########## @@ -850,15 +839,13 @@ private static void start( } catch (Exception ex) { ex.printStackTrace(); LOG.error("setup failure", ex); - Assert.fail(); Review Comment: this needs to stay -- 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