Checkpoint overuse from async code not flagged

[ibauersachs]

Version

4.5.11
(5.0.0.CR3 has no code changes that would affect a behavior change)

Context

Calling checkpoint.flag() from async code, e.g. from a vertx.setTimer callback or in a future doesn't trigger the documented overuse exception. Now, this is clearly a bug in the test itself, and the test should wait for the completion e.g. by using a CountDownLatch, closeHook, etc. But noticing this bug is actually rather difficult when tests pass.

Do you have a reproducer?

The following test passes, but should fail:

@ExtendWith(VertxExtension.class)
class TooMuchFlagging {
  @Test
  void flagTooMuch(Vertx vertx, VertxTestContext testContext) {
    Checkpoint checkpoint = testContext.checkpoint(3);
    for (int i = 0; i < 10; i++) {
      vertx.setTimer(1, x -> checkpoint.flag());
    }
  }
}

Extra

I played around with the source to figure out how this could be handled. Naturally, this is difficult and flag() calls from Java threads or CompletableFuture based code is probably impossible to catch. One way that could improve the handling though could be closing vert.x in VertxExtension.joinActiveTestContexts and waiting for it to complete.
At least for the timer example from above, adding the following three lines after waiting for the context completion would make the test fail as expected.

vertx-junit5/src/main/java/io/vertx/junit5/VertxExtension.java

Line 199 in 512ca92

if (context.awaitCompletion(timeoutDuration, timeoutUnit)) {

var so = store(extensionContext).get(VERTX_INSTANCE_KEY, ScopedObject.class);
if (so != null && so.get() instanceof Vertx vertx) {
  vertx.close().toCompletionStage().toCompletableFuture().get();
}

I don't like the idea of closing vert.x from the extension (since this is and should be handled by the VertxParameterProvider) but I don't have a better idea yet.

Somewhat related to #65, #90, #106, #113, #118

[tsegismont]

It seems that closing Vert.x from the extension meets your expectations in this particular case only because it allows all the scheduled tasks to fire. I haven't tried but I guess if the tasks were scheduled to some unrelated executor overuse of flag would not be caught.

Your original point is valid, imo, the test is not written correctly. It should either use another signal for completion or, if there is no other option, wait for some time before completing execution.

[ibauersachs]

I wasn't able to verify other executors yet. The general idea was to catch the invalid tests, since sometimes it's hard to spot the mistakes - especially if the test passes (when it really shouldn't) while writing it.

Do you have any idea how the extension could catch that better that I could try pursuing for a potential PR?

[tsegismont]

There is nothing the extension could do, as far as I can imagine. Writing async code is difficult, even in tests. Such a problem could be caught in a code review.