Improved information with failed ExpectedException assertion #369

arjenw · 2012-01-03T10:12:11Z

When using the ExpectedException to test for an exception to occur, it
throws an AssertionError when thrown exception does not match the
expected exception. Issue is however that in the JUnit runner results only
the actual exceptions message is shown and not the stacktrace. To fix
that, the actual exception is added to the cause of the AssertionError,
giving the exceptions in a format like:
java.lang.AssertionError: [reason]
Expected: [expected exception]
got: [actual exception + message]
at [AssertionError stacktrace]
Caused by: [actual exception + message]
at [actual exception stacktrace]

When using the ExpectedException to test for an exception to occur, it throws an AssertionError when thrown exception does not match the expected exception. Issue is however that in the JUnit runner results only the actual exceptions message is shown and not the stacktrace. To fix that, the actual exception is added to the cause of the AssertionError, giving the exceptions in a format like: java.lang.AssertionError: [reason] Expected: [expected exception] got: [actual exception + message] at [AssertionError stacktrace] Caused by: [actual exception + message] at [actual exception stacktrace]

KentBeck · 2012-01-03T15:42:03Z

lgtm. David?

When using the ExpectedException to test for an exception to occur, it throws an AssertionError when thrown exception does not match the expected exception. Issue is however that in the JUnit runner results only the actual exceptions message is shown and not the stacktrace. To fix that, the actual exception is added to the cause of the AssertionError, giving the exceptions in a format like: java.lang.AssertionError: [reason] Expected: [expected exception] got: [actual exception + message] at [AssertionError stacktrace] Caused by: [actual exception + message] at [actual exception stacktrace]

arjenw · 2012-01-03T16:47:12Z

I added a test case in the above commit

(they were the same) and relocated the changes to a better location in the file which is more inline with the rest of the file. And, of course, removed the superfluous brackets.

arjenw · 2012-01-04T16:39:31Z

I am a bit doubting what the right place is for this fix. Right now I made the fix in ExpectedException. This nicely limits the scope of this fix to the usage of ExpectedException only. However, I also encountered a similar situation when using the Assert.assertThat() API with the actual parameter being an exception. That situation was without the ExpectedException usage, but was verifying whether an exception was thrown. So, the alternative of this fix is:

    public static <T> void assertThat(String reason, T actual,
            Matcher<? super T> matcher) {
        if (!matcher.matches(actual)) {
            Description description= new StringDescription();
            description.appendText(reason);
            description.appendText("\nExpected: ");
            description.appendDescriptionOf(matcher);
            description.appendText("\n     got: ");
            description.appendValue(actual);
            description.appendText("\n");
            java.lang.AssertionError assertionError= new java.lang.AssertionError(description.toString());
            if(actual instanceof Throwable) 
                assertionError.initCause((Throwable) actual);
            throw assertionError;
        }
    }

In which I replaced the line

throw new java.lang.AssertionError(description.toString());

with:

            java.lang.AssertionError assertionError= new java.lang.AssertionError(description.toString());
            if(actual instanceof Throwable) 
                assertionError.initCause((Throwable) actual);
            throw assertionError;

What is/are your opinion(s) on this? Where should this be fixed?

Tibor17 · 2012-01-04T20:06:12Z

@KentBeck

IMHO; in order to make a clear solution, one should not be able to use
java.lang.AssertionError in ExpectedException.

Reasons:

The exception should not escape the "org.junit.*" packge;
When a test fails, the writer of tests should not be able to operate with failure exception, i.e. java.lang.AssertionError - JUnit should have own one;
The programmer should instead use
@test(failed=true) and an use of other descriptive methods on @test should not be possible;
java.lang.AssertionError is in conflicts with the JVM switch -ea (enabled assertions)
.

Me as a JUnit features designer do not like the idea, that have to be dependent on AssertionError - it's very dirty solution.

arjenw · 2012-01-04T20:38:04Z

@Tibor17
I am not sure whether I understand your comment correctly, but let me explain the reason I want to have this. Right now, when I make a test using the ExpectedException rule (or by using Assert.assertThat() directly), defining that I expect exception X to happen, JUnit will report me when instead of X, exception Y happened. However, it will report it as: java.lang.AssertionError: Expected X, but got Y (with the stacktrace of the assert statement only.

Now in order for me to find out why and where Y happened I have to debug the test first because I don't have the information of exception Y.

This solution proposal fixes that by adding Y as the cause of the AssertionError. This gives more insights in the error message and enables users to find the root cause faster and more easily.

that's a better place and has cleaner code than the original. Included tests for both scenario's (assertThat as well as the ExpectedException).

dsaff · 2012-02-01T21:58:12Z

@arjenw, I think that the version before this latest commit (where the logic was in ExpectedException) was the better take on this. Can we revert to that?

arjenw · 2012-02-02T09:47:01Z

The main issue with the version before latest commit is that it only works for ExpectedException rules. This severely limits the use as all situations in which you want to assert on the Exception itself (for example with exception message / message code matchers) and validate some extra state after such an exception has been thrown, it's not usable. In such cases you want to be able to assert the expected exception has been thrown with Assert.assertThat().

However, when the wrong exception has been thrown you'll loose the exception information.

On the other hand I understand that having such a type specific action in the generic Assert.assertThat() is not desired. However there is no possibility to hook into that and write the right describer for a specific type.

So, the question remains: what should we do?

dsaff · 2012-02-02T16:19:13Z

src/main/java/org/junit/rules/ExpectedException.java

@@ -111,6 +111,7 @@ public void evaluate() throws Throwable {
 			} catch (Throwable e) {
 				if (fMatcher == null)
 					throw e;
+


Take out extra line here.

dsaff · 2012-02-02T16:22:41Z

OK, I can see it your way. Please see inline comments.

* Moved CauseMatcher to ResultMatchers and splitted it up in two matchers (failureIs and causedBy) * Added tests for both matchers

arjenw · 2012-02-29T12:38:42Z

What do you think about my latest changes (committed one month ago), in which I implemented the comments of @dsaff?

dsaff · 2012-03-28T15:36:37Z

src/main/java/org/junit/experimental/results/ResultMatchers.java

+			@Override
+			public boolean matchesSafely(Result item) {
+				for(Failure f: item.getFailures())
+					return failureMatcher.matches(f.getException());


We should beef up the javadoc. This "Matches if the first exception in the failure matches the given {@code exceptionMatcher}"

* Fixed the javadoc of ResultMatchers.failureIs() * Made the ResultMatchers.failureIs() matcher based on PrintableResult instead of Result * Introduced method 'getFailures()' in PrintableResult to achieve that * Moved causedBy() matcher from ResultMatchers to JUnitMatchers (as it is a Throwable matcher) * Introduced the required tests at the right places

arjenw · 2012-04-25T19:02:40Z

I reworked the code based on the suggestions. Please let me know if this is fine.

dsaff · 2012-04-25T20:38:02Z

Thanks! I'm too swamped to review today, but will do so soon.

dsaff · 2012-05-24T13:37:29Z

src/main/java/org/junit/Assert.java

+			if (actual instanceof Throwable)
+				assertionError.initCause((Throwable) actual);
+			throw assertionError;
+	}


This brace looks oddly indented.

dsaff · 2012-05-24T13:39:12Z

Sorry for the long-delayed review!

…) and fixed the oddly indented brace

…self

arjenw · 2012-05-24T15:07:18Z

I implemented the required changes in the last two commits (sorry, struggling with GIT as I am not used to it).

dsaff · 2012-05-24T15:09:37Z

Hmm. Looks like we're going to need to rebase to HEAD. Do you know how to do that?

Conflicts: src/test/java/org/junit/tests/AllTests.java src/test/java/org/junit/tests/experimental/rules/ExpectedExceptionRuleTest.java Merged changes from the HEAD branch Besides the merged changes also the approach to test for the failure has been changed to comply to the new way of testing in ExpectedExceptionTest. That no longer required the changes in ResultMatchers (with their accompanying tests)

arjenw · 2012-05-24T20:13:57Z

You meant this? I have not only pulled all changes, but also refactored my changes a bit such that it is using the new format of the ExpectedExceptionTest. Therefore I moved the 'failureIs' matcher from ResultMatchers to EventCollector (which allowed me to undo my changes in ResultMatchers, ResultMatchersTests and PrintableResult. Let me know what you think of it.

Improved information with failed ExpectedException assertion

dsaff · 2012-05-24T20:27:46Z

Thanks!

unknown added 2 commits January 3, 2012 17:36

Merge branch 'master' of github.com:arjenw/junit

5e5ec57

arjenw added 2 commits January 4, 2012 09:23

Reused HasWrongExpectedException instead of own defined class

bd6dc3a

(they were the same) and relocated the changes to a better location in the file which is more inline with the rest of the file. And, of course, removed the superfluous brackets.

Merge branch 'master' of github.com:arjenw/junit

126b8eb

Implemented the change now on org.junit.Assert.assertThat() as I think

5392910

that's a better place and has cleaner code than the original. Included tests for both scenario's (assertThat as well as the ExpectedException).

dsaff reviewed Feb 2, 2012
View reviewed changes

* Removed empty line in ExpectedException

052fc09

* Moved CauseMatcher to ResultMatchers and splitted it up in two matchers (failureIs and causedBy) * Added tests for both matchers

dsaff reviewed Mar 28, 2012
View reviewed changes

dsaff reviewed May 24, 2012
View reviewed changes

arjenw added 2 commits May 24, 2012 17:00

Renamed ThrowableMatcher to CauseMatcher (including accompanying test…

07045d9

…) and fixed the oddly indented brace

Files referring to the new CauseMatcher including the CauseMatcher it…

e381886

…self

dsaff added a commit that referenced this pull request May 24, 2012

Merge pull request #369 from arjenw/master

99c4e59

Improved information with failed ExpectedException assertion

dsaff merged commit 99c4e59 into junit-team:master May 24, 2012

matthewfarwell mentioned this pull request May 25, 2012

If test that uses ExpectedException fails with an unexpected exception, the stack trace of the original exception is lost. #141

Closed

marcphilipp mentioned this pull request Jul 16, 2012

Upgrade to Hamcrest 1.3 #404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved information with failed ExpectedException assertion #369

Improved information with failed ExpectedException assertion #369

arjenw commented Jan 3, 2012

KentBeck commented Jan 3, 2012

arjenw commented Jan 3, 2012

arjenw commented Jan 4, 2012

Tibor17 commented Jan 4, 2012

arjenw commented Jan 4, 2012

dsaff commented Feb 1, 2012

arjenw commented Feb 2, 2012

dsaff Feb 2, 2012

dsaff commented Feb 2, 2012

arjenw commented Feb 29, 2012

dsaff Mar 28, 2012

arjenw Apr 25, 2012

arjenw commented Apr 25, 2012

dsaff commented Apr 25, 2012

dsaff May 24, 2012

dsaff commented May 24, 2012

arjenw commented May 24, 2012

dsaff commented May 24, 2012

arjenw commented May 24, 2012

dsaff commented May 24, 2012

Improved information with failed ExpectedException assertion #369

Improved information with failed ExpectedException assertion #369

Conversation

arjenw commented Jan 3, 2012

KentBeck commented Jan 3, 2012

arjenw commented Jan 3, 2012

arjenw commented Jan 4, 2012

Tibor17 commented Jan 4, 2012

arjenw commented Jan 4, 2012

dsaff commented Feb 1, 2012

arjenw commented Feb 2, 2012

dsaff Feb 2, 2012

Choose a reason for hiding this comment

dsaff commented Feb 2, 2012

arjenw commented Feb 29, 2012

dsaff Mar 28, 2012

Choose a reason for hiding this comment

arjenw Apr 25, 2012

Choose a reason for hiding this comment

arjenw commented Apr 25, 2012

dsaff commented Apr 25, 2012

dsaff May 24, 2012

Choose a reason for hiding this comment

dsaff commented May 24, 2012

arjenw commented May 24, 2012

dsaff commented May 24, 2012

arjenw commented May 24, 2012

dsaff commented May 24, 2012