@RetryingTest wraps AssertionFailedError, which prevents IntelliJ's "<Click to see difference>" from functioning

[liberaj]

When using the @RetryingTest functionality of Junit Pioneer, an AssertionFailedError will be thrown on the final invokation of the retrying test, with the previously failing tests throwing a TestAbortedException.

This approach works well, except for when the cause of test failure is an AssertionFailedError. In this scenario, the original AssertionFailedError will be wrapped inside of Pioneer's AssertionFailedError, as the cause. On paper this sounds like a sensible approach to ensuring that the original assertion details are not lost, but whilst still providing additional details of the status of the RetryingTest to the user.

Practically, for users of IntelliJ, this nesting of AssertionFailedError will interfere with the ability to view assertion differences.

Output from Regular @Test

    @Test
    void testAssertionFailsNormal() {
        assertEquals("expected", "actual");
    }

org.opentest4j.AssertionFailedError: 
Expected :expected
Actual   :actual
<Click to see difference>

<6 internal lines>
	at AlwaysFailingTest.testAssertionFailsNormal(AlwaysFailingTest.java:14) <31 internal lines>
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) <9 internal lines>
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) <28 internal lines>

Output from @RetryingTest

    @RetryingTest(maxAttempts = 3)
    void testAssertionFailsRetryable() {
        assertEquals("expected", "actual");
    }

org.opentest4j.AssertionFailedError: Test execution #3 (of up to 3 with at least 1 successes) failed ~> test fails - see cause for details

	at org.junitpioneer.jupiter.RetryingTestExtension$FailedTestRetrier.failed(RetryingTestExtension.java:158)
	at org.junitpioneer.jupiter.RetryingTestExtension.handleTestExecutionException(RetryingTestExtension.java:62) <26 internal lines>
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133) <20 internal lines>
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) <9 internal lines>
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) <28 internal lines>
Caused by: org.opentest4j.AssertionFailedError: expected: <expected> but was: <actual> <6 internal lines>
	at AlwaysFailingTest.testAssertionFailsRetryable(AlwaysFailingTest.java:9) <19 internal lines>
	... 81 more

Summary of Differences

Between the two outputs, we can notice how on the @RetryingTest example, IntelliJ hasn't recognised the output as being an assertion difference, so IntelliJ hasn't provided the tooling to view the differences using the built-in tooling.

Proposal

I believe that instead of throwing an AssertionFailedError from the RetryingTestExtension, we should first inspect the cause and if the cause is also of type AssertionFailedError, then we should instead throw a MultipleFailuresError that can include both the failure from @RetryingTest and the original AssertionFailedError. IntelliJ should correctly recognise the assertion difference then from the original error.

Extra Details

• IntelliJ IDEA 2023.1.1
• org.junit-pioneer:junit-pioneer:2.0.1

Contributions

I would be happy to attempt a pull request for this improvement providing that you agree with my assessment. Please let me know!

[Michael1993]

We talked about this and it makes perfect sense to throw a MultipleFailuresError instead of the AssertionFailedError.
I am not sure if we need to check the exception type for this change? Does it have any advantages?

Additionally, right now the exception throws only the last (unexpected) exception encountered - if we throw a MultipleFailuresError it would make sense to collect and throw all exceptions encountered with it.

If you still want to give this a try go ahead! If not, let us know and we will ping you on the related PR @liberaj 😃

[nipafx]

Great investigation and writeup @liberaj, thank you! I agree with @Michael1993: If we touch this anyway, we should collect all exceptions and stuff them into a MultipleFailuresError (which I never heard of, btw.).

[liberaj]

Thanks both for your feedback. I agree that generally throwing a MultipleFailuresError in all circumstances can make sense.

I will try and find some time in the next week or two to submit a PR to you. I haven't contributed to open-source before, so fancy this as a good opportunity to try it out.

[Michael1993]

Hi @liberaj !

Any updates on this? If you don't have the time we can take over this issue but we can also have it on indefinite hold if you want to work on it eventually. 🙂