Skip to content
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

Jump to bottom

GitHub 2913 #2914

Merged
merged 4 commits into from
May 30, 2023
Merged

GitHub 2913 #2914

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a4394b
add tests for maps, including failing test for null values (issue 2913)
May 25, 2023
4216a3d
fix the failing test for maps with null values
May 25, 2023
732fb0d
update CHANGES.txt
May 25, 2023
88eb73f
apply code review comments - TY
May 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Current

Fixed: GITHUB-2913: Maps containing nulls can be incorrectly considered equal
Copy link
Member

Choose a reason for hiding this comment

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

@ahgittin It would be good if you could please help add your ID/name at the end of this line, so that we can call out your contribution in the release notes of this version (when it goes out). Its our small way of thanking you for helping TestNG become better.


7.8.0
Fixed: GITHUB-2906: Generate testng-results.xml per test suite (Krishnan Mahadevan)
New: GITHUB-2897: Not exception but warning if some (not all) of the given test names are not found in suite files. (Bruce Wen)
Expand Down
7 changes: 5 additions & 2 deletions testng-asserts/src/main/java/org/testng/Assert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2109,11 +2109,14 @@ private static String getNotEqualReason(Map<?, ?> actual, Map<?, ?> expected) {
Object key = entry.getKey();
Object value = entry.getValue();
Object expectedValue = expected.get(key);
String assertMessage =
"Maps do not match for key:" + key + " actual:" + value + " expected:" + expectedValue;
if (!areEqualImpl(value, expectedValue)) {
String assertMessage =
"Maps do not match for key:" + key + " actual:" + value + " expected:" + expectedValue;
return assertMessage;
}
if (value == null && !expected.containsKey(key)) {
return "Maps do not match for key:" + key + " actual: null but not present in expected";
}
}
return null;
}
Expand Down
88 changes: 88 additions & 0 deletions testng-asserts/src/test/java/test/asserttests/AssertTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,4 +563,92 @@ public void checkArrayNotEqualsFailsWhenDifferentOrder() {

assertNotEquals(array1, array2);
}

@Test
public void checkMapEquals() {
Map<String, Object> map1 = new LinkedHashMap<>();
map1.put("a", "1");
map1.put("b", 2);
Map<String, Object> map2 = new LinkedHashMap<>();
map2.put("b", 2);
map2.put("a", "1");

assertMapsEqual(map1, map2);
}

@Test
public void checkMapNotEquals() {
Map<String, Object> map1 = new LinkedHashMap<>();
map1.put("a", "1");
map1.put("b", "2");
Map<String, Object> map2 = new LinkedHashMap<>();
map2.put("a", "1");
map2.put("b", 2);

assertMapsNotEqual(map1, map2);
}

@Test(description = "GITHUB-2913")
public void checkMapNotEqualsWithNull() {
Map<String, Object> map1 = new LinkedHashMap<>();
map1.put("a", 1);
map1.put("b", null);
Map<String, Object> map2 = new LinkedHashMap<>();
map2.put("a", 1);
map2.put("c", null);

assertMapsNotEqual(map1, map2);
}

protected void assertMapsEqual(Map m1, Map m2) {
assertEquals((Object) m1, (Object) m2);
assertEquals(m1, m2);

boolean succeeded = false;
try {
Copy link
Member

Choose a reason for hiding this comment

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

can we refactor this to

try {
     assertNotEquals((Object) m1, (Object) m2);
    Assert.fail("Maps reported as not equal when equal: " + m1 + " / " + m2);
} catch (AssertionError ignored) {}

This will do away with the extra variable succeeded

Copy link
Author

Choose a reason for hiding this comment

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

I like the var name instead of the comment -- I've called it expected.

However putting the Assert.fail call inside the try will cause the AssertionError that it throws to be ignored, won't it?, and that defeats the point of the test. I don't know of a simple way to avoid the ugly succeeded, apart from introducing lambdas or splitting up the test and using expectedExceptions or checking the body of the failure message -- all of which I think are worse. (But I don't feel at strongly and this is your codebase so LMK if you prefer something different!)

Copy link
Author

Choose a reason for hiding this comment

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

(gradle insists on putting a newline between the {} but it saves a bit of bloat)

Copy link
Member

Choose a reason for hiding this comment

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

You can try {} //

Copy link
Member

Choose a reason for hiding this comment

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

@ahgittin - Sigh! my bad. Yes you are correct.

assertNotEquals((Object) m1, (Object) m2);
succeeded = true;
} catch (AssertionError e) {
/* expected */
}
if (succeeded) {
Assert.fail("Maps reported as not equal when equal: " + m1 + " / " + m2);
Copy link
Member

Choose a reason for hiding this comment

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

Could be replaced but assertFalse

}

try {
assertNotEquals(m1, m2);
succeeded = true;
} catch (AssertionError e) {
/* expected */
}
if (succeeded) {
Assert.fail("Maps reported as not equal when equal: " + m1 + " / " + m2);
}
}

protected void assertMapsNotEqual(Map m1, Map m2) {
boolean succeeded = false;
try {
assertEquals((Object) m1, (Object) m2);
succeeded = true;
} catch (AssertionError e) {
/* expected */
}
if (succeeded) {
Assert.fail("Maps reported as equal when not equal: " + m1 + " / " + m2);
}

try {
assertEquals(m1, m2);
succeeded = true;
} catch (AssertionError e) {
/* expected */
}
if (succeeded) {
Assert.fail("Maps reported as equal when not equal: " + m1 + " / " + m2);
}

assertNotEquals((Object) m1, (Object) m2);
assertNotEquals(m1, m2);
}
}