removeAssertJRelatedElementsFromStackTrace does not remove all the JDK elements triggered by AssertJ

[almondtools]

If I call assertThat(0).isEqualTo(1);, I get an exception with a stacktrace

org.opentest4j.AssertionFailedError: 
expected: 1
 but was: 0
	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at net.amygdalum.WorldTest.testStacktrace(WorldTest.java:13)
	at ...

The first two lines are superfluous and not understandable (WorldTest:13 does not call newInstanceWithCaller). I think that some bug fix removing assertj internals from the stack now does not remove these lines any more.

This bug seems so obvious to me, that I am uncertain whether my configuration is the reason, so I added my IDE and my build system to the context below. Feel free to ask more, if you have a suspect.

• assertj core version: 3.25.3
• java version: 21
• test framework version: Junit Jupiter
• os (if relevant): Windows
• build: grade
• IDE: Eclipse

Test case reproducing the bug

class WorldTest {

	@Test
	void testStacktrace() {
		assertThat(0).isEqualTo(1);
	}
}

[scordio]

Good catch, @almondtools!

I would also consider the bug quite obvious but yours seems to be the first feedback on a code that never changed since the forking from Fest Assert (66f11f8) 🙂

According to the Javadoc example of removeAssertJRelatedElementsFromStackTrace, pruning all the stack trace elements triggered by AssertJ was never intended. Only the one right before the first AssertJ method is supposed to be removed.

If you would run the following:

@Test
void testStacktrace() {
	Assertions.setRemoveAssertJRelatedElementsFromStackTrace(false);
	assertThat(0).isEqualTo(1);
}

you would see how the stack trace really looks like:

...
kept -->	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
kept -->	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
removed -->	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
removed -->	at org.assertj.core.error.ConstructorInvoker.newInstance(ConstructorInvoker.java:28)
removed -->	at org.assertj.core.error.ShouldBeEqual.assertionFailedError(ShouldBeEqual.java:223)
...

I believe any element that is a consequence of org.assertj.core.error.ConstructorInvoker.newInstance is not valuable for the users and should be pruned.

Referring to the original example, what users should see is only:

org.opentest4j.AssertionFailedError: 
expected: 1
 but was: 0
	at net.amygdalum.WorldTest.testStacktrace(WorldTest.java:13)
	at ...

@joel-costigliola how do you see it?

[joel-costigliola]

I completely agree to remove the stack before the user's code as the aim is to clear the clutter from the stack.

Let's do it for 3.26.0

[almondtools]

Also check to keep the handling of this code consistent

	@Test
	void testStacktrace2() {
		assertThat(0)
			.satisfies(x -> assertThat(x).isEqualTo(1));
	}

Here the superfluous lines from the example above appear in the middle of the stack trace. I have no preference how to handle this, but just wanted to point out that this test is similar but different.