junit4->5 rewrites @Test(1000 /*ms*/) to @Timeout(1000 /*sec*/) which breaks test logic

[vlsi]

What version of OpenRewrite are you using?

latest.integration

JUnit4 uses milliseconds for the timeout (there's no way to configure the unit), and JUnit5 uses TimeUnit.SECONDS by default.

I believe this is a wrong test:

rewrite-testing-frameworks/src/test/java/org/openrewrite/java/testing/junit5/UpdateTestAnnotationTest.java

Lines 275 to 302 in 326c048

	void annotationWithTimeout() {
	//language=java
	rewriteRun(
	java(
	"""
	import org.junit.Test;
	public class MyTest {
	@Test(timeout = 500)
	public void test() {
	}
	}
	""",
	"""
	import org.junit.jupiter.api.Test;
	import org.junit.jupiter.api.Timeout;
	public class MyTest {
	@Test
	@Timeout(500)
	public void test() {
	}
	}
	"""
	)
	);

I think OpenRewrite should convert @Test(timeout = 5000) to @Timeout(5); and it should convert @Test(timeout = 500) to @Timeout(value = 500, unit = TimeUnit.MILLISECONDS).

[timtebeek]

That's indeed a bug, and a nasty one at that as well. Thanks for pointing it out!

[vlsi]

It might make sense to add a rewrite rule that would detect "too large timeouts" and suggest converting them to seconds.
For instance: imagine someone has @Timeout(1000). I am 100% sure they meant 1 second rather than 1000 seconds = 16.7 minutes.

As a workaround, users could type @Timeout(value = 17, unit = TimeUnit.MINUTES) or @Timeout(value = 1000, unit = TimeUnit.SECONDS).

So the rewrite rule could be @Timeout(6000) => @Timeout(60).

[timtebeek]

Yes I indeed think a separate recipe is in order that detects and corrects such implausible values; See also #451 (comment)

[timtebeek]

Should be fixed for the time being; would you want to log a separate issue for detecting implausible timeouts and simplifying existing timeouts?

[vlsi]

I've filed #452