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...@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
