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

[SUREFIRE-2024] Replace testng-junit5 by testng-engine #500

Merged
merged 2 commits into from Mar 30, 2022

Conversation

slawekjaranowski
Copy link
Member

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.

<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.support</groupId>
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

If engine should be not as a transitive dependency form testng-engin we simply should report a issue for testng-engin not introduce workaround here.

You point me to archived project https://github.com/testng-team/testng-junit5

Now maintenanced is:
https://github.com/junit-team/testng-engine

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 27, 2022

@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.

@slawekjaranowski
Copy link
Member Author

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.

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 27, 2022

@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...

@slawekjaranowski
Copy link
Member Author

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 slawekjaranowski marked this pull request as draft March 27, 2022 18:23
@Tibor17
Copy link
Contributor

Tibor17 commented Mar 27, 2022

@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.

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 29, 2022

@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.

@slawekjaranowski
Copy link
Member Author

@Tibor17
Ok, I removed additional test, and use only existing.

I think that exclusions in most of time it is workaround for problem in target project.

As we see on the project page https://github.com/junit-team/testng-engine there are no exclusions in examples.

And next https://repo1.maven.org/maven2/org/junit/support/testng-engine/1.0.1/testng-engine-1.0.1.pom
has dependency to testng in runtime scope
but for junit-platform-engine they use compile scope.

@slawekjaranowski slawekjaranowski marked this pull request as ready for review March 29, 2022 04:59
@Tibor17
Copy link
Contributor

Tibor17 commented Mar 29, 2022

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.

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 29, 2022

@slawekjaranowski

And next https://repo1.maven.org/maven2/org/junit/support/testng-engine/1.0.1/testng-engine-1.0.1.pom
has dependency to testng in runtime scope
but for junit-platform-engine they use compile scope.

They would appear in our test classpath.
See the table which describes that runtime and compile scoped artifacts would be resolved in test classpath.
https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#dependency-scope

@slawekjaranowski
Copy link
Member Author

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?

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 30, 2022

@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.

@slawekjaranowski
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants