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

New Method Proposal: ExpectedException#expect(Throwable, Callable) #706

Closed
Tibor17 opened this issue Jul 20, 2013 · 20 comments

Comments

Projects
None yet
6 participants
@Tibor17
Copy link
Contributor

commented Jul 20, 2013

Many times I needed to assert a block of code in test-method to throw an exception, and check how the throwing block has changed values.
I had to use try-catch.

So I would like to implement new method with such of signature ExpectedException#expect(Throwable, Callable).

Usage:

Object obj = expect( Exception.class , new Callable() { ..do and return something.. });

Usage of Lambda in Java 8:

expect( Exception.class , () -> { ..do and return something.. } )

@dsaff

This comment has been minimized.

Copy link
Member

commented Jul 23, 2013

Although I contributed to ExpectedException, I have to admit that I personally just use try/catch in most of my tests that involve exceptions. Agreed that once Java 8 is widely usable, the syntax becomes much more appealing.

@stefanbirkner

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2013

I've done something similar for stefanbirkner/system-rules#5. Just one difference: I created a new interface Assertion with a no-args void method checkAssertion(): http://www.stefan-birkner.de/system-rules/#ExpectedSystemExit

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2013

@stefanbirkner the rule is different. If you or somebody else already implemented my Rule, then the things would be more simple to commit to JUnit.
@dsaff Having try/catch in a test code is a kind of duplicate to the functionality that the framework has. I can almost write a console application instead. If I remember well, the try/catch block was not the best practice in unit tests. That's the reason why I submitted matchers in junit-contrib, but this extension in @rule would be less complex and more useful.

@dsaff

This comment has been minimized.

Copy link
Member

commented Jul 24, 2013

@Tibor17, to be precise, we've tried, twice (with @test(expected=Foo.class) and ExpectedException) to duplicate functionality that Java's already had since its birth. In both cases, I've found eventually that the framework solutions are harder to read and reason about for large teams, and are just as easy to use mistakenly, as vanilla try/catch. If there's a best-practices document that says differently, it would be interesting to talk to the author and understand how their experience differs.

In this case, the method you suggest is easily implemented as a static method in a team's code repository without any changes to JUnit. Again, when Java 8 is widely used, I can then see that the savings in typing would merit considering something like this as something to bless as an "official" JUnit extension.

Although in my Java 8 meanderings, I tend to use a thrown() method that takes a block and returns any exceptions thrown, so that you can assert anything you want about the exception:

assertEquals(Exception.class, thrown(() -> foo()).getClass());

or:

assertEquals("yikes!", thrown(() -> foo()).getMessage());

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2013

@dsaff
I was busy with ParallelComputer in maven-surefire. Released in 2.16.

It is easy to say how the assert would look like now, but we have to talk about real calls next year when the Java 8 will be out. There is #722 with some examples. As you can see I am not the only who requested.

Most probably there will be discussions about returned values out of the block, assert signatures, Matchers for classes, Matchers for exception messages, etc.

Perhaps Hamcrest will take care of new assertions next year, who knows.

With the current status of Java 8 Lamba expressions you can already design pure Java signatures for assertThat:
assertThat(Callable) and assertThat(Runnable), e.g.
assertThat(Excaption.class, () -> foo());
assertEquals("yikes!", thrown(() -> foo()).getMessage());
The ExpectedException.expect(Exception.class, () -> method()) still makes sense .

@lukaseder

This comment has been minimized.

Copy link

commented May 7, 2014

I was just wishing for the same! How about:

assertThrows(Exception.class, () -> foo());
assertThrows(Exception.class, () -> foo(), e -> assertEquals("yikes!", e.getMessage()));

Note that unlike @Tibor17, I'm suggesting a third argument that allows for adding additional checks on the Exception. I.e. we would have the following signatures:

@FunctionalInteface
interface ThrowableRunnable {
    void run() throws Throwable;
}

static void assertThrows(
    Class<? extends Throwable> throwable, 
    ThrowableRunnable runnable
) {}

static <T extends Throwable> void assertThrows(
    Class<Throwable> throwable, 
    ThrowableRunnable runnable, 
    Consumer<T> exceptionConsumer
) {}

Note also that neither Callable nor Runnable are really suited for this use-case. Neither of them allows for throwing Throwable

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

@lukaseder
The consumer is something I don't know.
I think we need these three use cases with Java Hamcrest:
assertThat(() -> {do something}, MyException.class)
assertThat(() -> {do something}, Matcher<Throwable>)
<T> assertThat(() -> {do something}, Matcher<T>) because of Callable<T> returns a value.
This means assert thrown exception and assert returned value.

Callable#call() throws Exception. That's fine Java API design because if you suppose an expected exception like RuntimeException or an Error, then these two subtypes are unchecked and therefore they are not mentioned in the signature of Callable#call() and still can be thrown or caught or expected.
This means that we can still use Callable because the user can throw any exception and we don't need to create new class.
Normally Throwable are not used in method signature. The people use java.lang.Exception or some subtype. Instead the Throwable is used only in catch block which only simplifies handling of all exceptions. Every exception extends from Throwable.

@lukaseder

This comment has been minimized.

Copy link

commented May 7, 2014

The consumer is something I don't know.

http://docs.oracle.com/javase/8/docs/api/java/util/function/Consumer.html
This was just an example. It may make sense not to depend on JDK 8 API yet.

That's fine Java API design because if you suppose an expected exception like RuntimeException or an Error

RuntimeException extends Exception...

I agree with the part about Error, but nothing prevents users from extending Throwable directly. This is done quite often in Scala, for instance. Or there's org.junit.Test.None. It's an edge-case, agreed, but it would be too bad not to allow reasoning about Throwable types, just to "save" a type by reusing Callable.

Note, I haven't given all of this too much thought, and I'm not well acquainted with Hamcrest, so your assessment may well be more reasonable in a broader context.

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

I agree with using Throwable.
We cannot use Java 8 java.lang.FunctionalInteface yet.
Can we make suggestions of what new methods we want?
Proposing these assertion methods:
ExpectedException.expect(Class<? extends Throwable> c, ThrowableRunnable r)
assertThrows(Class<? extends Throwable> c, ThrowableRunnable r)
<T extends Throwable> assertThrows(Class<Throwable> c, Runnable<T> exceptionConsumer, ThrowableRunnable r)
assertThrows(Matcher<Throwable> matcher, ThrowableRunnable r)
<T> assertReturns(Matcher<T>, ThrowableFunction<T> f)
Interface ThrowableRunnable has method void run() throws Throwable.
Interface ThrowableFunction<T> has method T run() throws Throwable.

@marcphilipp

This comment has been minimized.

Copy link
Member

commented May 7, 2014

I like the general idea! We just need to get the details right... ;-)

For instance, I'm not very fond of the name ThrowableRunnable but do not have a better suggestion either. Maybe ThrowingRunnable? Other suggestions?

@Tibor17 How would you use ExpectedException.expect(Class<? extends Throwable> c, ThrowableRunnable r)? It does not make sense to have it as a rule then or am I missing something?

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2014

I forgot to mention assertNotThrows().
The tests will discover generics detils.
Yes ThrowingRunnable is even better.
@marcphilipp Example with ExpectedException: test method has two statements and both may throw same exception but only the last statement is expeced to throw it.

@marcphilipp

This comment has been minimized.

Copy link
Member

commented May 12, 2014

@Tibor17 I am well aware of the benefits of using the rule ExpectedException. However, I don't understand how you want to use it along with the method you are proposing. Can you please give us an example?

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2014

@marcphilipp
yes, we can remove ExpectedException from the list.

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2014

@marcphilipp
Is there anything we need to do before implementing?

@marcphilipp

This comment has been minimized.

Copy link
Member

commented Jun 20, 2014

No, I don't think so. Feel free to go ahead and submit a pull request.

@stefanbirkner

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2014

@Tibor17 Sorry, that I'm entering the discussion only now. A few weeks ago I created a library named Vallado. If I understand your concern right, then this library solves it.

assertEquals(1, objectUnderTest.property);
when(() -> objectUnderTest.doSomething()).thenA(RuntimeException.class).isThrown();
assertEquals(2, objectUnderTest.property);

With Java 8 (or with an anonymous class) statements could be executed directly within the test method. You don't need to implement a rule interface. Hence you can create an own microlibrary for your concern that is independent from the JUnit framework and therefore usable by other test frameworks like TestNG, too.

@stefanbirkner

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2014

@Tibor17 Does Vallado solve your problem?

@Tibor17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

@stefanbirkner
We already like to use JUnit Assert class.
Vallado syntax with Fluent Objects is maybe useful in BDD.

@kcooney

This comment has been minimized.

Copy link
Member

commented Aug 27, 2014

Not quite sure why a fluent interface would only be useful in BDD. Guice, Truth and Guava all have classes with fluent interfaces.

I think the syntax that @stefanbirkner solves a few of the more annoying problems of the other solutions. With ExpectedException it isn't always clear to people that no code is executed after the method that throws the exception. Specifying exceptions in the @Test annotation doesn't limit the scope of what method call causes the exception.

I personally would rather see what other frameworks end up using for specifying that an assertion is thrown and making assertions on the properties of the exception, and then if there's an obvious winner, either endorse it or integrate a similar API in JUnit. JUnit doesn't need to solve everyone's assertion problems.

@kcooney

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

Fixed in #1154

@kcooney kcooney closed this Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.