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

[Roger Riggs]

Hi Pavel,

In one case, 'should throw NPE' is not a useful message;  for example in 
parallelPrefix.

If the test was being written new I would advocate to add messages that 
reflect the semantics of
the test case, not the exception message.

But ": Must throw ClassCastException for parameter which is not Comparable."
in Empty navigableMap is much more useful.

And wrapping exceptions in exceptions makes debugging much harder, because
it requires unwrapping the context and figuring out what the test was 
trying to
do and which was the relevant source line.  Here I would advocate for 
the test author
to make sure the diagnostic information is direct and informative and 
not obscured
by stack traces and too much irrelevant detail.

So, in many cases I would give a pass to the changes proposed but wanted 
to observe
that there are improvements possible if they were worth the effort and 
accept that
we don't all use the same style or rigor.

I should probably try to file a bug against TestNG.assertThrows to add a 
variant with
a message. Many/most of the TestNG asserts have variants with messages.

Regards, Roger


On 5/2/2017 4:51 PM, Pavel Rappo wrote:
> I'm not sure I fully understand the points mentioned by Roger, Daniel and Paul.
> Nevertheless I will kindly disagree with all of them, just for the sake of
> having a constructive discussion :-)
>
> My understanding is that org.testng.Assert.assertThrows provides a rich message
> explaining what has happened. Here is what I have just seen:
>
> No exception:
>
>      assertThrows(NullPointerException.class, () -> { });
>
> outputs:
>
> Exception in thread "main" 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 MessageTest.main(MessageTest.java:10)
>
> Wrong exception:
>
>      assertThrows(NullPointerException.class, () -> { throw new IllegalArgumentException("Hey!");});
>
> outputs:
>
> Exception in thread "main" java.lang.AssertionError: Expected NullPointerException to be thrown, but IllegalArgumentException was thrown
>     at org.testng.Assert.expectThrows(Assert.java:1013)
>     at org.testng.Assert.assertThrows(Assert.java:989)
>     at MessageTest.main(MessageTest.java:9)
> Caused by: java.lang.IllegalArgumentException: Hey!
>     at MessageTest.lambda$main$0(MessageTest.java:9)
>     at org.testng.Assert.expectThrows(Assert.java:1005)
>     ... 2 more
>
> I believe that it is as good as it gets in these situations. And might, just
> might, be a tiny bit better than the previous more ahem...cryptic... message:
>
>      should throw NPE
>
> I would also disagree with Roger that having the wrong exception in the cause of
> a thrown one is "a bit awkward". I think it's more straightforward and leaves
> the original exception's message intact and provides even more information.
>
> I would however agree with Paul on having a bunch of shortcuts for standard
> exceptions like: assertThrowsNPE, assertThrowsIAE, assertThrowsISE, etc.
>
> -Pavel
>
>>> On 2 May 2017, at 02:19, Amy Lu <amy.lu at oracle.com> 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
>>>