On 5/3/17 12:53 AM, Daniel Fuchs wrote:
Hi Amy,
Looks good generally - there are a couple of cases where the
description that was passed to the AssertionError in the original
file is dropped (for instance
http://cr.openjdk.java.net/~amlu/8023897/webrev.00/test/java/util/Map/Defaults.java.frames.html)
Is that going to be an issue when analyzing test logs in case of
failure? IIRC testng prints the parameters passed to the test
method (which includes the description - so maybe that's OK).
Is that why you decided to drop the description?
I tried best to keep the useful message, EmptyNavigableMap
EmptyNavigableSet are examples. For Map/Defaults.java, most cases'
message are test's description and/or "should throw NPE", such
information already in the output.
For example:
The old Defaults.java (modified), in case of failure of no exception thrown
162: assertThrows(
163: () -> { /* map.replaceAll(null); */ },
164: NullPointerException.class,
165: description);
The output:
test
Defaults.testReplaceAllNoNullReplacement("Collections.synchronizedMap(ConcurrentHashMap)",
java.util.Collections$SynchronizedMap@65162892): failure
java.lang.AssertionError: Collections.synchronizedMap(ConcurrentHashMap)
Failed to throw java.lang.NullPointerException expected [true] but found
[false]
at org.testng.Assert.fail(Assert.java:94)
at org.testng.Assert.failNotEquals(Assert.java:496)
at org.testng.Assert.assertTrue(Assert.java:42)
at Defaults.assertInstance(Defaults.java:1010)
at Defaults.assertThrows(Defaults.java:994)
at Defaults.testReplaceAllNoNullReplacement(Defaults.java:162)
The new Defaults.java changed this to:
166: assertThrowsNPE(() -> map.replaceAll(null));
[ change this to demo the failure of no exception:
assertThrowsNPE(() -> {}); ]
The output:
test
Defaults.testReplaceAllNoNullReplacement("*Collections.synchronizedMap(ConcurrentHashMap)*",
java.util.Collections$SynchronizedMap@15502cc3): failure
java.lang.AssertionError: *Expected NullPointerException to be thrown,
but nothing was thrown*
at org.testng.Assert.expectThrows(Assert.java:1018)
at org.testng.Assert.assertThrows(Assert.java:989)
at Defaults.assertThrowsNPE(Defaults.java:972)
at Defaults.testReplaceAllNoNullReplacement(Defaults.java:166)
The only exception is for testReplaceAllNoNullReplacement, the message
" should not allow replacement with null value"
for case
map.replaceAll((k,v) -> null)
is dropped. I just think that it might not worth with extra function for
just this one usage only.
I’ll make this message as a one-line comment when push.
Thanks,
Amy
best regards
-- daniel
On 02/05/2017 10:19, Amy Lu wrote:
Please review this test-only change.
Some java/util tests use a function executeAndCatch which essentially
asserts that an exception is thrown, while some other tests use
assertThrows for doing the same work. For both cases, with jtreg
upgraded to testng 6.9.5 (CODETOOLS-7901639), test can then leverage
TestNG Assert.assertThrows
Please review the patch to update executeAndCatch and assertThrows to
Assert.assertThrows for java/util testng tests.
bug: https://bugs.openjdk.java.net/browse/JDK-8023897
webrev: http://cr.openjdk.java.net/~amlu/8023897/webrev.00/
Thanks,
Amy
