Trim internal frames from AssertionFailedError

[mpkorstanje]

When using JUnit.start and creating a failing test, users will be confronted with a large stacktrace with mostly irrelevant information. Even after #5158 is merged, the stacktrace will contain several internal frames:

org.opentest4j.AssertionFailedError: expected: <11> but was: <12>
	at [email protected]/org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:158)
	at [email protected]/org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:139)
	at [email protected]/org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:201)
	at [email protected]/org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:152)
	at [email protected]/org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:147)
	at [email protected]/org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:558)
	at com.examp.project/com.example.project.HelloTest.stringLength(HelloTest.java:14)

By pruning these internal frames, the stacktrace can be reduced to a much more readable:

org.opentest4j.AssertionFailedError: expected: <11> but was: <12>
	at [email protected]/org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:558)
	at com.examp.project/com.example.project.HelloTest.stringLength(HelloTest.java:14)

The pruning functionality is intentionally added to the AssertionFailureBuilder rather than the ExceptionUtils to enable other users to also prune the internal frames from their own assertions.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[marcphilipp]

I think we should preserve the bottommost Assertions/Assumptions stack frame:

org.opentest4j.AssertionFailedError: expected: <11> but was: <12>
	at [email protected]/org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:558)
	at com.examp.project/com.example.project.HelloTest.stringLength(HelloTest.java:14)

[mpkorstanje]

I'm not convinced that this extra frame is useful. Users will never visit it, because the frame above it is their code and the place they'll ultimately have to fix the issue.

public class HelloTest {

    @Test
    void stringLengthJupiter() {
        org.junit.jupiter.api.Assertions.assertEquals(11, "Hello JUnit2".length());
    }
    @Test
    void stringLengthAssertJ() {
        org.assertj.core.api.Assertions.assertThat("Hello JUnit2".length()).isEqualTo(11);
    }
}

And looking at other projects: IDEA already filters it out. And AssertJ doesn't include it in the first place. edit: As does Google Truth.

On a more technical note, for the exceptions thrown by the infline fun in Assertions.kt the stack frames must either start at AssertionFailureBuilder.build or the test method. In which case the latter seems preferable.

What benefits do you think this extra frame adds?

[marcphilipp]

What benefits do you think this extra frame adds?

To me, the extra frame makes it explicit that a specific assertion method failed. Let's discuss with the rest of the team, though.

[sormuras]

Would it make sense to overload this with a "pass 1" variant?

public AssertionFailureBuilder trimStacktrace(@Nullable Class<?> target) {
    return trimStacktrace(target, 1);
}

[mpkorstanje]

I've thought about that. But the "natural" default value seems to be 0. We are in most places keeping one extra. So though duplicated quite often (but not always), I think we should keep that.

On reflection I do have my doubts about putting both parameters in a single method though. For a builder, this really should be two separate methods.

[mpkorstanje]

On reflection I do have my doubts about putting both parameters in a single method though. For a builder, this really should be two separate methods.

It does force the design towards using a strategy pattern but I don't think we have enough design constraints for that yet. So I've just extracted a new method for the second argument and set the default value to 1.

[sormuras]

Looks good to me, added a tiny question regarding an overload for the common case of retain 1 frame.