Assertj-core: assertThat with CompletionStage is not waiting CompletableFuture to complete

[almothafar]

Summary

I'm trying to do some test with firebase SDK things, the problem that the assertThat always failing with message:

[error] Expecting
[error] [error] to be completed.
[error] Be aware that the state of the future in this message might not reflect the one at the time when the assertion was per
formed as it is evaluated later on, took 0.145 sec

and I'm not sure how to make it wait, I can do just a stupid solution with .join() to make it work, but really it is not something that I'm looking for with this case.

Example

I have the following method of my service:

    /**
     * @param email
     * @return token
     */
    public CompletableFuture<String> getCustomToken(String email) {
        CompletableFuture<String> future = new CompletableFuture<>();
        ApiFuture<UserRecord> emailAsync = firebaseAuthProvider.get().getUserByEmailAsync(email);

        emailAsync.addListener(() -> {
            try {
                UserRecord userRecord = emailAsync.get();
                if (userRecord != null && userRecord.getUid() != null) {
                    ApiFuture<String> customTokenAsync = firebaseAuthProvider.get().createCustomTokenAsync(userRecord.getUid());
                    customTokenAsync.addListener(() -> {
                        try {
                            future.complete(customTokenAsync.get());
                        } catch (InterruptedException | ExecutionException e) {
                            future.completeExceptionally(e);
                        }
                    }, ec.current());
                } else {
                    future.completeExceptionally(new BusinessException(MsgKeys.ERR_USER_UID_NOT_FOUND));
                }
            } catch (InterruptedException | ExecutionException exception) {
                logger.error(exception.getMessage());
                future.completeExceptionally(new BusinessException(MsgKeys.ERR_VALIDATE_TOKEN_FAILED, exception.getCause()));
            }
        }, ec.current());
        return future;
    }

Then This is the test method:

    @Test
    public void testLoginWithCustomTokenMustFailWithVerifyIdToken() {
        logger.info("Testing login, we will create a custom token then verify it, it should be success until reach verifyIdToken and fail, because we can only verifyIdToken with " +
                "tokens from frontend");
        String email = config.getString("firebase.test.email");
        CompletionStage<String> tokenFuture = firebaseAuthService.getCustomToken(email);
        logger.info("Testing creating custom token for email " + email);

        assertThat(tokenFuture).isCompletedWithValueMatching(token -> {
            JsonNode jsonNode = Json.newObject().put("token", token);
            logger.info("Trying to verify token " + token);

            exception.expectCause(allOf(Matchers.instanceOf(FirebaseAuthException.class), hasProperty("message", startsWith("verifyIdToken() expects an ID token, but was given a custom token"))));
            Http.RequestBuilder request = Helpers.fakeRequest()
                    .method(POST)
                    .bodyJson(jsonNode)
                    .uri("/api/user/firebaseLogin");

            Result route = route(app, request);
            return route.status() == 200;
        }, "Creating custom token for " + email);

    }

But when I replace CompletionStage<String> tokenFuture = firebaseAuthService.getCustomToken(email); with CompletionStage<String> tokenFuture = CompletableFuture.completedFuture(firebaseAuthService.getCustomToken(email).join()); it will work.

Is it my code issue, or test not waiting correctly, or am I missing something?

Evironment:

Java 8 & 10:

java version "1.8.0_162"
Java(TM) SE Runtime Environment (build 1.8.0_162-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.162-b12, mixed mode)

java 10 2018-03-20
Java(TM) SE Runtime Environment 18.3 (build 10+46)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10+46, mixed mode)

Play Framework (2.6.x): https://www.playframework.com/documentation/2.6.x/JavaFunctionalTest
SBT: 0.13.15
AssertJ 3.9.1: libraryDependencies += "org.assertj" % "assertj-core" % "3.9.1" % Test

[joel-costigliola]

The isCompletedWithValueMatching assertion javadoc states that the future under test should be completed thus the error.

It looks like the future assertion API is not as useful as it should (see also this long discussion in https://github.com/joel-costigliola/assertj-core/issues/1149 and https://github.com/joel-costigliola/assertj-core/issues/1150).

The one thing I'm not convinced that having assertion with side effects on the object under test is a good thing, I believe that is user responsibility to make sure the object under test is in the right state for validation.

In your case you are asking AssertJ to join / get the future for you so that you can test its result but what if the future completes exceptionally ? or takes a very long time ? In that latter case a timeout is likely to be needed ... not as straightforward as it looks.

I'm alright enriching the API maybe with something like:
java assertThat(tokenFuture).completesWithin(timeout, unit) .hasValueMatching(somePredicate) .hasValueSatisfying(requirements);
Having said that completesWithin(timeout) is something users can do themselves, it's not really assertion related.

Thoughts and feedback welcome.

[jypma]

We ran into the same thing and ended up writing our own CompletionStageAssert, with which one can e.g. write

  assertThat(tokenFuture).succeedsWith(Token.create("hello"))

and

  Token success = assertThat(tokenFuture).succeeds();

or even

  assertThat(tokenFuture).success().matches(aRealToken)

This does a global timeout internally, but you can configure it using assertThat(tokenFuture).within(3, SECONDS).succeeds()).

I'd be happy to submit a PR to assertj-core if you think it's an improvement. The global default timeout wouldn't need to be as hardcoded as it currently is.

[stovocor]

I'm alright enriching the API maybe with something like:

assertThat(tokenFuture).completesWithin(timeout, unit)
                       .hasValueMatching(somePredicate)
                       .hasValueSatisfying(requirements);

As for the argument for "completesWithin" I would prefer java.time.Duration.

[jypma]

@joel-costigliola Would it be OK if I created a PR with the DSL I suggested earlier? I think by having the DSL include .success() and .failure(), returning explicit other assertion objects, there's a bit more re-use and less new words introduced into the language.

[joel-costigliola]

@jypma this one has fallen under my radar. Let me think about it a bit before I'll get back to you.

[oscarfh]

Any news on this?

[joel-costigliola]

sorry will have a look at this for the next release.

[joel-costigliola]

Sorry for the long wait, here are my current thoughts:

assertThat(tokenFuture).completesWithin(duration)  // and/or completesWithin(timeout, unit)
                       .hasValueMatching(somePredicate)
                       .hasValueSatisfying(requirements);

or alternatively returning assertions on the future's result (since the future completed)

assertThat(tokenFuture).completesWithin(duration) // and/or completesWithin(timeout, unit)           
                       .matches(somePredicate)
                       .satisfies(requirements);

same assertions for failed future returning assertions on the future's exception:

assertThat(tokenFuture).failsWithin(duration) // and/or failsWithin(timeout, unit)           
                       .hasMessage("boom!")
                       .hasNoCause();

my preference goes with returning the future's result for further assertions (so not the first option) to leverage existing assertions, moreover with asInstanceOf it is now possible to call specific type assertions:

assertThat(stringFuture).completesWithin(duration) // and/or completesWithin(timeout, unit)           
                        .asInstanceOf(InstanceOfAssertFactories.STRING)
                        .startsWith("foo");

an alternative would be to pass the InstanceOfAssertFactory as the last parameter with the optional as() syntactic sugar:

assertThat(stringFuture).completesWithin(duration, as(STRING)) 
                        .startsWith("foo");

@jypma @almothafar @oscarfh @stovocor @scordio WDYT?

[scordio]

I like the second option to reuse existing assertions together with InstanceOfAssertFactory 👍

[joel-costigliola]

another thought, replace completesWithin by completesNormallyWithin or succeedsWithin to make it clear the future must return a value

[scordio]

The succeedsWithin/failsWithin pair seems a good option to me

[joel-costigliola]

I'll go with succeedsWithin(long timeout, TimeUnit timeUnit) over succeedsWithin(Duration timeout) as the error message with Duration does not look as good as with long timeout, TimeUnit timeUnit, moreover succeedsWithin(long timeout, TimeUnit timeUnit) is quite readable:

IMO

assertThat(future).succeedsWithin(10, MILLISECONDS);

is slightly nicer to read than

assertThat(future).succeedsWithin(Duration.ofMillis(10));

error messages comparison

  <CompletableFuture[Incomplete]>
to be completed within 10L Millis.

vs

  <CompletableFuture[Incomplete]>
to be completed within PT0.01S.

[jypma]

My thoughts:

• At least keep .succeedsWithin(Duration) as an overload. I expect many cases have a timeout-like value here, that you'd want to hide away / scale upon globally, and reuse. So sth like .succeedsWithin(reasonableTimeout). As for the error message, nothing says we _have_ to use Duration.toString() (which, I agree, sucks).

• For type-safe assertions on the type inside the future, I'd prefer the .completesWithin(duration, as(STRING)), since the type signature can be made to have CompletionStage<T> match the T against the type givin inside the as(). This will allow the compiler to already catch non-sensical typecasts (since you don't go through Object).

[joel-costigliola]

thanks for your touhgts @jypma, I'll add/keep the Duration alternative, I'll see if I can come up with a better representation for Duration.

On the second point, I was planning to implement succeedsWithin(duration, as(TYPE)), note that the other will be there anyway since it's available for all XxxAssert classes.

[hadrienk]

Hi, was the following method forgotten or you decided to drop it?

assertThat(tokenFuture).failsWithin(duration) // and/or failsWithin(timeout, unit)           
                       .hasMessage("boom!")
                       .hasNoCause();

[joel-costigliola]

forgotten mostly, now that we have added succeedsWithin for 3.17.0 https://github.com/joel-costigliola/assertj-core/pull/1930, we should add failsWithin too