Skip to content

MAPREDUCE-7428. Fix failing MapReduce tests due to the JUnit upgrades in WebServicesTestUtils #5243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aajisaka merged 7 commits into from
Dec 23, 2022
Merged

MAPREDUCE-7428. Fix failing MapReduce tests due to the JUnit upgrades in WebServicesTestUtils #5243

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
baffc92
Test: Remove JUnit API from WebServicesTestUtils to fix MAPREDUCE-7428
aajisaka Dec 19, 2022
f7e550d
Add changes to run tests in hadoop-mapreduce-client-app and hadoop-ma…
aajisaka Dec 19, 2022
31773be
Revert "Add changes to run tests in hadoop-mapreduce-client-app and h…
aajisaka Dec 19, 2022
2889872
Revert "MAPREDUCE-7428. Fix failures related to Junit 4 to Junit 5 up…
aajisaka Dec 19, 2022
75f9604
Remove JUnit API from TestContainerLogsUtils
aajisaka Dec 19, 2022
e1269ae
Add check
aajisaka Dec 20, 2022
ff6d5fa
Remove AssertJ libraries
aajisaka Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,26 +124,6 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
11 changes: 4 additions & 7 deletions ...rn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/TestContainerLogsUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@
import org.apache.hadoop.yarn.logaggregation.filecontroller.LogAggregationFileControllerContext;
import org.apache.hadoop.yarn.logaggregation.filecontroller.LogAggregationFileControllerFactory;

import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* This class contains several utility functions for log aggregation tests.
*/
Expand Down Expand Up @@ -75,13 +73,12 @@ public static void createContainerLogFileInRemoteFS(Configuration conf,
if (fs.exists(rootLogDirPath)) {
fs.delete(rootLogDirPath, true);
}
assertTrue(fs.mkdirs(rootLogDirPath));
fs.mkdirs(rootLogDirPath);
Path appLogsDir = new Path(rootLogDirPath, appId.toString());
if (fs.exists(appLogsDir)) {
fs.delete(appLogsDir, true);
}
assertTrue(fs.mkdirs(appLogsDir));

fs.mkdirs(appLogsDir);
Copy link
Contributor

@steveloughran steveloughran Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does cut the check of the outcome...are we all happy with this?

Copy link
Member Author

@aajisaka aajisaka Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we are happy because the directory has been removed in the previous line, but rethinking this, it's possible to fail removing the directory and then mkdir fails. Adding back the check in the next commit.

Copy link
Contributor

@slfan1989 slfan1989 Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your question, I think we should keep this assertTrue.

We are currently experiencing some difficulties.

TestContainerLogsUtils.java uses assertTrue of org.junit.jupiter, hadoop-mapreduce-client-hs and hadoop-mapreduce-client-app use TestContainerLogsUtils.java, hadoop-mapreduce-client-hs and hadoop-mapreduce-client-app These two modules have not upgraded the junit test, so when running the test, the classes cannot be found.

hadoop-mapreduce-client-hs and hadoop-mapreduce-client-app may take some time to upgrade junit test.

aajisaka's idea can quickly fix this problem without affecting the compilation of other modules.

I will continue to follow up the mapreduce junit upgrade. If the mapreduce module junit upgrade is completed, I will change this back.

Copy link
Contributor

@slfan1989 slfan1989 Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aajisaka I see your latest changes, your changes are reasonable. LGTM.

Copy link
Contributor

@hotcodemacha hotcodemacha Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we are happy because the directory has been removed in the previous line, but rethinking this, it's possible to fail removing the directory and then mkdir fails. Adding back the check in the next commit.

Thanks for adding it back. It makes sense. LGTM

Copy link
Contributor

@hotcodemacha hotcodemacha Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @slfan1989

hadoop-mapreduce-client-hs and hadoop-mapreduce-client-app may take some time to upgrade junit test.

I have already raised PR for both the modules. Can see some failures in hadoop-mapreduce-client-app - will fix it as well in that PR.

Copy link
Contributor

@steveloughran steveloughran Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about removing the asserts and just calling getFileStatus(dir) after.

remember also that delete() returning false means "dir wasn't there"...not a reason to fail

Copy link
Member Author

@aajisaka aajisaka Dec 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added getFileStatus(dir) to check the dir exists in the latest commit. Thank you @steveloughran

createContainerLogInLocalDir(appLogsDir, containerToContent, fs, fileName);
// upload container logs to remote log dir

Expand All @@ -95,7 +92,7 @@ public static void createContainerLogFileInRemoteFS(Configuration conf,
if (fs.exists(path) && deleteRemoteLogDir) {
fs.delete(path, true);
}
assertTrue(fs.mkdirs(path));
fs.mkdirs(path);
uploadContainerLogIntoRemoteDir(ugi, conf, rootLogDirList, nodeId, appId,
containerToContent.keySet(), path);
}
Expand All @@ -111,7 +108,7 @@ private static void createContainerLogInLocalDir(Path appLogsDir,
if (fs.exists(containerLogsDir)) {
fs.delete(containerLogsDir, true);
}
assertTrue(fs.mkdirs(containerLogsDir));
fs.mkdirs(containerLogsDir);
Writer writer =
new FileWriter(new File(containerLogsDir.toString(), fileName));
writer.write(content);
Expand Down
17 changes: 6 additions & 11 deletions .../hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/webapp/WebServicesTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.assertj.core.api.Assertions.assertThat;

public class WebServicesTestUtils {
public static long getXmlLong(Element element, String name) {
Expand Down Expand Up @@ -121,28 +120,24 @@ public static String getXmlAttrString(Element element, String name) {
}

public static void checkStringMatch(String print, String expected, String got) {
assertTrue(
got.matches(expected),
print + " doesn't match, got: " + got + " expected: " + expected);
assertThat(got).as(print).matches(expected);
}

public static void checkStringContains(String print, String expected, String got) {
assertTrue(
got.contains(expected),
print + " doesn't contain expected string, got: " + got + " expected: " + expected);
assertThat(got).as(print).contains(expected);
}

public static void checkStringEqual(String print, String expected, String got) {
assertEquals(got, expected);
assertThat(got).as(print).isEqualTo(expected);
}

public static void assertResponseStatusCode(StatusType expected,
StatusType actual) {
assertResponseStatusCode(null, expected, actual);
assertThat(expected.getStatusCode()).isEqualTo(actual.getStatusCode());
}

public static void assertResponseStatusCode(String errmsg,
StatusType expected, StatusType actual) {
assertEquals(expected.getStatusCode(), actual.getStatusCode(), errmsg);
assertThat(expected.getStatusCode()).withFailMessage(errmsg).isEqualTo(actual.getStatusCode());
}
}