Skip to content
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

Add Assert#expectThrows #1154

Closed
wants to merge 15 commits into from
71 changes: 71 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,75 @@ 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 <T extends Throwable> T expectThrows(ThrowingRunnable runnable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Choose 1 . with Generics and then ThrowingRunnable and without cast,
or without Generics . IMHO the purpose is to catch an exception and in most cases it is whatever exception. If I am lazy developer I would not care about super class/exception.

return (T) expectThrows(null, Throwable.class, runnable);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cast is checked at runtime, just like an explicit downcast from 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 with the given {@code
* 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 <T extends Throwable> T expectThrows(String message, 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.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return (T) expectThrows(message, Throwable.class, runnable);
}

/**
* Asserts that {@code runnable} throws an exception of type {@code throwableClass} when
* executed. If it does, the exception object is returned. If it does not throw an exception, an
* {@link AssertionError} is thrown. If it throws the wrong type of exception, an {@code
* AssertionError} is thrown describing the mismatch.
*
* @param throwableClass the expected type of the exception
* @param runnable A function that is expected to throw an exception when executed
* @return The exception thrown by {@code runnable}
Copy link
Member

Choose a reason for hiding this comment

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

Please add @since 4.13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If 4.13 doesn't happen, will this be changed to @since 5.0 by a regular expression or something? I think I saw this come up in another PR or source branch.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we will update all of the @since 4.13 to @since 5.0 if we decide to skip 4.13.

Note one of the other maintainers might request to just have this branch target the 5.0 branch.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is only really usable in the presence of JDK 8 and lambda expressions, I believe this should end up in our planned Java 8 extensions project which is codenamed junit-lambda. We've already created the repo on GitHub but the project is not set up to accept pull requests yet.

Copy link
Member

Choose a reason for hiding this comment

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

@marcphilipp I actually disagree we should target these changes to JUnitLambda, for two reasons. Firstly, we don't have a timeframe for the JDK 8 release. Secondly, we are deprecating Hamcrest APIs in 4.13/5.0, making ExpectedException much less useful. The API in this pull could be used by JUnit users that are already on JDK 8, without waiting for us to come up with other Lambda APIs.

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

/**
* Asserts that {@code runnable} throws an exception of type {@code throwableClass} when
* executed. If it does, the exception object is returned. If it does not throw an exception, an
* {@link AssertionError} is thrown with the given {@code message}. If it throws the wrong type
* of exception, an {@code AssertionError} is thrown describing the mismatch.
*
* @param message the identifying message for the {@link AssertionError}
* @param throwableClass the expected type of the exception
* @param runnable A function that is expected to throw an exception when executed
* @return The exception thrown by {@code runnable}
*/
public static <T extends Throwable> T expectThrows(String message, Class<T> throwableClass, ThrowingRunnable runnable) {
try {
runnable.run();
} catch (Throwable t) {
String mismatchMessage = String.format("Expected %s to be thrown, but got %s instead",
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid the formatting by calling isInstance true. It wouldn't be a large gain compared to the cost of the exception handling, but it's probably worth it.

Also, I suggest "expected %s to be thrown, but %s was thrown", and prepend "message" if it's not null, like we do in Assert.format(String, Object, Object)

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 don't understand this remark. What about isInstance?

Copy link
Member

Choose a reason for hiding this comment

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

Sigh. I really dislike how hard it is to follow comments on anything but the latest commit in github. :-(

My point was that mismatchMessage only needs to be constructed if there's a mismatch.

throwableClass.getSimpleName(), t.getClass().getSimpleName());
assertTrue(mismatchMessage, throwableClass.isInstance(t));
return (T) 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.

}
}
95 changes: 87 additions & 8 deletions src/test/java/org/junit/tests/assertion/AssertionTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
package org.junit.tests.assertion;

import org.junit.Assert;
Copy link
Member

Choose a reason for hiding this comment

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

we put static imports before non-static ones. See
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html

when in doubt, don't reorder imports.

Copy link
Member

Choose a reason for hiding this comment

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

@rschmitt please fix the imports to match the previous ordering

import org.junit.Assert.ThrowingRunnable;
import org.junit.ComparisonFailure;
import org.junit.Test;
import org.junit.internal.ArrayComparisonFailure;

import java.io.IOException;
import java.math.BigDecimal;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
Expand All @@ -11,15 +20,9 @@
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.ComparisonFailure;
import org.junit.Test;
import org.junit.internal.ArrayComparisonFailure;

/**
* Tests for {@link org.junit.Assert}
*/
Expand Down Expand Up @@ -170,7 +173,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 +677,80 @@ 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) {
assertEquals("message", ex.getMessage());
}
}

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

Throwable throwable = expectThrows(throwingRunnable(npe));

assertSame(npe, throwable);
}

@Test(expected = ClassCastException.class)
Copy link
Member

Choose a reason for hiding this comment

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

I personally think throwing ClassCastException isn't very useful. You have to dig through the code (and possibly the implementation of expectThrows()) to understand why this was thrown. If the caller instead had to pass the exception type, the caller would always get a clear exception 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.

Now that I think about it, we should be more concerned about losing the exception that actually was caught. That's the main piece of information you would need to debug a problem in the underlying SUT. (Note that fixing this is orthogonal to whether the expected exception type is specified/required.)

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the exception that was caught could be a checked exception, so if an unexpected exception is caught, we would need to do wrapping at least for checked exceptions.

How about we if we catch an unexpected exception, we make that exception the cause of the AssertionFailure that we throw?

Edit: Note that if we catch an unexpected unchecked exception or an unexpected error, we could just rethrow it, but then the behavior of the API would be hard to predict. And yes, I know there are hacks that allow you to throw checked exceptions from methods that don't declare them, but I'd rather not use them here :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've already done that. You can throw an undeclared checked exception using a technique like Lombok uses to implement sneakyThrow, but wrapping in an AssertionError is the right thing to do if you want inline type assertions.

public void expectThrowsDetectsTypeMismatchesViaAssignment() {
NullPointerException npe = new NullPointerException();

IOException ioException = expectThrows(throwingRunnable(npe));
}

@Test(expected = AssertionError.class)
public void expectThrowsDetectsTypeMismatchesViaExplicitTypeHint() {
NullPointerException npe = new NullPointerException();

expectThrows(IOException.class, throwingRunnable(npe));
}

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

try {
expectThrows(IOException.class, throwingRunnable(npe));
fail();
} catch (AssertionError error) {
assertEquals("Expected IOException to be thrown, but got NullPointerException instead",
error.getMessage());
}
}

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;
}
};
}
}