Add TestMethodReturnValueHandler SPI for non-void @Test methods

[FroMage]

Quarkus supports reactive programming, so methods typically can return void and be blocking, or return a reactive type and we handle the plumbing to get it executed in the proper reactive context. But JUnit does not accept non-void test methods.

Supporting this without JUnit modifications requires us to run bytecode modification to move the non-void method under a different suffixed name, and generate a void method that is just for show so JUnit accepts it and then later our existing JUnit extension will replace any method it finds with that special suffix with the real one that returns a Uni and then handle the plumbing. That is pretty convoluted and requires a custom ClassLoader and bytecode manipulation.

Apparently, JUnit already has some support for non-void test methods for Kotlin, but it did not turn into an extension SPI.

This PR introduces a new extension point that allows frameworks to support @Test methods with non-void return types. This enables reactive frameworks (e.g., Quarkus with Mutiny Uni) to let test methods return their native types instead of requiring bytecode transformation to work around the void limitation.

The SPI is discovered via ServiceLoader. During discovery, IsTestableMethod accepts methods with a registered handler. During execution, the handler receives an Invocation it must proceed() on, giving it control over both the execution context and return value processing.

I am not entirely happy with the new SPI, given that all the other extensions are registered under the regular Extension SPI, but apparently that happens later after tests are discovered, making it too late, and hence Claude advised to introduce a new SPI. We could still use the same SPI and just ignore the TestMethodReturnValueHandler extensions in the regular extension loading (since it's too late to use them) if you prefer.

What do you think of this PR? I know non-void test methods have generated a lot of debate and issues in the past, but this looks clean enough as an extension point, no?

---

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

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[mpkorstanje]

At a glance your issue looks similar to #5292 which is currently waiting for interest.

To remove any ambiguity, do you have a simple but non-trivial example of what a reactive test would look like?

[FroMage]

So, this is a blocking test with Quarkus Data (we're testing inserting a new DB record):

      @Test
      @TestTransaction
      void testPersist() {
          assertThat(Person.count()).isZero();

          Person person = new Person();
          person.name = "Alice";
          person.persist();

          assertThat(Person.count()).isEqualTo(1);
          List<Person> all = Person.listAll();
          assertThat(all).hasSize(1);
          assertThat(all.get(0).name).isEqualTo("Alice");
      }

The same but with reactive programming (if this PR or similar support gets added):

      @Test
      @TestTransaction
      Uni<Void> testPersist() {
          Person person = new Person();
          person.name = "Alice";

          return Person.count()
                  .invoke(count -> assertThat(count).isZero())
                  .chain(() -> person.persist())
                  .chain(() -> Person.count())
                  .invoke(count -> assertThat(count).isEqualTo(1L))
                  .chain(() -> Person.<Person>listAll())
                  .invoke(all -> {
                      assertThat(all).hasSize(1);
                      assertThat(all.get(0).name).isEqualTo("Alice");
                  })
                  .replaceWithVoid();
      }

The "beauty" here is that this reflects exactly how these two methods would be written if they were regular real business methods and not just test methods (well, minus the assertions, but the style is exactly legit).

What we currently have to do, due to the void limitation is this:

      @Test
      @TestTransaction
      void testPersist(UniAsserter asserter) {
          asserter.execute(() -> Person.count()
                  .invoke(count -> Assertions.assertEquals(0L, count)));

          asserter.execute(() -> {
              Person person = new Person();
              person.name = "Alice";
              return person.persist();
          });

          asserter.execute(() -> Person.count()
                  .invoke(count -> Assertions.assertEquals(1L, count)));

          asserter.execute(() -> Person.<Person>listAll()
                  .invoke(all -> {
                      Assertions.assertEquals(1, all.size());
                      Assertions.assertEquals("Alice", all.get(0).name);
                  }));
      }
  }

That is, instead of composing the future (Uni in the case of Quarkus, but others would be CompletionStage or similar), we have to delegate execution to a synthetic parameter. This is entirely different from how people would write regular reactive methods, so we have to teach people two ways of doing reactive: the real way for production code, and the workaround-way for tests.

And even then, we're simplifying a lot because we already have other hacks to let us invoke the test method in the proper event loop thread rather than the JUnit thread.

This PR would be a lot cleaner for us, but would also work for other reactive types.

[rmannibucau]

maybe use Type instead of Class<?>?

[FroMage]

It would be more versatile, for sure. I can't think of any reason why anybody would need more than the raw type to determine whether it's reactive or not, but other use-cases might benefit from this. It would probably be wise to also make that Type available somehow from execute() (unless it's already present via the parameters somehow).

[rmannibucau]

shouldn't it be reversed so be reactive by default to enable really concurrent method execution instead of reactive/async impl being made blocking?

side note: virtual threads can help but would also hide issues when not used in the enclosing environment so are not an answer to that need alone

[FroMage]

I'm not sure I understand. What should be reversed?

[rmannibucau]

should likely be infinite or respect @timeout on the method but not 30s by default IMHO

as mentionned before it would be better to ensure the execution is reactive (not blocking there)

[rmannibucau]

instead of making not void method void friendly, make void methods reactive friendly (CompletionStage can be a neutral contract), this way you can just run all reactive tests concurrently - respect lock semantic indeed.

the PR proposal is to make reactive method blocking so just enables syntaxic sugar but not a paradigm change/enablement.

[FroMage]

This is a test, not an implementation, but otherwise I agree. Perhaps the API should make sure we can access any @Timeout on the method.

As for the execution having to block, I'm afraid that's pretty much forced, at least in the case of an event-loop reactive implementation such as Vert.x and Quarkus, because the tests are run on platform threads, and the test body has to be delegated to the event loop, so we cannot avoid delegation and blocking, but we only block the test thread, the test method is properly non-blocking.

I definitely don't mean to add support for async/reactive in JUnit, that would be a much bigger endeavour. Not one I would recommend, and definitely not just for this feature.

[rmannibucau]

As for the execution having to block, I'm afraid that's pretty much forced, at least in the case of an event-loop reactive implementation such as Vert.x and Quarkus, because the tests are run on platform threads, and the test body has to be delegated to the event loop, so we cannot avoid delegation and blocking, but we only block the test thread, the test method is properly non-blocking.

no, the idea is that junit retrieve a CompletionStage<?> as result and calls handle on this stage so literally

try {
  execute();
  onSuccess();
} catch (...) {
  onError();
}

is replaced by

execute().handle((ok, ko) -> { if (ko != null) onError(ko); else onSuccess(); });

this way the test execution becomes reactive, indeed the "void" (current) execute will be blocking as of today:

final var result = new CompletableFuture<Void>();
try {
  execute();
  result.complete(null);
} catch (final Throwable ex) {
  result.completeExceptionally(ex);
}
return result;

TIP: it can use a dedicated thread pool ;)

so literally you can bridge any reactive or not framework and have a fully reactive execution (just take care of thread pools to not lock but this can be a detail of the SPI impl)

[FroMage]

In most cases I would not want to run reactive tests any more concurrently than blocking tests. Most of my tests have cleanup/teardown that would break if run concurrently. That's not the point of reactive tests.

IMO the only point is to test APIs that are reactive in a way that is natural for people writing reactive tests, rather than rely on syntax that is workaround-specific.

But true, given a properly parallelisable test suite, it might require something different to be able to collect all reactive tests and run them in parallel. It would be different than how blocking tests run in parallel, because we could just compose all the tests and block on a single thread, rather than start a thread pool.

I was hoping this kind of support could come later, as I don't need it and would not know how to achieve it, and this initial support could be a good first step, as it would open the door to tests that return a reactive type, and later if someone contributes support for more efficient parallelising of reactive tests, the syntax from the PoV of the users would not have to change: you can already return reactive types. But the SPI might have to change then, in ways I'm unsure about without experimentation/research.

In general I prefer to have something valuable now and small than something perfect later, but that's up to you, I can understand both points of view 😅

[FroMage]

execute().handle((ok, ko) -> { if (ko != null) onError(ko); else onSuccess(); });

Most frameworks not written with async in mind won't support this easily. I am not familiar with the insides of JUnit, so probably you know better, but I would assume this would be hard to do.

[rmannibucau]

In most cases I would not want to run reactive tests any more concurrently than blocking tests. Most of my tests have cleanup/teardown that would break if run concurrently. That's not the point of reactive tests.

🤔 it is in the original ticket - and is exatly the reference to https://docs.nunit.org/articles/nunit/writing-tests/attributes/test.html or https://learn.microsoft.com/en-us/archive/msdn-magazine/2014/november/async-programming-unit-testing-asynchronous-code for ex. Else you just save a .get() from your test body which is already doable through 3-4 other ways (like having an await helper like awaitability or a parameter swallowing the promise to await it synchronously or through a junit interceptor).

Indeed your current tests will be broken but it will also require some rewriting. Side note: it is already the case since junit supports concurrent test execution on half of the related aspects so nothing new there.

Most frameworks not written with async in mind won't support this easily.

while it is wrapped it will not be worse than today and threading model is controlled by junit so think it will be ~the same no?

[FroMage]

🤔 it is in the original ticket - and is exatly the reference to https://docs.nunit.org/articles/nunit/writing-tests/attributes/test.html or https://learn.microsoft.com/en-us/archive/msdn-magazine/2014/november/async-programming-unit-testing-asynchronous-code for ex.

These are for programming languages that are not Java 🤔

Else you just save a .get() from your test body which is already doable through 3-4 other ways (like having an await helper like awaitability or a parameter swallowing the promise to await it synchronously or through a junit interceptor).

I did mention we currently achieve this via a parameter and a style that is not reactive-friendly. This is not just throwing a .get() as I mentioned we also need to offload the test method onto the event-loop. I don't think we tried using a JUnit interceptor, but given that void methods are currently banned, I don't think it would work.

And BTW, I don't think just because a test is reactive it should be run in parallel, I think that the same rule should apply as for blocking methods wrt. running them in parallel or not.

while it is wrapped it will not be worse than today and threading model is controlled by junit so think it will be ~the same no?

I would defer to the JUnit experts to answer that. Given that they support parallel tests, it should be doable. Not necessarily easy. It was definitely beyond the scope of my PR, though.

[rmannibucau]

.net is quite aligned on "modern" (java >= 8) java on that aspect, java ecosystem just never payed the bill (likely cause it is split compared to MS ecosystem - blind guess).

And BTW, I don't think just because a test is reactive it should be run in parallel, I think that the same rule should apply as for blocking methods wrt. running them in parallel or not.

this is where this programming model shines, you do decide since the caller just let you hande the context (thread) you do use, so all good and it embraces your statement 100% IMHO

[rmannibucau]

proposing this base for future work on that topic: #6012