JDK 9 RFR of JDK-8023897: Replace/update/rename executeAndCatch in various tests to assertThrows

[Amy Lu]

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 at 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 at 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
>>
>