TestNG 7.3.0 breaks AssertJ assumeThat

[C-Otto]

TestNG Version

7.3.0

Expected behavior

Using AssertJ's org.assertj.core.api.Assumptions.assumeThat method should produce a org.testng.SkipException, skipping the test.

Actual behavior

The code in Assumptions throws org.junit.AssumptionViolatedException instead. This exception is not supported by TestNG, causing a test failure instead of a skipped test.

This is because TestNG includes JUnit 4.12, which is detected by Assumptions.

Is the issue reproductible on runner?

• Shell
• Maven
• Gradle
• Ant
• Eclipse
• IntelliJ
• NetBeans

Test case sample

Gradle: implementation 'org.assertj:assertj-core:3.12.1'

public class FooTest
{
    @Test
    public void test()
    {
        org.assertj.core.api.Assumptions.assumeThat(false).isTrue();
    }
}

[C-Otto]

Caused by fee81b4 which exposes JUnit classes to the runtime classpath (api), instead of just using it for compilation (compileOnly).

[C-Otto]

Please do not expose JUnit and the other dependencies unless strictly necessary.

[scordio]

As we are going to release AssertJ version 3.17.0 soon, we could introduce a workaround in AssertJ for this issue but it would be great to know if TestNG has any plan to address it.

[joel-costigliola]

I was hoping we could use org.opentest4j.TestSkippedException but that fails the test instead of skipping it.
I think the best way forward would be to define a common exception in opentest4j to express skipping tests.

Thoughts?

[scordio]

This issue should be addressed in #2358.

[krmahadevan]

@scordio - I am not sure if TestNG would be going through with a release soon. We just got 7.3.0 released.

@cbeust @juherr - Your thoughts ?

[juherr]

TestNG should not expose JUnit classes by default but both TestNG and JUnit classes are available when the JUnit support feature is used.

IMO, It should be fixed in AssertJ first and improve the dependencies of TestNG asap.

[scordio]

@krmahadevan @juherr as far as I understand, the TestNG dependencies have been fixed in #2358, which is already merged. Would it be an option to release a bugfix version soon?

To be precise, on AssertJ side there is nothing wrong right now. We check which classes are in the classpath to derive the underlying test framework, in the following order:

1. org.junit.AssumptionViolatedException (JUnit 4)
2. org.opentest4j.TestAbortedException (JUnit 5)
3. org.testng.SkipException (TestNG)

See: https://github.com/joel-costigliola/assertj-core/blob/a36275ecd89ffa12cca931e65b3c9e4fa5322f7f/src/main/java/org/assertj/core/api/Assumptions.java#L1295-L1306

As TestNG exposes JUnit 4 classes transitively, the current AssertJ implementation assumes that the test is running on JUnit 4. The workaround I proposed would check TestNG first and then JUnit.

AssertJ 3.17.0 is already out, but we might release 3.17.1 soon due to other issues. However, having a new release from TestNG would be the preference to address this issue.

[scordio]

Also, as mentioned by @joel-costigliola, it would be interesting to know if there is any plan on adopting opentest4j.

[juherr]

Thanks for the details.

I don't see yet the emergency of a TestNG release.
As I said, having both JUnit and TestNG in the classpath is a feature, not a bad practice.

I think the logic is a bit more complicate:

• junit 4 + testng => maybe testng with the junit feature
• junit 5 + testng => maybe junit5 with https://github.com/testng-team/testng-junit5
• other => current logic

it would be interesting to know if there is any plan on adopting opentest4j.

No current plan :(

[scordio]

@juherr sorry, I misunderstood your previous message and I was focusing only on the transitive dependency aspect. I got only now that both frameworks together is a proper use case if the user decides to have both in the classpath.

I will then change the order of AssertJ checks.

[scordio]

No current plan :(

Would you be interested in a PR about it?

[krmahadevan]

@juherr @cbeust - The link https://github.com/ota4j-team/opentest4j#projects-already-contacted says TestNG has been contacted regarding this. Are you folks aware of this ?

I don't recall seeing any mail on this on the TestNG dev users list.

@scordio - Maybe you can create a PR (so the technical work related to moving towards this would be done), but we need to get a confirmation from @cbeust on what the plans are for this.

@juherr - WDYT ?

[juherr]

Are you folks aware of this?

Yes, we were contacted by mail years ago but without enough free time to contribute.

@juherr - WDYT?

It's adding a new dependency and I'm not sure to understand the value-add of the 6 classes but at the same time, it is not a big deal if it can help someone.
I'm just wondering if it is possible to add the support into an independent packaging without over-engineering.

we need to get a confirmation from @cbeust on what the plans are for this.

+1, I let the final words for @cbeust

[cbeust]

I just checked opentest4j and the project has had a handful of commits over the past two years, so it looks pretty dead. What's the advantage of supporting it?

[C-Otto]

I think it's just a bunch of interfaces/abstract classes, so there shouldn't be the need for lots of development. If TestNG implemented those interfaces / extended those classes, tools like AssertJ could simplify the code base.

[scordio]

I just checked opentest4j and the project has had a handful of commits over the past two years, so it looks pretty dead. What's the advantage of supporting it?

I wouldn't say it's dead, it's just there was no need of so much activity.

The main advantage I see is in the handling of the cases mentioned by @juherr:

I think the logic is a bit more complicate:

* junit 4 + testng => maybe testng with the junit feature
* junit 5 + testng => maybe junit5 with https://github.com/testng-team/testng-junit5
* other => current logic

Having the same exception thrown by both TestNG and JUnit would simplify the matrix of all the possible cases that libraries like AssertJ need to handle.

[scordio]

assertj-core 3.17.1 has been released. TestNG exception now takes precedence over JUnit 4.

@C-Otto could you please try again and let us know?

[C-Otto]

@scordio confirmed. Fails with TestNG 7.3.0 and AssertJ 3.17.0, fixed with 7.3.0 / 3.17.1.

[krmahadevan]

@C-Otto - Can we close this issue since its now taken care of at AssertJ side ?