RFR: 8328629: JUnit test without a timeout value can hang indefinitely

[Kevin Rushforth]

On Thu, 19 Sep 2024 23:09:37 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> This PR adds a default timeout of 120 seconds for JUnit 5 tests that do not have an explicit `@Timeout` on either the methods or the class, and a default timeout of 20 seconds for lifecycle methods (e.g., `@BeforeEach`, `@BeforeAll`, `@AfterEach`, `@AfterAll`).
>> 
>> JUnit 5 effects its timeout by interrupting the test thread. We have several tests that catch and ignore InterruptedException for sleep or await, so I fixed these tests to throw an error if InterruptedException is caught (they may or may not be a problem, but if any of them can happen in a loop, it would block the timeout from killing the test).
>> 
>> While testing this, I discovered that by itself, this JUnit 5 timeout is ineffective if the deadlock described in [JDK-8238505](https://bugs.openjdk.org/browse/JDK-8238505)  occurs. The reason for this is that the deadlock almost always occurs when called from a shutdown hook (called by `System.exit`), and that prevents the process from exiting even when interrupted by the test runner after the timeout. A separate PR is out for that bug fix.
>> 
>> ### Testing instructions
>> 
>> I tested this in connection with PR #1574 / [JDK-8340405](https://bugs.openjdk.org/browse/JDK-8340405) using my [test-timeout-and-shutdown-hang](https://github.com/kevinrushforth/jfx/tree/test-timeout-and-shutdown-hang) branch. That branch has the fixes from both this PR and PR #1574 as well as tests that will hang indefinitely without the fixes and fail due to the expected timeout with the fixes.
>> 
>> The default timeout values for test and lifecycle methods can be modified using a gradle property:
>> 
>> * `JUNIT_TEST_TIMEOUT` : default = 120s
>> * `JUNIT_LIFECYCLE_TIMEOUT` : default = 20s
>> 
>> For example, to set a timeout of 60 seconds for tests and 15 seconds for lifecycle methods:
>> 
>> 
>> gradle -PJUNIT_TEST_TIMEOUT=60s -PJUNIT_LIFECYCLE_TIMEOUT=15s test
>
> tests/system/src/test/java/test/robot/javafx/scene/control/behavior/BehaviorRobotTestBase.java line 329:
> 
>> 327:     protected void sleep(int ms) {
>> 328:         try {
>> 329:             // KCR: this should be sleep(ms) not a hard-coded 1
> 
> good thing it's never called, but it is my fault.
> this method was later replaced with `waitForIdle`
> 
> I can either delete the method or fix it as part of #1569
> what do you think?

If it isn't being used, then I like your idea of deleting it. Especially since this seems redundant given `Util.sleep(ms)`. I'll revert my changes and let you delete it as part of #1569.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1575#discussion_r1768487648