RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out

[Jaikiran Pai]

On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman <alanb at 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