Junit5: Introduce Supplier variants of timeout assertions

[nickgrealy]

To whom it may concern,

I'd like to be able to have a test, which fails if a particular code snippet takes longer than a certain amount of time to execute.

As far as I can tell, there is currently only the ability to set a timeout at the test level... however I feel this has the undesirable side effect of validating the execution time of the test code... I'd like to test only the execution code.

More than happy to complete a pull request - depending on whether this behaviour is already supported and whether lambdas are supported.

An example:

Given the following timeout method

/**
 * Ensures a function completes within the given timeout.
 */
public static <Result> Result timeout(final long timeoutMillis, Supplier<Result> invoke){
    long start = System.currentTimeMillis();
    Result result = invoke.get();
    long duration = System.currentTimeMillis() - start;
    if (duration > timeoutMillis) {
        Assert.fail(String.format("Actual time %s exceeded timeout %s (millis).", duration, 
                timeoutMillis));
    }
    return result;
}

And the sayHello method takes 1000 milliseconds to return
When I test the sayHello method

@Test
public void testSomeMethod(){
    String greeting = timeout(500, () -> greetingService.sayHello("Nick"));
    assertEquals("Hello Nick", greeting);
}

Then the test should fail with the message Actual time 1000 exceeded timeout 500 (millis).

[jbduncan]

Hi @nickgrealy, please correct me if I've misunderstood what you're requesting, but I believe this has actually been implemented already in JUnit 5 via Assertions.assertTimeout and Assertions.assertTimeoutPreemptively.

I'm wondering if your confusion is coming from the fact that the documentation you linked applies to JUnit 4 rather than JUnit 5? :smiley:

[nickgrealy]

Hi @jbduncan, no I hadn't seen that (-> assertTimeout)... looks good!

I'll just preface the next statement with: I haven't used the functional interface Executable yet...

Can we improve the existing code, by adding additional method signature(s), to use a Supplier<Result> and return the supplied object? That way we can:

• supply method references (is this possible with Executable? e.g. someClass::someMethod)
• make assertions on the method's return value.

(N.B. I'm not sure how Java will handle the similar functional interface signatures, if they clash perhaps we could put them in separate classes?)

e.g.

static <Result> Result assertTimeout(Duration timeout, Supplier<Result> executable) { ... }

[sbrannen]

Hi guys,

I was gonna say that I already implemented assertions for timeouts in JUnit Jupiter, but @jbduncan beat me to it. 😉

@nickgrealy, regarding support for Supplier<T> in addition to Executable, that's definitely worth considering.

Though we would likely implement the alternative to the existing assertTimeout() as follows.

static <T> T assertTimeoutWithResult(Duration timeout, Supplier<T> supplier) { ... }

Similarly, the remaining method would likely be something like assertTimeoutWithResultPreemptively().

I will therefore rename this issue's title accordingly

Regards,

Sam

[sbrannen]

@junit-team/junit-lambda, let's discuss how to name such methods.

[nickgrealy]

@sbrannen - awesome!

There are only two hard things in Computer Science: cache invalidation and naming things (, and off-by-one errors).

Let me know if I can help implement the solution. I'm sure it's trivial - but I'd love to have contributed to Junit5!

[sbrannen]

Ahhhh, yes... one of my favorite CS jokes. 😉

[sbrannen]

Let me know if I can help implement the solution. I'm sure it's trivial - but I'd love to have contributed to Junit5!

You're right: it is rather trivial, and normally we just implement stuff like that ourselves in order to save time. However, since you feel so inclined, sure.... give it a shot. Please just use the existing code to guide you in your solution and documentation. Also, make sure to write appropriate unit tests and to update the release notes for M4 accordingly.

Have fun!

[sbrannen]

Assigned to @nickgrealy and tagged as _in progress_.

[sbrannen]

Please note that Supplier is insufficient for our purposes.

We will therefore need a ThrowingSupplier analogous to our existing ThrowingConsumer which resides in the org.junit.jupiter.api.function package.

[marcphilipp]

@junit-team/junit-lambda, let's discuss how to name such methods.

Shouldn't we first decide on a name?

[marcphilipp]

I think overloading the methods would be fine, no need for a WithResult suffix.

Other opinions?

[sbrannen]

Shouldn't we first decide on a name?

Well.. it wouldn't hurt. 😇

All kidding aside: renaming the methods ex post facto wouldn't be that bad, but deciding up front is likely better.

[sbrannen]

I think overloading the methods would be fine, no need for a WithResult suffix.

True. I actually just implemented it as a proof of concept, and overloading works totally fine. 👍

    @Test
    void test() {
        String foo = assertTimeout(ofMillis(50), () -> {
            // do something that takes a bit of time...
            return "foo";
        }, () -> "Tempus" + " " + "Fugit");

        assertEquals("foo", foo);
    }

[sbrannen]

So, this is the signature/implementation of one of the simplistic timeouts in Assertions:

public static <T> T assertTimeout(Duration timeout, ThrowingSupplier<T> supplier, Supplier<String> messageSupplier) {
    return AssertTimeout.assertTimeout(timeout, supplier, messageSupplier);
}

And I'll leave the rest of the exercise to @nickgrealy.

[nickgrealy]

@sbrannen - quick question: I'm just implementing the assertTimeoutPreemptively method, and I want to capture the result OR the caught exception... the existing Future was only expecting a Throwable. i.e.

Future<Throwable> future = executorService.submit(() -> {
    try {
        return supplier.get();
    } catch (Throwable ex) {
        return ex;
    }
    return null;
});

How do you want me to tackle this one?

1. Create a class/pojo/container, to hold an Optional<T> result and Optional<Throwable> exception, and have Future<ResultOrException> future = ... (which package and what name would you want?)
2. Or... ?

[nickgrealy]

Hi @sbrannen,

I've pushed some changes into my own fork/branch for now, can you please review them before I submit a pull request?

Of particular note:

1. AssertionsDemo.java - converted tabs from tab character to 4 spaces (was this the spotlessApply task & should I revert?)
2. Assertions.java - Added <P> tags (was this the spotlessApply task & should I revert?)
3. AssertionsAssertTimeoutTests.java - would you like these tests re-written as a Parameterized test?
4. AssertTimeout.java - The ResultOrException inner class - are you happy with this solution?

Thanks,
Nick

[sbrannen]

Hi @nickgrealy,

I unfortunately don't have any spare time at the moment.

@junit-team/junit-lambda, maybe one of you can provide some guidance here?

[marcphilipp]

AssertionsDemo.java - converted tabs from tab character to 4 spaces (was this the spotlessApply task & should I revert?)

No, that was most probably your IDE. This file is mostly ignored by Spotless because there's a // @formatter:off immediately after the package statement. Can you please revert the whitespace changes?

Assertions.java - Added <P> tags (was this the spotlessApply task & should I revert?)

Nope, that wasn't Spotless either. In fact the build now produces a large number of warnings, e.g.

:junit-jupiter-api:javadoc
/Users/marcp/Repositories/junit5/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java:21: warning: empty <p> tag
 * <p>
   ^
[...]
/Users/marcp/Repositories/junit5/junit-jupiter-api/src/main/java/org/junit/jupiter/api/Assertions.java:32: warning: empty <p> tag
 * <p>
   ^
31 warnings

AssertionsAssertTimeoutTests.java - would you like these tests re-written as a Parameterized test?

I think we can live with the duplication for now. Especially since we don't have parameterized test support in Jupiter, yet.

AssertTimeout.java - The ResultOrException inner class - are you happy with this solution?

I think this version is easier and does not need ResultOrException:

static <T> T assertTimeoutPreemptively(Duration timeout, ThrowingSupplier<T> supplier,
        Supplier<String> messageSupplier) {
    ExecutorService executorService = Executors.newSingleThreadExecutor();

    try {
        Future<T> future = executorService.submit(() -> {
            try {
                return supplier.get();
            } catch (Throwable throwable) {
                throw ExceptionUtils.throwAsUncheckedException(throwable);
            }
        });

        long timeoutInMillis = timeout.toMillis();
        try {
            return future.get(timeoutInMillis, TimeUnit.MILLISECONDS);
        }
        catch (TimeoutException ex) {
            throw new AssertionFailedError(
                buildPrefix(nullSafeGet(messageSupplier)) + "execution timed out after " + timeoutInMillis + " ms");
        }
        catch (ExecutionException ex) {
            throw ExceptionUtils.throwAsUncheckedException(ex.getCause());
        }
        catch (Throwable ex) {
            throw ExceptionUtils.throwAsUncheckedException(ex);
        }
    }
    finally {
        executorService.shutdownNow();
    }
}

@nickgrealy Do you have time to make these changes?

[nickgrealy]

@marcphilipp - thanks for the feedback. I sure do, will make the changes today.

[nickgrealy]

Hi @jbduncan, @marcphilipp, @sbrannen,

I've made the changes as suggested (I've also reverted the import re-ordering on AssertionsDemo.java)... is there anything else you'd like changed?

1. squash the commits?
2. submit a pull request?
3. sign a JUnit Contributor License Agreement?

[jbduncan]

Hi @nickgrealy.

Thank you for taking the time to do this. I'm impressed by the cleanliness of your code, so well done!

Please have a look at https://github.com/nickgrealy/junit5/commit/5d9526ae4d1a74d57fa30a8b3bed9b09de78050c#commitcomment-20774063, where I made some final review comments on both minor nitpicks and small javadoc issues I found in the code.

If you were to fix those, then that would completely satisfy me!

As for squashing commits & raising a PR, I'm sure the JUnit team would be able to guide you on that.

With the JUnit Contributor License Agreement, as long as you agree with the terms in CONTRIBUTING.md and put the following text at the bottom of your PR body, then that should be all you need to do to "sign" it. :)

I hereby agree to the terms of the JUnit Contributor License Agreement.

[nickgrealy]

@jbduncan - thanks for the feedback! Changes have been made.

[jbduncan]

Thanks, looks good to me now. :)

[jbduncan]

Oh very sorry @nickgrealy, I just noticed two other areas for improvement which I missed the first time round.

1. Would you kindly change these lines of javadoc so that the "executable" keywords are surrounded with {@code [...]} brackets, for consistency with {@code supplier} blocks in the assertTimeout*(*) methods you introduced?
   (Would you do this for all the executable-based assertTimeout methods.)
2. Would you also introduce @see org.junit.jupiter.api.Assertions#assertTimeout(Duration, ThrowingSupplier) under this line in ThrowingSupplier, so that people know a main place where ThrowingSuppliers are used?

[nickgrealy]

@jbduncan - thanks, changes have been made.

[jbduncan]

Thank you very much for bearing with me @nickgrealy - now your changes look _very_ good to me. :)

[marcphilipp]

@nickgrealy Don't worry about squashing. We can do that when we merge. Please go ahead and submit a pull request. Thanks!

[nickgrealy]

All (@jbduncan, @marcphilipp, @sbrannen), thanks for that. It was far less painful then I anticipated!