Should assertThrows swallow instances of TestAbortedException?

[sormuras]

@lahodaj asked in a different channel

The original TestNG test is this (test/jdk/java/foreign/TestSegmentCopy.java):

    @Test(expectedExceptions = IndexOutOfBoundsException.class, dataProvider = "types")
    public void testBadOverflow(Type type) {
        if (type.layout.byteSize() > 1) {
            MemorySegment segment = MemorySegment.ofArray(new byte[100]);
            MemorySegment.copy(segment, type.layout, 0, segment, type.layout, 0, Long.MAX_VALUE);
        } else {
            throw new SkipException("Byte layouts do not overflow");
        }
    }

the script converts this to:

    @ParameterizedTest
    @MethodSource("types")
    public void testBadOverflow(Type type) {
        Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {
            if (type.layout.byteSize() > 1) {
                MemorySegment segment = MemorySegment.ofArray(new byte[100]);
                MemorySegment.copy(segment, type.layout, 0, segment, type.layout, 0, Long.MAX_VALUE);
            } else {
                Assumptions.assumeTrue(false, "Byte layouts do not overflow");
            }
        });
    }

this seems sensible to me at the first sight (apart from the assumption should be changed differently for style, but lets leave that for now), BUT: the assumption will throw org.opentest4j.TestAbortedException, and assertThrows then fails, as it only expects an IndexOutOfBoundsException.
[...]
Why can't I skip a test (by aborting it mid-flight via the Jupiter Assumptions API) where I expect an exception?

[sormuras]

Perhaps assertThrows and friends should (silently) ignore/swallow org.opentest4j.TestAbortedException instances. 🤔

[mpkorstanje]

What if I'm testing code that intentionally throws TestAbortedException?

[sbrannen]

See #1689 (comment). 😉

[lahodaj]

FWIW, if the meaning of assumeTrue/False would be "this test cannot continue, and that's more or less OK", then I would say assertThrows should pass/rethrow the exception.

As for the example from #1689 (comment):

assertThrows(TestAbortedException.class, () -> assumeTrue(false));

Although this is a valid question, I would assume this to be extraordinarily rare/niche (probably would only appear in a handful of test cases in core test frameworks, like JUnit itself). If I would ignore possible compatibility issues for an already released API, I would say assertThrows should not accept the TestAbortedException.class argument, and should unconditionally fail. The handful of tests testing a specific behavior would have to implement the check using different means.

[sbrannen]

Duplicates:

• Nested assumption failures should not cause assertion failures for enclosing assertThrows() #1689
• AssertThrows should probably propogate instances of AssertionError or AssumptionViolatedException unless they are also instances of user expected Type #2050

[mpkorstanje]

On the face of it, this test just seems badly written. And with garbage in, you get garbage out.

The assert throws is overly broad, perhaps because expectedExceptions = IndexOutOfBoundsException.class doesn't allow specifying exactly which method should throw. So unwanted, but not unreasonable.

But then it is also throwing an exception for cases where the test doesn't apply. I don't understand why. I guess it is a workaround for the fact that an exception is always expected

But then types for which type.layout.byteSize() > 1 doesn't hold shouldn't be in the source. The source method should filter these out already when generating the types.

So I think the automatic conversion highlights a bad pattern. And that pattern should be fixed, not accommodated.

Why can't I skip a test (by aborting it mid-flight via the Jupiter Assumptions API) where I expect an exception?

To answer the question: because with JUnit you can specify more precisely where an exception should occur. You can abort before you expect that exception.

[sbrannen]

To answer the question: because with JUnit you can specify more precisely where an exception should occur. You can abort before you expect that exception.

I agree with @mpkorstanje.

That test should be rewritten, for example as follows (not tested in IDE).

    @ParameterizedTest
    @MethodSource("types")
    public void testBadOverflow(Type type) {
        Assumptions.assumeTrue(type.layout.byteSize() > 1, "Byte layouts do not overflow");

        Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {
            MemorySegment segment = MemorySegment.ofArray(new byte[100]);
            MemorySegment.copy(segment, type.layout, 0, segment, type.layout, 0, Long.MAX_VALUE);
        });
    }

[lahodaj]

Yes, this test case can be rewritten (easily manually, much harder for me automatically). And I do understand the "pure" approach. Although I don't find it too difficult to image a test that throws SkipException sensibly from a place where code reshuffling is not possible. Luckily, it seems this is not a significant problem so far for the (pragmatic) TestNG->JUnit conversion.

But, I think there's still a point (and possibly a question?) to make:

• what throw new SkipException(...) means? I would say it signals "this test cannot continue to run, and that's (more or less) OK"
• what is the high-level meaning of Assumptions.assumeTrue/False(...)? Is it:

1. "this test cannot continue to run, and that's (more or less) OK"
2. throw a specific exception if the assumption does not hold?
3. something else?

If it is "1.", then I would say the fact that assumeTrue throws an exception is an implementation detail, and assertThrows should handle it gracefully, to support composability.

If it is "2.", then assumeTrue is (from a "pure" standpoint) not the correct idiom to use in place of SkipException, as it means something else. Is there a better idiom in JUnit 5 that would respect the "this test cannot continue to run, and that's (more or less) OK" meaning? Or is JUnit 5 missing this feature?