On Thu, 19 Sep 2024 14:15:23 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 Reviewer: @arapte or @andy-goryachev-oracle modules/javafx.controls/src/test/java/test/javafx/scene/control/AlertTest.java line 96: > 94: } catch (InterruptedException e) { > 95: // fail(e); > 96: throw new AssertionError(e); I left the commented out "fail(e)" as a possibly better alternative once these tests are updated to JUnit 5, which is under review at PR #1569. Depending on the timing of integrating that PR and this one, I will either merge in master (I've already done a test merge and there are no merge conflicts) and update these calls to use "fail" or file a follow-on bug. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1575#issuecomment-2361155717 PR Review Comment: https://git.openjdk.org/jfx/pull/1575#discussion_r1766935521
