Add Assert#expectThrows

[rschmitt]

Continued from #1153. This is dramatically simpler than the first draft.

[kcooney]

I strongly feel this should take a class object of the expected type, for all the reasons that I gave as #1153

While Arrange-Act-Assert is a good framework to keep in mind when writing tests, it isn't the only lens we should use when we design APIs.

A common thing I think about when designing an API is "Make Interfaces Easy to Use Correctly and Hard to Use Incorrectly" (see http://programmer.97things.oreilly.com/wiki/index.php/Make_Interfaces_Easy_to_Use_Correctly_and_Hard_to_Use_Incorrectly).

The API here encourages code that provides less than useful error messages when the code under test doesn't meet the tester's expectations. For example:

 // produces a useless message when it fails
assertTrue(expectThrow(client::invoke) instanceof RuntimeException);

// slightly better messaging, but ugly,
// and doesn't allow the caller to provide context
IllegalArgumentException e
    = (IllegalArgumentException) expectThrow(client::invoke); // not much better

Worse, the test could pass for the wrong reason:

expectThrow(client::invoke); // what the method starts throwing StackOverflowError ?

It also makes it difficult to do simple things:

assertEquals(3, ((MyException) expectThrow(client::invoke)).getErrorCode());

Both of these are solved if we pass in the expected type:

assertEquals(3, expectThrow(MyException.class, client::invoke).getErrorCode());

ClientException e = expectThrow(MyException.class, client::invoke);
assertThat(e.getMessage()).contains("connection is closed");
assertEquals(3, e.getErrorCode());

[rschmitt]

There's a middle ground here that I actually like better. We can adjust the definition by genericizing the return type:

    public static <T extends Throwable> T expectThrows(ThrowingRunnable runnable) {
        return expectThrows(null, runnable);
    }

    public static <T extends Throwable> T expectThrows(String message, ThrowingRunnable runnable) {
        try {
            runnable.run();
        } catch (Throwable t) {
            return (T) t;
        }
        Assert.fail(message);
        throw new AssertionError(); // This statement is unreachable.
    }

This permits the following:

        // I don't care what gets thrown, and that's okay:
        expectThrows(() -> {throw new NullPointerException();});

        // No explicit cast takes place here:
        NullPointerException npe = expectThrows(() -> {throw new NullPointerException();});
        Assert.assertNull(npe.getMessage());

        // The following will throw a CCE at runtime:
        IOException ex = expectThrows(() -> {throw new NullPointerException();});

Your examples become:

ClientException e = expectThrow(client::invoke);
assertThat(e.getMessage()).contains("connection is closed");

// This is still tricky without a cast--either way, an explicit type hint is required
assertEquals(3, ((ClientException) expectThrow(client::invoke)).getErrorCode());

ClientException e = expectThrow(client::invoke);
assertThat(e.getMessage()).contains("connection is closed");
assertEquals(3, e.getErrorCode());

[kcooney]

I don't think that addresses my concern about having a good error message if an unexpected exception is thrown.

I think being explicit provides a cleaner, more obvious and easier to use API.

[rschmitt]

See the latest diff.

[kcooney]

If someone feels strongly that they don't want to have to specify the expected exception type, they could do that outside of JUnit with a one-line method. Or we could add it in a future release. Let's start by requiring the caller explicitly, and see what people say when they start using the API.

[kcooney]

Could you explain the use case for this? When would you know a call would throw but not know or care what exception is thrown?

And how would the caller specify T?

[rschmitt]

If I have a constructor that enforces preconditions on its arguments (e.g. no null references), I don't really care what happens when bad arguments get passed into the constructor, as long as it doesn't return normally (thereby allowing a reference to an inconsistent object to escape). It doesn't matter if the constructor throws IllegalArgumentException or a NullPointerException or an IndexOutOfBoundsException. That's an irrelevant detail, and over-specifying it will lead to the kind of incidental complexity that Stuart Halloway warns about in the Narcissistic Design talk (at about 15:10).

The caller doesn't need to specify T, it will be inferred by the compiler in the context of (for instance) an assignment statement. This is just what I showed in my earlier example:

// No explicit cast takes place 
NullPointerException npe = expectThrows(() -> {throw new 
Assert.assertNull(npe.getMessage());

[kcooney]

You don't care if the constructor throws a StackOverflowError or ThreadDeath?

If you don't care between IllegalArgumentException and NullPointerException, then expect a RunTimeException. If you find that to be too much typing, create your own version that doesn't require an exception.

[rschmitt]

First of all, you're not objecting to my API design or my testing practices, you're objecting to catching Throwable. And you can play similar games with Exception too: what are you, crazy? You can't catch Exception! What if that's an InterruptedException you just swallowed?! You didn't set the interrupted flag, you monster!

Catching RuntimeException, if it even works, is just a different kind of hack. In your case, it's not what you actually want to catch, it just happens to be the "most recent common ancestor" of the exceptions that you're actually interested in, but since there's no way to specify a type union in Java you specify an ancestor class instead. I could just as easily respond by saying: don't you care if your constructor throws ArithmeticException? You weren't actually expecting the constructor to divide by zero, were you? Now the test is passing for the wrong reason!

Furthermore: no, I don't care if my constructor throws ThreadDeath or StackOverflowError, because I know apodictically and with absolute metaphysical certitude that no constructor I wrote is going to throw a VirtualMachineError during a unit test, unless the entire JVM is coincidentally destabilized at the exact same time for an unrelated reason.

Finally, even if you're very very precise in the types that you throw and catch, and you assert that (say) NullPointerException and only NullPointerException is caught, you still haven't necessarily demonstrated that you're making correct assertions. You might be catching an NPE thrown by a different (e.g. prior) statement, or one that was propagated from code the SUT called, and then your code is passing for the wrong reason. So at this point we have a few options:

• Associate a unique public exception type with each throws statement (which still doesn't work with recursion, maybe we need to add global locking or some sort of Semaphore)
• Require the user to specify an assertion containing both the exception type and the message that that exception is to contain, and then I guess publish some sort of Checkstyle config that ensures at build time that all such messages are unique (I guess this brings us to those opaque Microsoftian "error codes")
• Be reasonable and concede that "this call does not return via normal control flow" is a coherent and intelligible assertion that can stand alone in a unit test

[kcooney]

Thanks for the Javadoc and the tests

[marcphilipp]

This is an unchecked cast. If we keep this method and the one below IMHO it should rather return just Throwable without any generics.

[marcphilipp]

Maybe this method and the one below should be called expectThrowable?

[rschmitt]

The cast is checked at runtime, just like an explicit downcast from Throwable.

[marcphilipp]

When I call expectThrows(IllegalArgumentException.class, runnable) but runnable does not throw any exception we will call fail(null) here, won't we? I think we need a better default message than that.

[rschmitt]

Ok:

        if (message == null) {
            message = String.format("Expected %s to be thrown, but nothing was thrown.", throwableClass.getSimpleName());
        }

[marcphilipp]

Sorry, I'm still convinced that we should rather return Throwable instead of using generics in this way.

[dsaff]

I'm just getting up to speed with this pull request, but I also am a little unsure about this, since it appears to provide a way to write code that is functionally equivalent to one with an explicit cast, just with the cast hidden.

[kcooney]

I agree with Marc and David here. If the return type here is inferred to be something other than Throwable then the caller is expecting a specific exception, so the caller should provide a way to specify what is expected so they get a better error message (and one that includes the passed-in message).

If the caller doesn't care what is being thrown, but wants to assert that "something" is thrown, then we should return a Throwable and the caller can assert on it or do a cast on it.

[marcphilipp]

@rschmitt Thanks for your pull request, I like it!

I think this needs to go into junit-lambda, though. That would allow us to use JDK 8 interfaces like Supplier or Predicate. I would really like to use a Supplier<String> for the user-supplied message and maybe even a Predicate<? super Throwable> for the exception check so one could easily replace instanceof with exact-class semantic.

[marcphilipp]

Why have you replaced the call of isInstance with cast?

[rschmitt]

You don't need JDK8 to operate with functional interfaces like Supplier<T> and Predicate<T>. Any backport of any subset or superset of those interfaces is basically equivalent. My first draft of this change used a predicate instead of returning a throwable at all. The only major feature that actually requires JDK8 (from the perspective of a library author) is default methods on interfaces. However, all of interfaces you've already defined ship with JUnit itself, and cannot be redefined in junit-lambda. So unless you're planning on building something substantial on a lesser-known JDK8 feature like JSR308 type annotations, I'm not sure what the point of junit-lambda is. The JDK8 backwards compatibility story is very, very solid. You're not supposed to need to vend an extension library.

[kcooney]

Although I understand your argument about not caring about the exception type, very few people share that view. Those that do can pass in Throwable.class or make their own overload.

I also share Marc's concerns about the casting, and the return type being implicitlly referred (APIs that do often don't do what you want when you want).

Finally, it's extremely difficult to remove a method from JUnit once it's added. We can add it later. YAGNI.

I'm sorry, but I still strongly feel that the exception type should be explicitly specified, just like we do in ExpectedException and @Test, and just like you do in the tests you are adding.

That all said, I think we are at the point where we should ask the other maintainers for their view. @dsaff @marcphilipp and @stefanbirkner can you comment on these proposed APIs?

[kcooney]

@marcphilipp I'm intrigued by your idea of using Supplier or Predicate in an exception API. Do you think you'd have time to put together a pull request against JUnitLambda with a sketch of an API and some example usages?

Edit: I haven't this too much thought, but I'd be fine with submitting an API without Java APIs now, and adding overloads that take a Predicate instead of a Class<Throwable>. I think even with lambdas you would want both versions.

[kcooney]

@rschmitt apologies if I sound argumentative; it isn't my intention. We're all very passionate about APIs and testing, so naturally we'll have disagreements. As you can see, the JUnit maintainers don't always agree with each other :-)

[kcooney]

@Tibor17 I strongly feel that the caller should pass in the expected exception type into these methods. That way, JUnit can give an extremely useful message if a different exception is thrown (including a stack trace).

To get the behavior you want, users can pass in Throwable.class as the first parameter. We can always add the methods you are asking for to JUnit proper later.

I personally prefer expectThrows over expectedThrown, but I'll defer to @marcphilipp on method naming.

[rschmitt]

Okay, I think that's everything. Any additional comments on the latest revision (7d49b90)?

[marcphilipp]

LGTM!

@kcooney PTAL

[kcooney]

I believe this can be:

public static void assertThrows(Class<? extends Throwable> expectedThrowable, ...)

There's only a need to make the method generic if the value is used in more than once place

[kcooney]

@rschmitt looks very nice. Just some minor issues.

[rschmitt]

Done.

[kcooney]

Thanks @rschmitt ... LGTM. Just waiting for Marc's thoughts on ThrowingRunnable being top-level vs. nested.

[rschmitt]

Yeah, that's a tricky one. There's no way to get around it being public, and it's not really a perfect fit for any existing classes or packages. I have two suggestions:

• Make it a top-level class in org.junit.internal. I think it's unlikely that someone will want to use this interface for something other than a lambda-friendly API, but if that does occur you can at least promote it to the public API. (Distressingly, this change would not be binary compatible; even if nobody imports ThrowingRunnable in their Java source and just uses lambdas, javac will still generate a class that implements it explicitly.)
• Make it a top-level class in org.junit and just leave it there. What's the worst that could happen? It's not like you can accidentally depend on it in production code (unless you've already added junit as a compile-scoped Maven dependency or something).

[kcooney]

I don't think it makes sense to put it in an internal package. End users would effectively be creating anonymous classes that implement it.

I actually was thinking we should put the interface in a new package, org.junit.function.

[rschmitt]

That sounds like a decent idea if and only if you already have two or three highly plausible examples of other classes that could go in that package. It strikes me as odd to have a package with just one class in it, even though this is Java and the packages don't matter.

[marcphilipp]

Let's leave it where it is now. We can easily move it to a top-level class later on. We'd have to deprecate it in Assert and have it extend the top-level interface.

[rschmitt]

In that case, I guess we're done! Let me know if I should squash or rebase anything.

[marcphilipp]

We'll squash them ourselves, when we merge this pull request. This way, the conversations in this pull will stay readable, at least as readable as they are right now. I will merge it tonight.

[dsaff]

Belated, but +1 to @marcphilipp's approach here.

[marcphilipp]

Squashed and merged. Thank you very much, @rschmitt!

@rschmitt Can you please add a section about the two new methods to the release notes?

[rschmitt]

I'll be sure to do that. Is there a tentative release date for 4.13?

On Wed, Jul 8, 2015 at 12:39 AM, Marc Philipp [email protected]
wrote:

Squashed and merged. Thank you very much, @rschmitt
https://github.com/rschmitt!

@rschmitt https://github.com/rschmitt Can you please add a section
about the two new methods to the release notes
https://github.com/junit-team/junit/wiki/4.13-Release-Notes?

—
Reply to this email directly or view it on GitHub
#1154 (comment).

[rschmitt]

Release notes updated.

[kcooney]

FYI: we are proposing deleting expectThrows() and changing assertThrows() to return the caught exception. If you have objections or concerns, please comment on pull #1396