Skip to content

Add Assert#expectThrows #1154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 15 commits into from
42 changes: 42 additions & 0 deletions src/main/java/org/junit/Assert.java
Original file line number Diff line number Diff line change
Expand Up @@ -955,4 +955,46 @@ public static <T> void assertThat(String reason, T actual,
Matcher<? super T> matcher) {
MatcherAssert.assertThat(reason, actual, matcher);
}

/**
* This interface facilitates the use of expectThrows from Java 8. It allows
* method references to void methods (that declare checked exceptions) to be
* passed directly into expectThrows without wrapping. It is not meant to be
* implemented directly.
*/
public interface ThrowingRunnable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the junit has still the same tendency to use nested interfaces and nested annotations like Parameter.
It's really awful code like Assert.ThrowingRunnable and using static import for Assert.* and much easier to use and import ThrowingRunnable. Bad example is to design API like Parameterized.Parameter.
The only reason why nested classes is that they are not static and share one instance; otherwise it's useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason you need a star import. You can just static import Assert.ThrowingRunnable. Whether or not this interface should be extracted into a top-level file is up to the maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I dont think so it's thier decision. This is opensource and code review. The reviewer can be anyone.
The argument is that nested classes share instance; otherwise they are useless. Nested annotations/interfaces are really hard to use if you want to have nice and meaningful code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can comment all you want on matters of code organization, and in accordance with Parkinson's Law of Triviality, people commonly do. My point is that the maintainers (which is not a group I belong to) have the best understanding of what the relevant convention is (if any) for new code, and what conditions would be sufficient for breaking that convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are cases where static nested classes make sense as a kind of organization tool to avoid an explosion of unrelated top-level classes in a package (see, for example, Map.Entrry).

I personally haven't decided if ThrowingRunnnable should be a top-level class. If we think it will be used elsewhere (perhaps ErrorCollector) then it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ThrowingRunnnable is a bit of a special case because it's just a shim. It gives us a place to hang the specific functional type signature we want to accept. The interface itself is only intended to be implemented by the compiler; it's not really part of an API or SPI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet extension developers will use it when they develop more complicated APIs that reuse our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it seems like there's no way to "encapsulate" this interface. We won't be able to move, eliminate, or rename it later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcphilipp do you have thoughts about whether this should be a top-level class? If we need it in another class, or if API developers want to use our code, then they will either need to import a nested class or introduce their own version. I don't have a strong feeling, but wanted to make sure you saw this thread.

void run() throws Throwable;
}

/**
* Asserts that {@code runnable} throws an exception when executed. If it
* does, the exception object is returned. If it does not, an
* {@link AssertionError} is thrown.
*
* @param runnable A function that is expected to throw an exception when executed
* @return The exception thrown by {@code runnable}
*/
public static Throwable expectThrows(ThrowingRunnable runnable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the latest diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return expectThrows(null, runnable);
}


/**
* Asserts that {@code runnable} throws an exception when executed. If it
* does, the exception object is returned. If it does not, an
* {@link AssertionError} is thrown with the given message.
*
* @param message the identifying message for the {@link AssertionError}
* @param runnable A function that is expected to throw an exception when executed
* @return The exception thrown by {@code runnable}
*/
public static Throwable expectThrows(String message, ThrowingRunnable runnable) {
try {
runnable.run();
} catch (Throwable t) {
return t;
}
fail(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok:

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

throw new AssertionError(); // This statement is unreachable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the above two lines can be replaced by

throw new AssertionError(message);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Well, that's true now, since message can't be null.

}
}
53 changes: 52 additions & 1 deletion src/test/java/org/junit/tests/assertion/AssertionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.expectThrows;
import static org.junit.Assert.fail;

import java.math.BigDecimal;

import org.junit.Assert;
import org.junit.Assert.ThrowingRunnable;
import org.junit.ComparisonFailure;
import org.junit.Test;
import org.junit.internal.ArrayComparisonFailure;
Expand Down Expand Up @@ -170,7 +172,7 @@ public void oneDimensionalDoubleArraysAreNotEqual() {
public void oneDimensionalFloatArraysAreNotEqual() {
assertArrayEquals(new float[]{1.0f}, new float[]{2.5f}, 1.0f);
}

@Test(expected = AssertionError.class)
public void oneDimensionalBooleanArraysAreNotEqual() {
assertArrayEquals(new boolean[]{true}, new boolean[]{false});
Expand Down Expand Up @@ -674,4 +676,53 @@ public void assertNotEqualsIgnoresDeltaOnNaN() {
public void assertNotEqualsIgnoresFloatDeltaOnNaN() {
assertNotEquals(Float.NaN, Float.NaN, 1f);
}

@Test(expected = AssertionError.class)
public void expectThrowsRequiresAnExceptionToBeThrown() {
expectThrows(nonThrowingRunnable());
}

@Test
public void expectThrowsIncludesNoMessageByDefault() {
try {
expectThrows(nonThrowingRunnable());
fail();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that fail() throws an AssertionError, so you need to move the fail() call outside of the try block and put a return after the assertNull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nuts, this is the second time I've screwed this up.

} catch (AssertionError ex) {
assertNull(ex.getMessage());
}
}

@Test
public void expectThrowsIncludesTheSuppliedMessage() {
try {
expectThrows("message", nonThrowingRunnable());
fail();
} catch (AssertionError ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, one common convention is to name the exception "expected" to make it 100% clear that you expect it to be thrown. See https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s6.2-caught-exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd rather give that name to the parameter that is currently called throwableClass. It makes more sense in this context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

assertEquals("message", ex.getMessage());
}
}

@Test
public void expectThrowsReturnsTheSameObjectThrown() {
NullPointerException npe = new NullPointerException();

Throwable throwable = expectThrows(throwingRunnable(npe));

assertSame(npe, throwable);
}

private static ThrowingRunnable nonThrowingRunnable() {
return new ThrowingRunnable() {
public void run() throws Throwable {
}
};
}

private static ThrowingRunnable throwingRunnable(final Throwable t) {
return new ThrowingRunnable() {
public void run() throws Throwable {
throw t;
}
};
}
}