On Tue, 24 Sep 2024 15:31:02 GMT, Kevin Rushforth <k...@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 > > Kevin Rushforth has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - throw new AssertionError(ex) --> fail(ex) > - Merge remote-tracking branch 'upstream/master' into 8328629-junit-timeout > - Remove unused sleep method from BehaviorRobotTestBase. > - Fix typo (extra spaces) in system property name > - 8328629: JUnit test without a timeout value can hang indefinitely one minor formatting issue, otherwise looks good. ------------- Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1575#pullrequestreview-2325714559
