On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman <al...@openjdk.org> wrote:
> This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java line 269: > 267: assertTrue(executor.awaitTermination(10, TimeUnit.MILLISECONDS)); > 268: Throwable e = assertThrows(ExecutionException.class, > future::get); > 269: assertTrue(e.getCause() instanceof InterruptedException); Would using `assertEquals(InterruptedException.class, e.getCause().getClass())` be better? That way if/when the test fails, it even prints the unexpected exception type? But I then see that this test already uses `assertTrue` for similar cases in some other place, so maybe it's fine in the current form? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200291899
