Repository navigation
SoftAssertions nested in satisfies() or allSatisfy() only output first failing assertion #1353
Description
Summary
When using SoftAssertions together with satisfies() or allSatisfy() the additional assertions made in the consumer supplied to these methods are handled differently. In fact, the first failing assertion prevents other in the same consumer to be reported, as expected.
Example
public class MyTest {
@Test
public void softAssertionTest() {
String str = "hello";
SoftAssertions.assertSoftly(sa -> {
sa.assertThat(str).satisfies(s -> {
// fails
sa.assertThat(s).hasSize(2);
// not reported, unless all previous assertions succeed
sa.assertThat(s).isEmpty();
});
});
}
}
Activity
joel-costigliola commentedon Nov 3, 2018
This should work, thanks for reporting this.
maverick1601 commentedon Nov 3, 2018
As far as I understood the code, it is a problem regarding the SoftProxies and nesting with respect to the ErrorCollector. The nested satisfy block seems to let errors propagate, which results in the first assertion (failing) to prevent follow up assertions to even execute.
joel-costigliola commentedon Nov 3, 2018
It seems the problems comes from the way AssertJ handles nested proxy calls.
Example:
softly.assertThat(true).isFalse()calls theisEqualTo(false)which is also proxied, in that case the assertion error is bubbled up. It looks like in this issue the assertion error should simply be recorded and not bubbled up .We count the nested level by count of
ErrorCollector.interceptwhich happens withsatisfiesand the assertions inside (hasSize).joel-costigliola commentedon Nov 3, 2018
You beat me by 2 min! :D
joel-costigliola commentedon Nov 3, 2018
One workaround is simply not to use
sato callsatisfies:maverick1601 commentedon Nov 3, 2018
I know that this is a workaround, but the example given was greatly simplified. This would also affect not using allSatisfy(), I assume, which i do use very often.
joel-costigliola commentedon Nov 6, 2018
I don't see an easy fix for this issue, I'll try to explain why.
Some of the assertions internally use others like
isNotNull()as in:Soft assertion are implemented by proxying assertions and catching any
AssertionErrrorand report all of them later on.In the example above,
containsandisNotNull()are proxied, ifisNotNull()fails the thrownAssertionErrrorwill be collected and the next line of code executed which will result in a NPE, not what we want!So what AssertJ does is to propagate the error to
containsand let the proxy mechanism to collect it (there is one collector per soft assertion instance). That's better.Oh and to do that, it counts the number of calls to
ErrorCollector.interceptfrom the stack trace, if > 1 it will see an internal call and propagate the error (inErrorCollector).Now in your example, you are nesting soft assertions so the the number of calls to
ErrorCollector.interceptwill be > 1 which leads AssertJ to think that it is in a situation likecontainscallingisNotNull(), it propagates theAssertionErrorand this is why the next assertion is not performed.To fix that, we would need to know that the nesting of proxied assertions was intended by the user and not an internal implementation, I'm not sure this is quite possible but I need to give it more time, there might be a solution that I don't see at the moment.
maverick1601 commentedon Nov 6, 2018
Isn't the "nesting" limited to calling satisfies() or allSatisfy(), so we could handle those two differently? Let alone the fact, that we could have deeper nesting as well, for instances satisfy() within satisfy().
It's not that we have a major issue with the current behavior. As long as all assertions succeed, we are sure the test cases are covered. Mainly, the issue comes up during refactorings, if many assertions should fail at once, which is why we use SoftAssertions in the first place.
joel-costigliola commentedon Nov 7, 2018
Maybe but it should be handled only if coming from a soft assertion ... need trying/investigating.
I was also thinking that it might be possible to differentiate internal AssertJ calls from users ones by looking the stack trace to see if the caller of
interceptwas made fromorg.assertjor not.This issue is a minor bugfix since AssertJ does not lead to false positive, as you said if the assertions succeed the test case is covered, and the test would fail if any assertion fails.
Improvements to AssertJ comparison.
JPERF-1160: Assert on impacts softly
JPERF-1160: Assert on impacts softly