Correct behavior with Throwables in Assertions.assertAll()

[smoyer64]

Overview

While working on the ErrorCollector rule examples for issue #343, I have found a situation that I'm having an interesting time translating from JUnit 4 to JUnit 5. In that issue I had stated that the functional behavior of the ErrorCollector rule could be accomplished via the Assertions.assertAll() method. This is not entirely true and the canonical example provided in the JUnit 4 javadocs is one example of where the translation fails.

The JUnit 4 ErrorCollector example (without the class and rule set-up) is as follows:

@Test
public void example() {
	collector.addError(new Throwable("first thing went wrong"));
	collector.addError(new Throwable("second thing went wrong"));
	collector.checkThat("ERROR! - something is broke", not(containsString("ERROR!")));
	// all lines will run, and then a combined failure logged at the end.
}

When this code is executed by JUnit 4, the test output provides three failures - two are the exceptions and the third is the AssertionError.

My attempt at a JUnit 5 equivalent is as follows (again stripped of unneeded code):

@Test
public void example() {
	assertAll(
		() -> { throw new Throwable("first thing went wrong"); },
		() -> { throw new Throwable("second thing went wrong"); },
		() -> assertThat("ERROR! - something is broke", not(containsString("ERROR!")))
	);
}

When this code is executed, only the first Executable is run. The Exception handling for assertAll() is documented at http://junit.org/junit5/docs/snapshot/api/org/junit/jupiter/api/Assertions.html#assertAll-java.lang.String-java.util.stream.Stream- and states:

If any supplied Executable throws an AssertionError, all remaining executables will still be executed, and all failures will be aggregated and reported in a MultipleFailuresError. However, if an executable throws an exception that is not an AssertionError, execution will halt immediately, and the exception will be rethrown as is but masked as an unchecked exception.

but wouldn't it be better to run the remainder of the Executables (potentially all throwing Exceptions if there is a problem with the test harness) as opposed to simply stopping at the first Exception that's not resolvable to AssertionError? It's my opinion that wrapping any Throwable inside the MultipleFailuresError is preferable as it provides the test developer/runner with more information.

[jbduncan]

I think I agree with this change. Of course, one has to be careful not to accidentally catch OutOfMemoryError, but I think org.junit.platform.commons.util.BlacklistedExceptions can help with this.

[sormuras]

The keyword throw in the JUnit 5 is the "unfair" difference. Change the first JUnit 4 to something like:
collector.addError(throw new Throwable("first thing went wrong")); // pseudo-code and I think, all the others are not executed as well.

Or change the JUnit 5 snippet to something like: () -> { assertDoesNotThrow(() -> throw new Throwable("first thing went wrong");) }, ... which really reads a bit complicated.

What happens, when you place the assertThat("ERROR! - something is[...] as the first argument? Is the second one executed?

[smoyer64]

@sormuras You're certainly right, that's an unfair comparison and I typed it up that way without even thinking about it (I copy/pasted the JUnit 4 example). I think that the equivalent could be done cleanly if we had a static method Assertions.fail(Throwable t);. The best "equivalent" code I could come up with is:

() -> { throw new AssertionError(new Throwable("first thing went wrong")); }

This code does indeed result in the same three errors reported by JUnit 4. A much cleaner method would look like (though it will add a failure every time and we really need something that adds the AssertionError if a Throwable is encountered):

() -> fail(new Throwable("first thing went wrong")),

Since the negative assertions are pretty consistently assertNotSomething(), I'd like to propose:

() -> assertNotThrowing(new Throwable("first thing went wrong")),

[marcphilipp]

I think adding Assertions.fail(Throwable) would be the a clean and consistent solution. IMHO assertNotThrowing sounds to much like assertThrows.

[smoyer64]

@marcphilipp

I was about to disagree with you, arguing that assertNotThrowing() is the logical negative of assertThrows() and that fail(Throwable) is unconditional but ...

If you think about the kind of tests we'd build with assertAll() or that were built using the ErrorCollector, the JUnit 4 example is pretty contrived. We rarely only test that an Executable doesn't throw an exception and I think I'd probably use an Assumption there since it's likely that we're performing some setup for the assertions that follow. So ... I think the fail(Throwable) method is the best way to accomplish this. We can leave the Exception handling rules for Assertions.assertAll() as they currently are and put the onus on the test developer to decide if they need to catch exceptions that don't resolve to AssertionException.

With this mindset, a real test would look something like this (sorry it's so long):

@Test
public void example() {
	assertAll(
		() -> {
			try {
				callSomeMethodThatShouldNotThrowOne();
			} catch(Throwable t) {
				fail("first thing went wrong", t)
			}
		},
		() -> {
			try {
				callSomeMethodThatShouldNotThrowTwo();
			} catch(Throwable t) {
				fail(t)
			}
		},
		() -> assertThat("ERROR! - something is broke", not(containsString("ERROR!")))
	);
}

Also note that I've included a second form: fail(String, Throwable) so that the user can provide a message in addition to the one that may be encapsulated in the Throwable itself. For the code that provides a JUnit 4 to JUnit 5 translation, I'll allow the code to be similarly contrived as follows:

@Test
public void example() {
	assertAll(
		() -> { throw new AssertionError(new Throwable("first thing went wrong")); },
		() -> { throw new AssertionError(new Throwable("second thing went wrong")); },
		() -> assertThat("ERROR! - something is broke", not(containsString("ERROR!")))
	);
}

Thanks for helping me think this through everyone!