You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/02 08:23:03 UTC
[GitHub] [flink] zentol commented on a diff in pull request #18986: [FLINK-26496][flink-yarn-test] [JUnit5 Migration] Module: flink-yarn-test
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org