Confusing behavior of AbstractCompletableFutureAssert.failsWithin

[vladykin]

Describe the bug

Sometimes I need to verify that the future completed with exception, but don't care with which exactly. Used assertThat(future).failsWithin(...) for that. But, to my greatest surprise, found that assertThat(future).failsWithin(...) passes when the future doesn't complete at all, neither successfully, nor exceptionally.

• assertj core version: 3.26.3
• java version: 17
• test framework version: junit 5

Test case reproducing the bug

    // a future that doesn't complete
    CompletableFuture<Void> future = new CompletableFuture<>();
    // the next assertion passes as if future has failed --- this is counterintuitive
    assertThat(future).failsWithin(Duration.ofMillis(100));
    // the next assertion fails, because the future hasn't actually failed --- this makes sense
    assertThat(future).hasFailed();

[joel-costigliola]

Thanks for reporting this @vladykin !

[joel-costigliola]

The behavior is correct according to the javadoc since the future has not completed within the given duration.

Having said that, the wording can be confusing as people might interpret failing differently, instead what we likely should have done is reuse the java wording': complete exceptionally, done and cancelled.

Note that hasFailed is deprecated because it is ambiguous, see https://www.javadoc.io/doc/org.assertj/assertj-core/latest/org/assertj/core/api/AbstractCompletableFutureAssert.html#hasFailed()

[vladykin]

Right, I've seen the javadoc showing exactly this example of the failsWithin behavior. However, I'm struggling to understand the logic behind it. The future hasn't completed with any of the possible results, it's still pending.

Let me show that using methods of the future directly, without the controversial hasFailed:

    // a future that doesn't complete
    CompletableFuture<Void> future = new CompletableFuture<>();
    // the next assertion passes as if future has failed
    assertThat(future).failsWithin(Duration.ofMillis(100));
    // however all the following checks indicate that future is not done yet, it can still have a result later
    assertThat(future.isDone()).isFalse();
    assertThat(future.isCancelled()).isFalse();
    assertThat(future.isCompletedExceptionally()).isFalse();
    assertThat(future.getNow(null)).isNull();

[joel-costigliola]

It's just that we considered (maybe wrongly) that if you expect a future to complete with a given duration, and it has not done so then something did not behave as expected and thus can be deemed as failed.

As there is no definition of failed in java, we came with our own interpretation.

[vladykin]

Well, waiting for the future to complete indeed failed - in the sense that future.get() threw an exception. But not the future itself. It's still pending and can succeed or fail later. That ambiguity - of what failed - is the source of my confusion.

Will it be possible to address this in future AssertJ version somehow? Maybe introduce more methods to disambiguate those situations?

[joel-costigliola]

I do agree this is confusing, what would you like to have as a new assertion ? here are the existing ones https://www.javadoc.io/static/org.assertj/assertj-core/3.26.3/org/assertj/core/api/AbstractCompletableFutureAssert.html#method-summary

[vladykin]

If I had the luxury not to care about backward compatibility, I'd change the semantics of the failsWithin() methods. If that's not an option, then probably add completesExceptionallyWithin(timeout). The behavior I'm looking for is that the assertion should pass if the future actually completes exceptionally, and fail otherwise. TimeoutException and InterruptedException thrown by future.get() are considered failure of the assertion, as the future itself is not actually failed/completed.

The case of TimeoutException might deserve its own assertion method. Although I don't need it often, I can see cases when I'd want to use it. It could be named notCompletesWithin(timeout). It should pass if future.get() throws TimeoutException and fail on any other outcome.

[joel-costigliola]

We won't change the semantics of failsWithin as other users rely on it.

completesExceptionallyWithin(timeout) and isNotCompleted(timeout) would be good additions. Are you would be keen to contribute to any of these assertions (even both !) ? No problem if you can't

[vladykin]

Why not :) Here is the first draft waiting for feedback: https://github.com/assertj/assertj/pull/3597

Focusing on completesExceptionallyWithin(timeout) for now. Then can consider doing the isNotCompleted(timeout)