Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SoftAssertions nested in satisfies() or allSatisfy() only output first failing assertion #1353

Open
maverick1601 opened this issue Nov 2, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@maverick1601
Copy link

commented Nov 2, 2018

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();
      });
    });
  }
}
@joel-costigliola

This comment has been minimized.

Copy link
Owner

commented Nov 3, 2018

This should work, thanks for reporting this.

@maverick1601

This comment has been minimized.

Copy link
Author

commented 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

This comment has been minimized.

Copy link
Owner

commented Nov 3, 2018

It seems the problems comes from the way AssertJ handles nested proxy calls.

Example:
softly.assertThat(true).isFalse() calls the isEqualTo(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.intercept which happens with satisfies and the assertions inside (hasSize).

org.eclipse.jdt.internal.junit.runner.RemoteTestRunner at localhost:36031	
	Thread [main] (Suspended)	
		ErrorCollector.intercept(ErrorCollector, Object, Callable<?>, Method, Object) line: 64	
		StringAssert$ByteBuddy$ZwZnwlBB.hasSize(int) line: not available	
		StringAssert$ByteBuddy$ZwZnwlBB.hasSize(int) line: not available	
		SoftAssertionsErrorDescriptionTest.lambda$2(SoftAssertions, String) line: 51	
		2052223881.accept(Object) line: not available	
		StringAssert$ByteBuddy$ZwZnwlBB(AbstractAssert<SELF,ACTUAL>).satisfies(Consumer<ACTUAL>) line: 669	
		StringAssert$ByteBuddy$ZwZnwlBB.satisfies$accessor$Qnd4MFMu(Consumer) line: not available	
		StringAssert$ByteBuddy$ZwZnwlBB$AssertJ$SoftProxies$Im2qiR2u.call() line: not available	
		ErrorCollector.intercept(ErrorCollector, Object, Callable<?>, Method, Object) line: 58	
		StringAssert$ByteBuddy$ZwZnwlBB.satisfies(Consumer<String>) line: not available	
		StringAssert$ByteBuddy$ZwZnwlBB.satisfies(Consumer) line: not available	
		SoftAssertionsErrorDescriptionTest.lambda$1(String, SoftAssertions) line: 54	
		1836463382.accept(Object) line: not available	
		SoftAssertions.assertSoftly(Consumer<SoftAssertions>) line: 161	
		SoftAssertionsErrorDescriptionTest.softAssertionTest() line: 47	
		NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
		NativeMethodAccessorImpl.invoke(Object, Object[]) line: 62	
		DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43	
		Method.invoke(Object, Object...) line: 498	
		ReflectionUtils.invokeMethod(Method, Object, Object...) line: 515	
		ExecutableInvoker.invoke(Method, Object, ExtensionContext, ExtensionRegistry) line: 115	
		TestMethodTestDescriptor.lambda$invokeTestMethod$6(ExtensionContext, JupiterEngineExecutionContext) line: 171	
		106374177.execute() line: not available	
		OpenTest4JAwareThrowableCollector(ThrowableCollector).execute(ThrowableCollector$Executable) line: 72	
		TestMethodTestDescriptor.invokeTestMethod(JupiterEngineExecutionContext, Node$DynamicTestExecutor) line: 167	
		TestMethodTestDescriptor.execute(JupiterEngineExecutionContext, Node$DynamicTestExecutor) line: 114	
		TestMethodTestDescriptor.execute(EngineExecutionContext, Node$DynamicTestExecutor) line: 59	
		NodeTestTask<C>.lambda$executeRecursively$5() line: 105	
		957465255.execute() line: not available	
		OpenTest4JAwareThrowableCollector(ThrowableCollector).execute(ThrowableCollector$Executable) line: 72	
		NodeTestTask<C>.executeRecursively() line: 95	
		NodeTestTask<C>.execute() line: 71	
		367746789.accept(Object) line: not available	
		ArrayList<E>.forEach(Consumer<? super E>) line: 1257	
		SameThreadHierarchicalTestExecutorService.invokeAll(List<TestTask>) line: 38	
		NodeTestTask<C>.lambda$executeRecursively$5() line: 110	
		957465255.execute() line: not available	
		OpenTest4JAwareThrowableCollector(ThrowableCollector).execute(ThrowableCollector$Executable) line: 72	
		NodeTestTask<C>.executeRecursively() line: 95	
		NodeTestTask<C>.execute() line: 71	
		367746789.accept(Object) line: not available	
		ArrayList<E>.forEach(Consumer<? super E>) line: 1257	
		SameThreadHierarchicalTestExecutorService.invokeAll(List<TestTask>) line: 38	
		NodeTestTask<C>.lambda$executeRecursively$5() line: 110	
		957465255.execute() line: not available	
		OpenTest4JAwareThrowableCollector(ThrowableCollector).execute(ThrowableCollector$Executable) line: 72	
		NodeTestTask<C>.executeRecursively() line: 95	
		NodeTestTask<C>.execute() line: 71	
		SameThreadHierarchicalTestExecutorService.submit(TestTask) line: 32	
		HierarchicalTestExecutor<C>.execute() line: 57	
		JupiterTestEngine(HierarchicalTestEngine<C>).execute(ExecutionRequest) line: 51	
		DefaultLauncher.execute(TestEngine, ExecutionRequest) line: 170	
		DefaultLauncher.execute(Root, ConfigurationParameters, TestExecutionListener...) line: 154	
		DefaultLauncher.execute(LauncherDiscoveryRequest, TestExecutionListener...) line: 90	
		JUnit5TestReference.run(TestExecution) line: 89	
		TestExecution.run(ITestReference[]) line: 41	
		RemoteTestRunner.runTests(String[], String, TestExecution) line: 541	
		RemoteTestRunner.runTests(TestExecution) line: 763	
		RemoteTestRunner.run() line: 463	
		RemoteTestRunner.main(String[]) line: 209	
	Thread [ReaderThread] (Running)	
@joel-costigliola

This comment has been minimized.

Copy link
Owner

commented Nov 3, 2018

You beat me by 2 min! :D

@joel-costigliola

This comment has been minimized.

Copy link
Owner

commented Nov 3, 2018

One workaround is simply not to use sa to call satisfies:

 SoftAssertions.assertSoftly(sa -> {
      // don't call with like sa.assertThat.satisfies( ... 
      assertThat(str).satisfies(s -> {
        
        // fails
        sa.assertThat(s).hasSize(2);
        
        // not reported, unless all previous assertions succeed
        sa.assertThat(s).isEmpty();
      });
    });

@maverick1601

This comment has been minimized.

Copy link
Author

commented 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

This comment has been minimized.

Copy link
Owner

commented 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:

  public SELF contains(VALUE expectedValue) {
    isNotNull(); // check actual is not null
    if (!actual.isPresent()) throwAssertionError(shouldContain(expectedValue));
    ...

Soft assertion are implemented by proxying assertions and catching any AssertionErrror and report all of them later on.
In the example above, contains and isNotNull() are proxied, if isNotNull() fails the thrown AssertionErrror will 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 contains and 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.intercept from the stack trace, if > 1 it will see an internal call and propagate the error (in ErrorCollector).

Now in your example, you are nesting soft assertions so the the number of calls to ErrorCollector.intercept will be > 1 which leads AssertJ to think that it is in a situation like contains calling isNotNull(), it propagates the AssertionError and 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

This comment has been minimized.

Copy link
Author

commented 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

This comment has been minimized.

Copy link
Owner

commented 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 intercept was made from org.assertj or 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.

@joel-costigliola joel-costigliola added the bug label Nov 7, 2018

@joel-costigliola joel-costigliola added this to the 3.12 milestone Nov 7, 2018

@joel-costigliola joel-costigliola modified the milestones: 3.12, 3.13 Jan 25, 2019

@joel-costigliola joel-costigliola removed this from the 3.13 milestone May 31, 2019

cpovirk added a commit to google/truth that referenced this issue Jul 3, 2019

Improvements to AssertJ comparison.
- AssertJ now supports assumptions, too (joel-costigliola/assertj-core#1156).
- The AssertJ soft-assertion bug I referred to has been fixed (joel-costigliola/assertj-core#1156), though another remains (joel-costigliola/assertj-core#1353).
- AssertJ now has enhanced support for field-by-field comparisons, including recursive comparisons.

Also:

- More clearly call out "soft assertions" / Expect as a similarity, highlighting a few differences.
- Mention that AssertJ supports Guava, too.

And link to new Javadoc.

Thanks to @joel-costigliola for the information in joel-costigliola/assertj-core#1543

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=256370356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.