On Mon, 22 May 2023 10:11:25 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> 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). I think it's fine to leave it in the current form then. If/when any of these tests fail due to unexpected exception type in these asserts, we can then move them all to the other proposed (or some other) construct. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200321915
