consistent exception testing in AssertionTest #1455
Conversation
| @@ -34,6 +34,10 @@ | |||
| // assert false; | |||
| // } | |||
|
|
|||
| private void failAssertionErrorExpected() { | |||
| throw new AssertionError("AssertionError expected"); | |||
There was a problem hiding this comment.
It's intentional that exception is thrown directly here, as this class also have tests for the fail() method. So I think not using fail() in this particular case is a good thing :-)
There was a problem hiding this comment.
I think I would prefer to extract a static constant for "AssertionError expected" and inline this method. That's because it's 100% clear that this code is wrong:
try {
fail("woops');
throw new AssertionError(ASSERTION_ERROR_EXPECTED);
} catch (AssertionError expected) {
...
| @@ -34,6 +34,10 @@ | |||
| // assert false; | |||
| // } | |||
|
|
|||
| private void failAssertionErrorExpected() { | |||
| throw new AssertionError("AssertionError expected"); | |||
There was a problem hiding this comment.
I think I would prefer to extract a static constant for "AssertionError expected" and inline this method. That's because it's 100% clear that this code is wrong:
try {
fail("woops');
throw new AssertionError(ASSERTION_ERROR_EXPECTED);
} catch (AssertionError expected) {
...
| @@ -367,9 +375,11 @@ public void arraysWithNullElementEqual() { | |||
| public void stringsDifferWithUserMessage() { | |||
| try { | |||
| assertEquals("not equal", "one", "two"); | |||
| } catch (Throwable exception) { | |||
| } catch (AssertionError exception) { | |||
There was a problem hiding this comment.
You can change this to catch ComparisonFailure instead (in which case, it's perfectly fine to call fail() in the try block)
|
Thanks! |
That was discovered while discussing #1454, so I would like it be more consistent.