assertThrows should close returned AutoCloseable in case of failing assert

[runeflobakk]

Hello,

I have encountered a subtle problem with the assertThrows API.

The API accepts an Executable, which conceptually is a void invocation run internally by JUnit, and an exception of a specified type is expected to be thrown, and thus caught. There is really no need to keep any returned value from the operation encapsulated by the Executable, because it is expected to throw.

However, there are times when some attention should be given to a returned value, if the assertion that it throws an exception actually fails, and the Executable manages to successfully execute: methods returning InputStream, OutputStream, and really any instance of AutoCloseable.

Say you have a method with the following signature:

InputStream getData();

And you are testing an error scenario like this:

assertThrows(UnableToGiveDataException.class, () -> getData());

In the case of this test failing (i.e. getData() successfully executes with no exception), the returned InputStream will not be closed, and this may introduce problems for any remaining tests in a suite.

One solution to this is of course to not use assertThrows with methods returning an AutoCloseable instance. But this is not evident when one knows and appreciates the assertThrows-methods, as there is nothing preventing you from using it, and it is admittedly a bit subtle why one should avoid this. It may not manifest as an actual problem in your particular test suite.

Would it be a viable solution to adapt the API to offer overloads of assertThrows which accept for instance a ThrowingSupplier<T>, so JUnit can actually access the returned value in the case of a failing assert? JUnit could then inspect the returned value to see if it is an AutoCloseable, and in that case invoke close(), and then finally throw the appropriate AssertionError because the expected exception was not thrown. This should also enable generating more informative test failure messages, as assertThrows(..) can then fail with information about the value which was returned instead of an expected exception being thrown.

What are your thought on this? 🙂

[marcphilipp]

We actually added overloads of assertThrows with ThrowingSupplier instead of Executable in 5.3.0, found out the hard way that that was a breaking change, and reverted it in 5.3.1 (see #1576 for details). We could add a special assertThrowsAndClose variant. However, since test authors would have to choose that over the regular one, I'm not sure how beneficial that would be. Thoughts?

[marcphilipp]

Tentatively slated for 5.7 M1 solely for the purpose of team discussion.

[runeflobakk]

Ah, yes, overloads with different functional interfaces are typically problematic, especially with method references which also may be overloaded, and they are referred to by name only, so you may get ambiguities.

I guess this issue is related to why e.g. CompletableFuture has methods named thenAccept and thenApply for respectively Consumer and Function, instead of just overloaded methods named then(..), which, in my opinion, would read more elegantly, but introduces the same problem with having overloads of assertThrows taking different functional interfaces as parameter.

If I can offer my two cents here, I am not fan of an assertThrowsAndClose variant, as the name acts a bit "closed" (no pun intended) for what it is used for. An assertThrowsXyz variant which accepts a ThrowingConsumer can also be used for non-AutoCloseable return values to include as information in a failed assertion. I think the API should provide general means to assert on thrown exceptions from operations returning values, and in the case of a failed assertion, it can use the returned value to provide more information in the message of the failing assert. And in addition, it can inspect any returned value to see if it is instance of AutoCloseable, and ensure it is closed.

While a bit cumbersome and non-elegant, I think the naming strategy employed in the API in CompletableFuture to avoid ambiguities with different functional interfaces is actually something to consider as inspiration in this case. Maybe a more appropriate API for asserting thrown exceptions would be something like this:

assertThrowsExecuting(Executable e)
assertThrowsGetting(ThrowingConsumer<?> c)

/**
 * @deprecated Use assertThrowsExecuting if testing a void operation
 *             (not returning a value), or assertThrowsGetting if testing an
 *             operation returning a value.
 */
@Deprecated
assertThrows(Executable e)

This is probably the most extreme solution, as it involves actually deprecating assertThrows. I am not implying this is the ideal solution, but I wanted to is to illustrate how I would prefer such method names to communicate that they are of "equal standing", and not that one is a "special case" of the other, which I think is somewhat implied if one method is named just by adding a postfix to an existing method name.

[marcphilipp]

Wouldn't this work?

assertThrows(UnableToGiveDataException.class, () -> getData().close());

[runeflobakk]

Dang, that's a nice workaround to be able to use the API as-is, and I didn't even think of. Still, one needs to be aware of this to avoid the issue of possible unclosed resources.

[sbrannen]

Still, one needs to be aware of this to avoid the issue of possible unclosed resources.

Sure, but that always holds true for both production code and test code.

In other words, I don't think it's the job of an assertion library to manage resources for the user.

[runeflobakk]

I don't think it's the job of an assertion library to manage resources for the user.

I agree, wholeheartedly.

With production code, one is already in the mindset of treating methods as being executed successfully, and in my experience, in the case of getting an AutoCloseable resource, it is easy to at least argue that one should properly learn and internalize the habit of doing proper resource management. In many cases, an IDE will even warn that there are unmanaged resources.

I think this case is interesting because it is a bit subtle as it sort of flips this mindset around, as operations are expected to fail (failing is success), but in the case of failure (i.e. an operation producing an AutoCloseable resource succeeds), you are in the situation where an instance has been created that really should not just be left for GC without having its resources released. And this involves a bit more mental overhead to reason about. And being that AutoCloseable is such an integral part of the language, it even has first class support with the try-with-resources language facility, and it is part of the java.lang package (as opposed to java.io), I can also see the argument that libraries prone to consume AutoCloseable instances which are not originally in scope of the call site, and neither relayed back to the caller, should take means to properly close it.

It's an interesting discussion none the less 🙂

[marcphilipp]

In the light of the above discussion, the simple workaround, and the lack of reasonable solutions, I'm closing this issue.