[SUREFIRE-2024] Replace testng-junit5 by testng-engine by slawekjaranowski · Pull Request #500 · apache/maven-surefire (original) (raw)

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without
  pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean install to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing the engine exclusion in this dependency.
In the previous revision, see the POM, the Jupiter version was 5.5.0, therefore we excluded it and the engine and api was removed as well. Adding it in the provider would mean that a new version of the engine and api would be consistent with another version.

Overriding the version of API in the profile junit5-api would mean that the original engine stays. That's not consistent.
So it's better to remove engine as a transitive dependency. The profile's responsibility would be to specify junit artifact.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slawekjaranowski
Here are two ITs. So two POMs for them.
There's a very little difference between both POMs. The dependencies are same in both. You could reach the same with the original POM. We usually alter the dependencies in the Maven profiles, that's the way how it should be done with both ITs. Then we as developers would have a simple situation because we would analyse only one place of IT sources/POM.

Another radical proposition, drop this it tests at all, testng-engine it is not our product it is extension for junit-platform.

So mention in documentation such possibility even without configuration examples and point to product page should be enough.

Our product is provider for junit-platform so we should take care about it not for every extension for junit-platform.

@slawekjaranowski
This is not a bug fix for users. It would not be a problem to postpone this to a later release.
Then we can continue on our discussions longer...

Ok, It is only improvement and can be postpone to a later release.
But we should know that can generate issues for as, like SUREFIRE-2043

@slawekjaranowski
It would be worth to test the TestNG groups as well.
Because it would be interesting to see how the TestNG groups are bound to JUnit5 groups. There can be more such features on both sides.

@slawekjaranowski
If we are so close to cut a new release, we should push this change and we can continue with the discussion and refactoring or joining the POMs in one later. WDYT?
If you would find time to do the last modification it would be worth.
It would be worth to provide it to the customers with working engine.

In the old revision of com.github.testng-team:testng-junit5, the exclusion was because the TestNG team released only one version of the engine with dependency of old junit-platform-engine 1.7.0. So I was interested what happens if we can still use it by override it to 1.8.2. I found that the testng-junit5 still works.
I excluded the dependency and did not override it directly for internal testing purposes and for the reason to avoid a situation that I override Jupiter API to 5.8.2 but junit-platform-engine 1.7.0.

Now with newer version of testng-engine is better I think.
Simply can be used with the newest junit-jupiter-api:5.8.2 and simple configuration should be shown to user.

So I prefer to not propagate workaround with exclusion to users, whose simply copy and paste it.

Is it exclusion is still needed and reasonable?
If it is needed and we show it, we should describe why we use exclusion?

I hope that JUnit team upgrade testng-engine to newest version of junit-platform.

WDYT?

@slawekjaranowski
I did not say that testng-engine has some problem. I only said that Surefire understands Jupiter Api and resolves it into engine, and we only prevent from strange versions combination- see the comment in the POM in the second commit - it is not about bug, it is about clarity of deps.
I added second commit.

Ok, thanks.
I assume that now is ok.
So when build have finished I will merge it.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})