junit-team / junit4

A programmer-oriented testing framework for Java.
https://junit.org/junit4
Eclipse Public License 1.0
8.52k stars 3.26k forks source link

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

Closed Tibor17 closed 7 years ago

Tibor17 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

@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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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

Tibor17 commented 10 years ago

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

marcphilipp commented 10 years ago

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

stefanbirkner commented 10 years ago

@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 commented 10 years ago

@Tibor17 Does Vallado solve your problem?

Tibor17 commented 10 years ago

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

kcooney commented 10 years ago

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 commented 7 years ago

Fixed in #1154