JUnit 4 to 5 migration of @Test(timeout=N) drops separate-thread semantics, allowing hanging tests to block the runner indefinitely

[mattdepaula]

Description

JUnit 4's @Test(timeout = N) runs the test body in a daemon thread via FailOnTimeout — it wraps the test in a FutureTask and calls FutureTask.get(timeout, timeUnit). When the timeout elapses the runner thread unblocks immediately and reports failure. A hanging test never blocks the suite.

UpdateTestAnnotation migrates this to:

@Timeout(value = 500, unit = TimeUnit.MILLISECONDS)

@Timeout without an explicit threadMode inherits ThreadMode.INFERRED, which resolves to SAME_THREAD by default. In SAME_THREAD mode the test runs synchronously in the runner thread. A ScheduledExecutorService sends an interrupt after N ms, but if the test body does not respond to interruption (infinite loop, lock contention without a blocking call) delegate.proceed() never returns and the runner thread blocks indefinitely.

End-to-end verification

Using the real JUnit Platform Launcher with two identical infinite-loop test methods — the only difference is threadMode:

-- CURRENT recipe output (@Timeout with SAME_THREAD — the bug) --
  RESULT : HUNG — runner still blocked after 3000ms
  A non-interruptible loop ignores SAME_THREAD's interrupt signal.
  In CI this causes the build to hang indefinitely.

-- CORRECT recipe output (@Timeout with SEPARATE_THREAD — the fix) --
  RESULT : completed in 319ms
  Tests run=1  failed=1
  Failure : hang() timed out after 300 milliseconds

Why SEPARATE_THREAD is safe for migrated tests

JUnit 4's FailOnTimeout was already running these tests in a separate daemon thread. Any test that passed under @Test(timeout=N) was already thread-isolated. Emitting SEPARATE_THREAD restores that exact model and cannot introduce threading issues that did not already exist in JUnit 4.

Expected output

@Timeout(value = 500, unit = TimeUnit.MILLISECONDS, threadMode = Timeout.ThreadMode.SEPARATE_THREAD)

Affected recipe

UpdateTestAnnotation

[mattdepaula]

Suggested fix approach

Rather than emitting @Timeout(value = N, unit = MILLISECONDS, threadMode = SEPARATE_THREAD), a more consistent approach would be to wrap the method body in assertTimeoutPreemptively — the same pattern OpenRewrite already uses for @Test(expected=Exception.class) → assertThrows.

Before (JUnit 4):

@Test(timeout = 500)
public void myTest() throws Exception {
    // test body
}

After (JUnit 5):

@Test
public void myTest() throws Exception {
    assertTimeoutPreemptively(Duration.ofMillis(500), () -> {
        // test body
    });
}

This approach:

• Is consistent with how ExpectedExceptionToAssertThrows handles @Test(expected=...) body wrapping
• Makes the preemptive behaviour explicit at the call site rather than hidden in an annotation attribute
• Handles checked exceptions transparently — JUnit 5's Executable declares throws Throwable
• Avoids the lesser-known threadMode API

[timtebeek]

I have #1076 (comment) ready to go, but it looks like you might have intended to go with mattdepaula#2 instead.

I feel keeping the annotation might be most in line with what folks expect from a migration, even if there's precedent in switching to a lambda assertions method. What are you proposing we do here?

[mattdepaula]

I have #1076 (comment) ready to go, but it looks like you might have intended to go with mattdepaula#2 instead.

I feel keeping the annotation might be most in line with what folks expect from a migration, even if there's precedent in switching to a lambda assertions method. What are you proposing we do here?

Hi @timtebeek, no, I don't. I still going through some company's processes to be able to contribute.

I don't have strong preferences, I believe either annotation and lambda assertions method fits well.

[timtebeek]

Great, then we'll go with the annotation approach as it's more correct to set the thread there and avoid hanging on long running tests.

[timtebeek]

Thanks for the report and prompt response!