Assertj-core: SoftAssertions for JUnit Jupiter should reset tracked exceptions between tests

[sbrannen]

Overview

The following code looks up tracked exceptions in an instance field.

https://github.com/joel-costigliola/assertj-core/blob/9692f7dc339765e9d7ef2725c89b3241c613268f/src/main/java/org/assertj/core/api/JUnitJupiterSoftAssertions.java#L45

The problem with doing that is that the tracked exceptions do not get reset between test method invocations if the test class is configured with @TestInstance(PER_CLASS) lifecycle semantics, since the test instance is cached along with the instance of JUnitJupiterSoftAssertions stored in an instance field.

I became aware of this due to the following comment in JUnit's issue tracker: https://github.com/junit-team/junit5/issues/1500#issuecomment-459720295

Affected Extensions

• JUnitJupiterSoftAssertions
• JUnitJupiterBDDSoftAssertions
• any other extensions implemented in a similar manner

Proposals

Modify JUnitJupiterSoftAssertions so that it implements JUnit Jupiter's BeforeEachCallback extension API and clears the collected errors in SoftProxies before each test method.

Another option would be to store the collected errors in Jupiter's ExtensionContext.Store for the currently executing scope.

Relate Issues

• #1229

[Sam-Kruglov]

See the exact test case here https://github.com/junit-team/junit5/issues/1756

[sbrannen]

Here is a test case in Java (using @TestMethodOrder support from the upcoming JUnit Jupiter 5.4 release) that reproduces the problem.

import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;

import org.assertj.core.api.JUnitJupiterSoftAssertions;
import org.junit.jupiter.api.MethodOrderer.Alphanumeric;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestMethodOrder;
import org.junit.jupiter.api.extension.RegisterExtension;

@TestMethodOrder(Alphanumeric.class)
@TestInstance(PER_CLASS)
class AssertJSoftAssertionsExtensionTests {

    @RegisterExtension
    final JUnitJupiterSoftAssertions softly = new JUnitJupiterSoftAssertions();

    @Test
    void test1() {
        // should fail
        softly.assertThat(1).isLessThan(0);
    }

    @Test
    void test2() {
        // should pass
        softly.assertThat(0).isLessThan(1);
    }

}

[sbrannen]

As a general rule of thumb, extensions in JUnit Jupiter should never be stateful.

Instead, they should store state in the ExtensionContext.Store whenever possible in order to allow JUnit to properly _scope_ the state for the current test class, current test method, etc.

[sbrannen]

FYI: this also applies to JUnitJupiterBDDSoftAssertions and any extensions implemented in a similar manner.

https://github.com/joel-costigliola/assertj-core/blob/9692f7dc339765e9d7ef2725c89b3241c613268f/src/main/java/org/assertj/core/api/JUnitJupiterBDDSoftAssertions.java#L45

[Sam-Kruglov]

Also if test instance per method is used and both tests are placed inside a @Nested class, then during test2 there are two instances of JUnitJupiterSoftAssertions are doing things, where during the execution of test2, the one created for test2 says SUCCESS and the one created for test1 says FAILURE which results in failure of test2.

import org.assertj.core.api.JUnitJupiterSoftAssertions;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.RegisterExtension;

@TestInstance(TestInstance.Lifecycle.PER_METHOD)
public class Bla {

    @RegisterExtension
    public final JUnitJupiterSoftAssertions softly = new JUnitJupiterSoftAssertions();

    @Nested
    class A {

        @Test
        void test1() {
            softly.assertThat(1).isEqualTo(2);
        }

        @Test
        void test2() {
            softly.assertThat(1).isEqualTo(1);
        }
    }
}

[joel-costigliola]

@sbrannen thanks for reporting this issue with great details, appreciated!

[sbrannen]

You're very welcome.

And... now that I've thought about it again, I think the only robust solution is to store the _current_ instance of SoftProxies in Jupiter's ExtensionContext.Store.

Otherwise, you will also run into issues with parallel test execution as well as the aforementioned issues with test instance per-class lifecycle semantics and nested test class.

[joel-costigliola]

@sbrannen could I get some feedback to see if I'm on the right track addressing this issue ?

To avoid having a SoftProxies field, I have refactored the soft assertions in AssertJ to let each concrete implementation to provide access to a SoftProxies through the getProxies() method.

Except for the JUnit5 ones, all concrete soft assertions implementation classes define their own SoftProxies field with getProxies() to access it.

Where I need your feedback is for the Junit5 implementations as the one below:

public class JUnitJupiterSoftAssertions extends AbstractStandardSoftAssertions implements BeforeEachCallback, AfterEachCallback {

  private static final Namespace ASSERTJ_JUNIT_EXTENSION_NAMESPACE = Namespace.create("AssertJ");
  private static final String SOFT_PROXIES_KEY = "JUnitJupiterSoftAssertions SoftProxies";
  private final AssertionErrorCreator assertionErrorCreator = new AssertionErrorCreator();
  private Store store;

  // is used by errorsCollected() which gets the errors from the SoftProxies instance in use when the tests ran.
  @Override
  protected SoftProxies getProxies() {
    return store.get(SOFT_PROXIES_KEY, SoftProxies.class);
  }

  @Override
  public void afterEach(ExtensionContext extensionContext) {
    List<Throwable> errors = errorsCollected();
    if (!errors.isEmpty()) assertionErrorCreator.tryThrowingMultipleFailuresError(errors);
  }

  @Override
  public void beforeEach(ExtensionContext extensionContext) throws Exception {
    store = extensionContext.getStore(ASSERTJ_JUNIT_EXTENSION_NAMESPACE);
    store.put(SOFT_PROXIES_KEY, new SoftProxies());
  }
}

This implementation makes the AssertJSoftAssertionsExtensionTests test case to behave correctly with test2 passing (same for @Sam-Kruglov test case).

What I'm not too sure of is if it's ok to have a Store field but if I don't have this field I don't know how to get the SoftProxies in the getProxies() implementation. Is there a way to get the current ExtensionContext in getProxies() ? If so I could get rid of the field.

Another question is whether I can reuse ASSERTJ_JUNIT_EXTENSION_NAMESPACE in JUnitJupiterBDDSoftAssertions, I believe it makes sense to reuse it as it is the global assertj namespace.

[joel-costigliola]

@sbrannen pinging you for a comment if you have time ;-)

[joel-costigliola]

@sbrannen or @Sam-Kruglov could have a look at https://github.com/joel-costigliola/assertj-core/pull/1530/files#diff-111b89b286b9f4ba07422f1e8c1e6200 and tell me if that looks correct to you ?

I have created a branch for this fix, so it is possible to test this version on a real code base, simply run mvn install -Dmaven.test.skip=true to get it built.

[sbrannen]

@sbrannen pinging you for a comment if you have time ;-)

Sorry for being incommunicado. I'll try to find time to review your work today or tomorrow.

[sbrannen]

TBH, I can't really see a feasible way to support soft assertions via an extension registered via @RegisterExtension using the ExtensionContext.Store.

It may be that I am overlooking something, but I currently think you'll have to go with a ThreadLocal to achieve the proper behavior.

I have reworked several of the classes in my local fork which I may share later, but for the time being I'll share my current drafts for the two Jupiter extensions.

public class JUnitJupiterSoftAssertions extends AbstractStandardSoftAssertions implements BeforeEachCallback, AfterEachCallback {

  private final AssertionErrorCreator assertionErrorCreator = new AssertionErrorCreator();
  private final ThreadLocal<SoftProxies> softProxies = new ThreadLocal<>();

  @Override
  protected SoftProxies getProxies() {
    return this.softProxies.get();
  }

  @Override
  public void beforeEach(ExtensionContext extensionContext) {
    this.softProxies.set(new SoftProxies());
  }

  @Override
  public void afterEach(ExtensionContext extensionContext) {
    try {
        List<Throwable> errors = errorsCollected();
        if (!errors.isEmpty()) this.assertionErrorCreator.tryThrowingMultipleFailuresError(errors);
    }
    finally {
      this.softProxies.remove();
    }
  }

}

public class JUnitJupiterBDDSoftAssertions extends AbstractBDDSoftAssertions implements BeforeEachCallback, AfterEachCallback {

  private final AssertionErrorCreator assertionErrorCreator = new AssertionErrorCreator();
  private final ThreadLocal<SoftProxies> softProxies = new ThreadLocal<>();

  @Override
  protected SoftProxies getProxies() {
    return this.softProxies.get();
  }

  @Override
  public void beforeEach(ExtensionContext extensionContext) throws Exception {
    this.softProxies.set(new SoftProxies());
  }

  @Override
  public void afterEach(ExtensionContext extensionContext) {
    try {
        List<Throwable> errors = errorsCollected();
        if (!errors.isEmpty()) this.assertionErrorCreator.tryThrowingMultipleFailuresError(errors);
    }
    finally {
      this.softProxies.remove();
    }
  }

}

Please let me know if you are comfortable with a ThreadLocal based solution, and if so, I can submit a PR.

[sbrannen]

Another option would be for the extension to implement ParameterResolver and then inject an instance of SoftAssertions into each method that needs it.

I think that would actually be cleaner, since it should theoretically work via the ExtensionContext.Store and would no longer require a ThreadLocal.

Such an extension would likely be better registered via @ExtendWith instead of @RegisterExtension since the field would no longer be useful to the user.

[sbrannen]

To answer your questions...

To avoid having a SoftProxies field, I have refactored the soft assertions in AssertJ to let each concrete implementation to provide access to a SoftProxies through the getProxies() method.

I think that's a good idea in general; however, the implementation can be improved with regard to proper encapsulation. I have those changes in my local fork, and I can provide them in case you decide to go that route.

This implementation makes the AssertJSoftAssertionsExtensionTests test case to behave correctly with test2 passing (same for @Sam-Kruglov test case).

Yes, it does appear to work; however, I am a bit uncomfortable with the implementation in that it is not _idiomatic_ for a JUnit Jupiter extension.

What I'm not too sure of is if it's ok to have a Store field but if I don't have this field I don't know how to get the SoftProxies in the getProxies() implementation.

That is the part that bothered me the most. It seems to work for the test cases in place, but I generally would recommend against that pattern.

Is there a way to get the current ExtensionContext in getProxies() ? If so I could get rid of the field.

Yes, that would be ideal! Unfortunately, you have not overlooked anything: there is currently no mechanism for accessing the "current" ExtensionContext outside of a dedicated extension callback. Though, we (the JUnit 5 team) may consider adding such functionality if the need arises more often.

Another question is whether I can reuse ASSERTJ_JUNIT_EXTENSION_NAMESPACE in JUnitJupiterBDDSoftAssertions, I believe it makes sense to reuse it as it is the global assertj namespace.

I would recommend against that: you don't want any kind of clashes. Typically it is best to scope the visibility of your objects using a Namespace based on the current extension (e.g., via Namespace.create(getClass())), and if you need that to be more fine-grained you can pass additional arguments to the Namespace.create() method.

If you have further questions, just let me know.

And... if you'd like me to submit a PR (perhaps just a spike) for the ParameterResolver alternative, let me know.

[joel-costigliola]

@sbrannen thanks for the thorough explanation!

Naive question, is it possible (or even reasonable) for JUnit to provide a ThreadLocal Store ?

If we use the ParameterResolver approach, is this how a typical test would look like ?

@ExtendWith(SoftAssertionsParameterResolver.class)
public class JUnitJupiterSoftAssertionsSuccessTest {

  @Test
  public void all_assertions_should_pass(JUnitJupiterSoftAssertions softly) {
    softly.assertThat(1).isEqualTo(1);
    softly.assertThat(list(1, 2)).containsOnly(1, 2);
  }
}

Is that approach compatible with parameterized tests ? Where would the resolved parameter be injected ?

@ParameterizedTest
@ValueSource(strings = { "racecar", "radar", "able was I ere I saw elba" })
void palindromes(String candidate, JUnitJupiterSoftAssertions softly) {
    softly.assertThat(StringUtils.isPalindrome(candidate)).isTrue();
}

Provided ParameterResolver plays nicely with parameterized tests, I'll be happy to review a spike/PR implementing it. Otherwise we can use the thread local approach you suggested.

[sbrannen]

Naive question, is it possible (or even reasonable) for JUnit to provide a ThreadLocal Store ?

We are considering providing access to the _current_ ExtensionContext via a ThreadLocal mechanism that would allow you to access the current Store, though we do not yet have a ticket for that as far as I recall.

So feel free to open a new JUnit 5 GitHub issue requesting that.

If we use the ParameterResolver approach, is this how a typical test would look like ?

I would say that the resolved parameter type should simply be SoftAssertions. I don't see any need to force the user to know about a JUnit Jupiter specific implementation detail.

So I think it would look more like this:

@ExtendWith(SoftAssertionsParameterResolver.class)
class JUnitJupiterSoftAssertionsSuccessTest {

   @Test
   void all_assertions_should_pass(SoftAssertions softly) {
      softly.assertThat(1).isEqualTo(1);
      softly.assertThat(list(1, 2)).containsOnly(1, 2);
   }
}

Is that approach compatible with parameterized tests ?

Yes.

Where would the resolved parameter be injected ?

With a @ParameterizedTest, the parameterized arguments must always come first. Arguments resolved by a ParameterResolver come at the end of the argument list. That is documented here.

Provided ParameterResolver plays nicely with parameterized tests, I'll be happy to review a spike/PR implementing it. Otherwise we can use the thread local approach you suggested.

Now that I've answered your questions, let me know your current thoughts.

[joel-costigliola]

I'm happy with the ParameterResolver approach, it would be great if you can create a PR for that otherwise I can give it a shot provided you'll review my work ;-).

I guess MockitoExtension would be a good example to follow.

[sbrannen]

I'm happy with the ParameterResolver approach, it would be great if you can create a PR for that

OK. I'll see if I can quickly put something together over the coming days.

otherwise I can give it a shot provided you'll review my work ;-).

Sure. If I don't submit a PR, I'd be glad to review your work.

I guess MockitoExtension would be a good example to follow.

Yes, you could use that for inspiration; however, that's a bit more involved than what you'll need.

[sbrannen]

OK. I've already implemented a spike.

/*
 * Copyright 2012-2019 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.assertj.core.api.junit.jupiter;

import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.extension.AfterEachCallback;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
import org.junit.jupiter.api.extension.ExtensionContext.Store;
import org.junit.jupiter.api.extension.ParameterContext;
import org.junit.jupiter.api.extension.ParameterResolver;

/**
 * Extension for JUnit Jupiter that provides support for injecting an
 * instance of {@link SoftAssertions} into test methods and test-level lifecycle
 * methods (i.e., {@code @BeforeEach} and {@code @AfterEach} methods).
 *
 * <h3>Applicability</h3>
 *
 * <p>In this context, the term "test method" refers to any method annotated with
 * {@code @Test}, {@code @RepeatedTest}, {@code @ParameterizedTest},
 * {@code @TestFactory}, or {@code @TestTemplate}. This extension does not inject
 * {@link SoftAssertions} arguments into test constructors or class-level lifecycle
 * methods.
 *
 * <h3>Scope</h3>
 *
 * <p>The scope of the {@code SoftAssertions} instance managed by this extension
 * begins when a parameter of type {@code SoftAssertions} is resolved for a
 * {@code @BeforeEach} method, test method, or {@code @AfterEach} method. The
 * scope of the instance ends after all {@code @AfterEach} methods have been
 * executed for the corresponding test. When the scope ends,
 * {@link SoftAssertions#assertAll() assertAll()} will invoked on the instance
 * to verify that no soft assertions failed.
 *
 * <h3>Example</h3>
 *
 * <pre class="code">
 * {@literal @}ExtendWith(SoftAssertionsExtension.class)
 * class ExampleTestCase {
 * 
 *    {@literal @}Test
 *    void multipleFailures(SoftAssertions softly) {
 *       softly.assertThat(2 * 3).isEqualTo(0);
 *       softly.assertThat(Arrays.asList(1, 2)).containsOnly(1);
 *       softly.assertThat(1 + 1).isEqualTo(2);
 *    }
 * }</pre>
 *
 * @author Sam Brannen
 * @since 3.13
 */
public class SoftAssertionsExtension implements ParameterResolver, AfterEachCallback {

    @Override
    public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
        boolean isTestLevel = extensionContext.getTestMethod().isPresent();
        boolean isSoftAssertionsParameter = parameterContext.getParameter().getType() == SoftAssertions.class;
        return (isTestLevel && isSoftAssertionsParameter);
    }

    @Override
    public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
        return getStore(extensionContext).getOrComputeIfAbsent(SoftAssertions.class);
    }

    @Override
    public void afterEach(ExtensionContext extensionContext) throws Exception {
        SoftAssertions softAssertions = getStore(extensionContext).get(SoftAssertions.class, SoftAssertions.class);
        if (softAssertions != null) {
            softAssertions.assertAll();
        }
    }

    private Store getStore(ExtensionContext extensionContext) {
        return extensionContext.getStore(Namespace.create(getClass(), extensionContext.getUniqueId()));
    }

}

[sbrannen]

And here's a test class to play around with it.

/*
 * Copyright 2012-2019 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.assertj.core.api.junit.jupiter;

import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;

import java.util.Arrays;

import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestMethodOrder;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

/**
 * Tests for {@link SoftAssertionsExtension}.
 *
 * @author Sam Brannen
 */
@ExtendWith(SoftAssertionsExtension.class)
@TestMethodOrder(OrderAnnotation.class)
@TestInstance(PER_CLASS)
class SoftAssertionsExtensionTests {

    @BeforeEach
    void beforeEach(SoftAssertions softly) {
        softly.assertThat(1).isEqualTo(1);
    }

    @Test
    @Order(1)
    void oneAssertionShouldFail(SoftAssertions softly) {
        softly.assertThat(1).isEqualTo(1);
        softly.assertThat(Arrays.asList(1, 2)).containsOnly(1);
    }

    @Test
    @Order(2)
    void allAssertionsShouldPass(SoftAssertions softly) {
        softly.assertThat(1).isEqualTo(1);
        softly.assertThat(Arrays.asList(1, 2)).containsOnly(1, 2);
    }

    @ParameterizedTest
    @CsvSource({ "1, 1, 2", "1, 2, 3" })
    void parameterizedTest(int a, int b, int sum, SoftAssertions softly) {
        softly.assertThat(a + b).isEqualTo(sum);
        softly.assertThat(a).isEqualTo(b);
    }

}

[sbrannen]

If you're happy with the direction it's going, I'll submit a PR with real tests (likely using the EngineTestKit).

[sbrannen]

I pushed my work to a branch in my fork, in case that makes it easier for you to view: https://github.com/sbrannen/assertj-core/commits/issues/1418-jupiter-soft-assertions

[rweisleder]

Should the SoftAssertionsExtension also resolve parameters of type BDDSoftAssertions?

[sbrannen]

Should the SoftAssertionsExtension also resolve parameters of type BDDSoftAssertions?

That could either be handled by a dedicated BddSoftAssertionsExtension, or SoftAssertionsExtension could potentially be reworked to support both types (with a bit of added complexity).

[sbrannen]

Should the SoftAssertionsExtension also resolve parameters of type BDDSoftAssertions?

That was easy enough, so I went ahead and implemented that in https://github.com/sbrannen/assertj-core/commit/79ae922cce8bec36c6647449ab51294cb95807f6. 😉

[joel-costigliola]

@thanks for the work, looking at your branch right now.

[sbrannen]

After putting more thought into it, I realized that it never makes sense to have a SoftAssertions instance scoped beyond a specific method -- for example, a @Test method.

Rationale: if the extension scopes the tracked exceptions for the SoftAssertions instance across @BeforeEach, @Test, and @AfterEach methods, any tracked exceptions from the @BeforeEach method do not actually cause a failure until _after_ the @AfterEach methods.

And that makes zero sense: if there is a failure in a @BeforeEach method, the @Test method should never execute.

One solution is to limit the scope to the actual @Test method, which I'm about to push a commit for to my branch for the SoftAssertionsExtension.

Another solution would be to allow the extension-managed SoftAssertions to be used in any method (including lifecycle methods such as @BeforeEach methods), but for that to work you would have to be able to intercept the invocation of lifecycle methods which will not be available until JUnit 5.5 is released.

JUnit 5.5 is scheduled to be released very soon, but you may not want to require JUnit 5.5 for use with this extension in AssertJ.

[joel-costigliola]

I have just commented the PR I have created from your branch, ignore the comments if they are not relevant anymore. Let me know when it is ready / good enough for a review.

IMO the scope should only be the @Test methods, @BeforeEach or @AfterEach are not places to perform assertions so that rules out the dependency on JUnit 5.5.

[sbrannen]

IMO the scope should only be the @Test methods, @BeforeEach or @AfterEach are not places to perform assertions.

I just made that the case in https://github.com/sbrannen/assertj-core/commit/88736c26762d34191daa82fa48607bbc360caa2a. 😉

[sbrannen]

I have just commented the PR I have created from your branch, ignore the comments if they are not relevant anymore. Let me know when it is ready / good enough for a review.

I have addressed your comments and made changes.

At this point, I think the SoftAssertionsExtension implementation is nearly complete (if not already complete). So, feel free to review that.

The tests, however, are just a playground, and some of them fail. So that's obviously not up for review yet. I'll have to use the EngineTestKit to test those properly.

[joel-costigliola]

The SoftAssertionsExtension looks good :)

I'm keen to see how you are going to use the EngineTestKit, that will be some good learning for me.

[sbrannen]

I'm keen to see how you are going to use the EngineTestKit, that will be some good learning for me.

I added proper tests in https://github.com/sbrannen/assertj-core/commit/dc74f341bc42d0138c868dd54791b25da04c949e.

[sbrannen]

And in https://github.com/sbrannen/assertj-core/commit/a55d8709a8fdb32ca2d85947280bda6bb0c596ca, the extension now throws an exception if the user accidentally _requests_ parameter resolution for a constructor or non-test method.

I think it's best to fail the test in such cases as opposed to silently ignoring the misconfiguration. JUnit Jupiter would of course throw an exception anyway if the parameter cannot be resolved, but the explicit approach I've taken provides a contextual error message explaining why the parameter cannot be resolved.

[sbrannen]

@joel-costigliola, if you're happy with the basic approach and status quo, I'll squash the commits and submit a proper PR.

[joel-costigliola]

@sbrannen I'm happy with the approach :) waiting for your PR.

[sbrannen]

@sbrannen I'm happy with the approach :)

I squashed the commits and added an appropriate commit message in https://github.com/sbrannen/assertj-core/commit/1f0747b10317eaeb5bac34e4dc9178ee4f6b4679.

waiting for your PR.

Unfortunately, I cannot currently submit a PR, as explained in https://github.com/joel-costigliola/assertj-core/pull/1537#issuecomment-508063568.

🤷🏻‍♂️

[sbrannen]

If the SoftAssertionsExtension is accepted, the AssertJ team will need to decide the fate of JUnitJupiterSoftAssertions and JUnitJupiterBDDSoftAssertions.

1. Should they be deprecated?
2. Should they be fixed with the aforementioned ThreadLocal workaround as an interim solution for those people who already use them?
3. Should they be deprecated and fixed?

In any case, I think the Javadoc for JUnitJupiterSoftAssertions and JUnitJupiterBDDSoftAssertions needs to be updated to point users to the new SoftAssertionsExtension, and the known shortcomings should be documented as well.

If you'd like, I can raise a separate issue to address those concerns.

[joel-costigliola]

I'll deprecate them in favor of the extension, I don't think they must be fixed.

[sbrannen]

I'm keen to see how you are going to use the EngineTestKit, that will be some good learning for me.

And... now that you've seen it in action, what do you think?

[joel-costigliola]

Being able to run tests, retrieve a bunch of information about them and verify they ran as expected is really powerful :clap:.

The fluent API is nice but slightly misleading sometimes (I have copied the code below for the sake of discussion), for example the first time I read execute().tests() I thought it was for triggering tests executions, in that case testsEvents or recordedTestsEvents would have worked better for me (same thing with failed). Using testsEvents would make the API more consistent with assertThatEvents which mention events.

When I integrated the PR I had just changed the format of MultipleFailuresError, this made the tests to fail (for the right reasons) but finding the error was not easy with the long error message from the haveExactly assertion (I know this is AssertJ :laughing:). IMHO it is an overuse of Condition and this is basically why I'm not a big fan of them (I got them when I forked Fest Assert so I could not really get rid of them).
I would use ... soft assertions in that case. :wink:

Here's an example of the error message that I got:

 to have exactly 2 times <all of:<[all of:<[is a test, descriptor with uniqueId substring 'multipleFailures']>, all of:<[type is FINISHED, event with result where all of:<[status is FAILED, all of:<[throwable matches instance of org.assertj.core.error.AssertJMultipleFailuresError, throwable matches message matches predicate]>]>]>]>>

It would benefit from putting each AllOf conditions message on a new lines with some indentation. I just created https://github.com/joel-costigliola/assertj-core/issues/1548 for that.

EngineTestKit.engine("junit-jupiter")
             .selectors(selectClass(testClass))
             .execute().tests()
             .assertStatistics(stats -> stats.started(nested ? 8 : 4).succeeded(nested ? 4 : 2).failed(nested ? 4 : 2))
             .failed()
             // @format:off
             .assertThatEvents().haveExactly(nested ? 2 : 1,
                                             event(test("multipleFailures"),
                                                   finishedWithFailure(instanceOf(AssertJMultipleFailuresError.class),
                                                                       message(msg -> msg.contains("Multiple Failures (2 failures)")))))
                                .haveExactly(nested ? 2 : 1,
                                             event(test("parameterizedTest"),
                                                   finishedWithFailure(instanceOf(AssertJMultipleFailuresError.class), 
                                                                       message(msg -> msg.contains("Multiple Failures (1 failure)")))));

[sbrannen]

Being able to run tests, retrieve a bunch of information about them and verify they ran as expected is really powerful 👏.

Glad to hear you like it in that regard. For me, that is the biggest benefit. We (the JUnit 5 team) use it internally, and I have used it to test the SpringExtension as well.

The fluent API is nice but slightly misleading sometimes (I have copied the code below for the sake of discussion), for example the first time I read execute().tests() I thought it was for triggering tests executions, in that case testsEvents or recordedTestsEvents would have worked better for me (same thing with failed). Using testsEvents would make the API more consistent with assertThatEvents which mention events.

Mea culpa: I'm the one who wrote that fluent API. At the time it seemed nice and succinct, but I can see that it might be slightly confusing when merely reading the code. Migrating from all(), containers(), and tests() to allEvents(), containerEvents(), and testEvents(), respectively, seems like it might be a good idea.

Would you like to to open an issue against JUnit 5 for that?

When I integrated the PR I had just changed the format of MultipleFailuresError, this made the tests to fail (for the right reasons) but finding the error was not easy with the long error message from the haveExactly assertion

Yes, to be honest, I'm also not happy with the failure messages from AssertJ in that regard. I actually usually end up temporarily adding debug(System.err) to the pipeline in order to better assess the failure.

(I know this is AssertJ 😆).

Mmmmmmmm........ 😉

IMHO it is an overuse of Condition and this is basically why I'm not a big fan of them (I got them when I forked Fest Assert so I could not really get rid of them).
I would use ... soft assertions in that case. 😉

We (the JUnit 5 team) are also not totally happy with the use of Condition like that.

So, we would be very grateful for any concrete suggestions for improvements there -- for example, in the form of a proposal or PR!

It would benefit from putting each AllOf conditions message on a new lines with some indentation. I just created #1548 for that.

I wholeheartedly agree. Thanks for creating #1548. I'm looking forward to that improvement in the user experience!

[joel-costigliola]

Mea culpa: I'm the one who wrote that fluent API. At the time it seemed nice and succinct, but I can see that it might be slightly confusing when merely reading the code. Migrating from all(), containers(), and tests() to allEvents(), containerEvents(), and testEvents(), respectively, seems like it might be a good idea.
Would you like to to open an issue against JUnit 5 for that?

I'll be happy to, would that be a bug report or a feature request ? it looks like https://github.com/junit-team/junit5/issues/new/choose does not give me an improvement issue type.

We (the JUnit 5 team) are also not totally happy with the use of Condition like that.
So, we would be very grateful for any concrete suggestions for improvements there -- for example, in the form of a proposal or PR!

I can play with it a bit and see if I can come with something more user friendly, I'll have a look after I release AssertJ Core 3.13.0, this release has been way to much delayed.

[sbrannen]

I'll be happy to, would that be a bug report or a feature request ?

Feature request / enhancement, but don't worry about the labels. We'll take care of that once we've triaged the issue.

it looks like junit-team/junit5/issues/new/choose does not give me an improvement issue type.

Just click on the Open a regular issue link below those options. 😉

I can play with it a bit and see if I can come with something more user friendly, I'll have a look after I release AssertJ Core 3.13.0, this release has been way to much delayed.

Sounds good, and... no rush.

[slackwing]

@sbrannen - Came here just to find out why JUnitJupiterSoftAssertions was deprecated, ended up learning so much about JUnit5 extensions. Your patient and detailed explanations are truly inspiring, thanks and keep it up.