Assumptions are broken in JUnit4 when opentest4j is in the classpath with 3.17.1

[JoergSiebahn]

Summary

#1983 causes assumptions to fail instead of ignoring the test in edge cases.

This only occurs with JUnit4 when opentest4j is in the classpath, e.g. as transitive dependency from other test utilities like Json-Unit.

JUnit4 does not recognize org.opentest4j.TestAbortedException as expected and fails.

Example

I created a test case for Json-Unit.

package net.javacrumbs.jsonunit.test.junit4;

import org.assertj.core.api.Assumptions;
import org.junit.Test;

public class AssumptionsWithAssertJTest {
    @Test
    public void shouldBeIgnoredButFails() {
        Assumptions.assumeThat("Test").isEqualTo("NotEqualToTest");
    }
}

See also lukas-krecan/JsonUnit#276

[scordio]

Thanks for raising this @JoergSiebahn.

@lukas-krecan is there a particular reason that JsonUnit is providing opentest4j as a transitive dependency instead of being optional?

[lukas-krecan]

JsonUnit supports multiple modes of operation, it can work as standalone assertion library, as Hamcrest macher or as an extension of AssertJ. In some scenarios it throws AssertionFailedError from opentest4j to report multiple failures (I believe that the error is displayed better by IDEs if it's AssertionFailedError).

Introduced to implement this lukas-krecan/JsonUnit#146

[joel-costigliola]

The ideal fix would be for JUnit4 to support org.opentest4j.TestAbortedException but that's not going to happen since the 4.13 is the final JUnit 4 release (unless they changed their mind).

Possible way forward

1. Revert the fix done in 3.17.1 but I'm not super keen to support a library that is being replaced by JUnit 5 TBH.

2. Make opentest4j optional in JsonUnit: JsonUnit would only raise org.opentest4j.MultipleFailuresError or org.opentest4j.AssertionFailedError when opentest4j is in the classpath and fall back to the previous mechanism if not. That's for @lukas-krecan to decide, not me ;-)

3. Migrate to JUnit 5: Might be a chunky piece of work for projects but that's the future.

[scordio]

1. Revert the fix done in 3.17.1 but I'm not super keen to support a library that is being replaced by JUnit 5 TBH.

The important part of #1983 was to have TestNG winning over JUnit 4. We might also push down the priority of opentest4j.
If we go with the following order:

1. org.testng.SkipException
2. org.junit.AssumptionViolatedException
3. org.opentest4j.TestAbortedException

TestNG with JUnit 4 would still work and JsonUnit should also work.

However, I would vote for option 2 as this would make JsonUnit work nicely with both JUnit 4 and 5 use cases. Keen to contribute in case @lukas-krecan agrees.

Option 3 would be the best but timeline and feasibility can vary from project to project.

[lukas-krecan]

I can do 2. but I am not sure if it's the only case when people have opentest4j in the classpath v JUnit 4.

[joel-costigliola]

I can do 2

That would be awesome, thanks!

but I am not sure if it's the only case when people have opentest4j in the classpath v JUnit 4.

We will have the same issue if another testing library strongly depends on opentest4j, how likely is hard to say so I would not worry too much about that and wait for someone to raise an issue.

[joel-costigliola]

I'm ok to follow @scordio suggestion and change the order assumption exception, it's pragmatic solution that does not require any changes to JsonUnit.

[lukas-krecan]

I guess we can do both, I have already made opentest4j optional in JsonUnit, will release it soon.

[scordio]

@joel-costigliola change applied, shall we go with a 3.17.2 release?

[scordio]

Also, JsonUnit 2.19.0 has been released. Thanks @lukas-krecan!

[joel-costigliola]

can do 3.17.2

[JoergSiebahn]

Thank you very much for this amazingly fast response.

Option 3 would be the best but timeline and feasibility can vary from project to project.

You are right in both parts. In our case there are around 50 (micro) services that are based on JUnit4 and there are hundreds more at developing customers that rely on our tooling. It's really a long term task to upgrade. Luckily cases like this are rare compared to total amount of tests.

[asolntsev]

@JoergSiebahn @lukas-krecan @scordio I have upgraded to AssertJ 3.17.2, and my tests failed. :(
I have the opposite case:

• my classpath contains both junit.jar and testh.jar
• I run JUnit tests
• I have assumeThat(false).isTrue(); in my tests.

AssertJ 3.17.2 throws org.testng.SkipException which causes my JUnit test failure. :(

[scordio]

Hi @asolntsev, could you share some more details about why both JUnit 4 and TestNG appear in the classpath, but tests are running with JUnit 4?

BTW your issue is more related to #1961 rather than this issue.

[asolntsev]

@scordio I am developing Selenide - a library for UI tests. It provides several rules/listeners for JUnit4, JUnit5 and TestNG. That's why it must have junit4.jar, junit5.jar, testng.jar in classpath.

A better solution would be to extract those rules/listeners to separate submodules with their own classpaths, but it means more work for us... :)

[scordio]

If I understand it correctly, it is an issue only for Selenide own tests as JUnit 4, 5 and TestNG are not transitive dependencies.

We are trying to satisfy the use cases for the majority of the users and your use case might be tough to be covered without breaking the others.

The current priority order of the assumption exceptions can be found in the breaking changes section of the release notes: https://assertj.github.io/doc/#assertj-core-3-17-2-release-notes

If splitting the project into modules is not an option, there might be different strategies to have proper testing:

• Move classpath dependent tests into separate integration tests, using the maven-invoker-plugin (the strategy I used for AssertJ tests)
• Upgrade to JUnit 5 and manipulate the classpath with a custom JUnit extension (Spring Boot approach)

[asolntsev]

@scordio Don't bother too much :)
Splitting the project into modules is an option. Actually, it's the only valid option. :)
Just wanted to share my today's finding with you. It was quite unexpected that a minor upgrade of assertj could fail our tests.

Ideally, libraries like AssertJ should not depend on JUnit, TestNG or any other testing libraries. Currently you are supporting only JUnit and TestNG, but there may be hundreds of other libraries... It's just not possible to support all of them.
Or well, support for those libraries could also be split to separate AssertJ modules (like assertj-junit4, assertj-junit5, assertj-testng, assertj-geb etc.).

[scordio]

@asolntsev thanks for sharing, this helps us to understand how much the use case matrix is growing 🙂

AssertJ has to depend on the underlying test engine due to the fluent assumptions, which must throw exceptions understandable by the test engine itself. Ideally, there would be no issue to handle those cases if all the test engines would adopt opentest4j.

Unfortunately, this is not the case yet, although with TestNG we started to talk about it in testng-team/testng#2352.