On Mon, 22 May 2023 10:05:49 GMT, Jaikiran Pai <j...@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? I don't mind moving this but it would require changing them everywhere (as you point out). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200299321
