Hi Amy,
Yes, a good cleanup.
- EmptyNavigableMap.java in assertThrows(class, runnable, message) will be a
bit awkward to debug since the stack trace of the real cause will be
buried in
the cause of the new AssertionError thrown.
Without re-implementing the TestNG.expectThrows[1] method, I would be
more inclined
to print the message and then re-throw the exception.
Still not best, but doesn't require knowing to look in the exception
cause to find the
location of the original throw.
Ditto EmptyNavigableSet.java:72
The messages are useful when debugging to know the reason for the
exception without
having to reverse engineer the code.
- java/util/Map/Defaults.java:167 The {}'s around the lambda value are
not necessary (like 166).
Thanks, Roger
[1]
https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/Assert.java
: 1119
On 5/2/2017 7:15 AM, Pavel Rappo wrote:
Hi Amy,
Thanks a lot for doing this! I personally think it's a beginning of a very
valuable refactoring to our test base. It turns out this little piece of
functionality, namely, checking that a particular exception is thrown from code
is error-prone. I have seen many buggy implementations defective in one way or
another. What's worse is that sometimes many of them coexist in the same code
base. Even in the JDK this beast has many names:
tryCatch, verifyException, expectThrows, checkException, expectThrow,
assertThrows, THROWS, check, uncaughtException, testThrowException,
assertSecurityException, ...
This list does not include countless ad-hoc bare try-catch constructs. After
this has been done, I would go even further, and in some cases change tests that
use @Test(expectedException=...) annotation to use this method instead.
Looks good. Wait for a Reviewer and you are good to go.
Thanks,
-Pavel
On 2 May 2017, at 10: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
