HADOOP-18947. Fixed test flakiness during string comparision#6215
Merged
ayushtkn merged 1 commit intoapache:trunkfrom Nov 4, 2023
Merged
HADOOP-18947. Fixed test flakiness during string comparision#6215ayushtkn merged 1 commit intoapache:trunkfrom
ayushtkn merged 1 commit intoapache:trunkfrom
Conversation
slfan1989
changed the title
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Oct 23, 2023
| String[] lines2 = str2.trim().split("\n"); | ||
| Arrays.sort(lines1); | ||
| Arrays.sort(lines2); | ||
| return Arrays.equals(lines1, lines2); |
Member
There was a problem hiding this comment.
You don't use the output.
You maye want to call it something closer to assert and also do a final:
assertEquals(lines1, lines2)
ayushtkn
reviewed
Oct 24, 2023
Member
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
You are not checking the return value of compareStrings, so even if it returns false, I believe your test will pass.
btw. rather than sort & then array equals and then asserting true/false, did you explore something like
public static void compareStrings(String str1, String str2) {
Assertions.assertThat(str1.split("\n")).containsExactlyInAnyOrder(str2.split("\n"));
}
kavvya97
force-pushed
the
nondex-testHistoryViewPrinterAll
branch
from
0988bcd to
0ce8ce9
Compare
we have incorporated the above changes in the PR |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Oct 26, 2023
| } | ||
|
|
||
| private void compareStrings(String str1, String str2) { | ||
| Assertions.assertThat(Arrays.asList(str1.trim().split("\n"))).containsExactlyInAnyOrderElementsOf(Arrays.asList(str2.trim().split("\n"))); |
Member
There was a problem hiding this comment.
- This could be made a little more readable by extracting the two chunks.
- Make the strings final.
- Name the method something like assertEqualLines().
- Make method static.
kavvya97
force-pushed
the
nondex-testHistoryViewPrinterAll
branch
from
0ce8ce9 to
32af584
Compare
|
🎊 +1 overall
This message was automatically generated. |
kavvya97
force-pushed
the
nondex-testHistoryViewPrinterAll
branch
from
32af584 to
777daa6
Compare
|
incorporated the changes. Thanks for the review |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Nov 1, 2023
Member
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
LGTM
PS. This util is good here, because we know str1 & str2 can't be null, else the util can throw NPE
goiri
approved these changes
Nov 1, 2023
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…g comparison (apache#6215). Contributed by Rajiv Ramachandran. Reviewed-by: Inigo Goiri <inigoiri@apache.org> Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Setup:
Java version: openjdk 11.0.20.1
Maven version: Apache Maven 3.6.3
Issue: https://issues.apache.org/jira/browse/HADOOP-18947
Description of PR
The test
org.apache.hadoop.mapreduce.jobhistory.TestHistoryViewerPrinter#testHumanPrinterAllcan fail due to flakiness. These flakiness occurs because the test utilizes Hashmaps values and converts the values to string to perform the comparision and the order of the objects returned may not be necessarily maintained. This can be detected by utilizing the Nondex plugin.Steps to reproduce:
git clone https://github.com/apache/hadoopmvn install -pl hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core -am -DskipTestsmvn -pl hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core test -Dtest=org.apache.hadoop.mapreduce.jobhistory.TestHistoryViewerPrinter#testHumanPrinterAllmvn -pl hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.apache.hadoop.mapreduce.jobhistory.TestHistoryViewerPrinter#testHumanPrinterAll-DnondexMode=ONE& FULL Mode-DnondexMode=FULL(shuffles differently for each call)Root cause:
The test utilizes Hashmap for storing job information and builts the string using HashMap.values(). However, the order of the objects returned by a Hashmap may not be maintained. Hence the test fails due to the string comparision. The following error occurs
Fix
The Fix involves extracting a single line of the string and sorting them to perform the string comparision. Since the strings are sorted, The comparision passes successfully. The Alternate way of sorting was converting to json but string being built is not json. Moreover, There was no library available to convert string to json.
How was this patch tested?
The fix was tested by adding a suitable fix and running the Nondex plugin again and ensuring that all the tests pass in FULL Mode and ONE Mode of the Nondex runs.