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-1564] ensure provider dependencies can be overriden #193

Conversation

rmannibucau
Copy link
Contributor

either by the plugin definition or through project dependencies (in this order to let the plugin force a transitive dependency of the test stack), fixes the fact JUnit 5 integration is broken with 5.3.0 release

…fintion or through project dependencies (in this order to let the plugin force a transitive dependency of the test stack), fixes the fact JUnit 5 integration is broken with 5.3.0 release
sormuras added a commit to junit-team/junit5-samples that referenced this pull request Sep 4, 2018
Prior to this commit, Surefire was still using JUnit Platform version
1.2.0 and Jupiter/Vintage version 5.2.0 -- now Surefire picks up the
correct version, 1.3.0 and 5.3.0, to launch the Platform successfully.

This is achieved by setting explicit Surefire plugin dependencies.

See also apache/maven-surefire#193
@sormuras
Copy link
Contributor

sormuras commented Sep 4, 2018

You may already override the JUnit Platform and Engine versions by supplying plugin dependency definitions. See

https://github.com/junit-team/junit5-samples/blob/a516d38439dec7be42488a5fd608f99af1d3c0f2/junit5-jupiter-starter-maven/pom.xml#L18-L48

@rmannibucau
Copy link
Contributor Author

@sormuras: this doesnt work - you can check on meecrowave (junit module) project because classpath ignores both peoject and plugin dependencies so you end up using plugin build dependencies for part of the stack.

@rmannibucau rmannibucau changed the title ensure provider dependencies can be overriden [SUREFIRE-1564] ensure provider dependencies can be overriden Sep 5, 2018
@Tibor17
Copy link
Contributor

Tibor17 commented Sep 5, 2018

@sormuras
The POM in platform provider contains junit-platform-launcher.
I think that Romain's problem is with existence of this dependency in the provider classpath. I think it should have <scope>provided</scope>. Then the dependency would not appear in the provider's classpath and the user has to add.
Another way would be more complicated for us but more user friendly for user. Discover the junit-platform-launcher and it transitive dependencies in dependencies and the algorithm would be complicated because it has to resolve them. Basically it has to use the launcher and engine if specified; otherwise use the one with version 1.2.0. But this would lead to uncertain behavior because the users would not clearly understand when to use it and when not in certain versions.

@rmannibucau
Why on why people still use plugin dependencies since the plugin has its own ClassLoader and has nothing to do with project dependencies.

@rmannibucau
Copy link
Contributor Author

@Tibor17 cause you don't want to put the dependency in the project at all since it is about the platform and surefire integration, not the project itself. Even the engine shouldn't be in the project dependencies, only the API. Also this was the way junit5 surefire plugin was done before the donation ;).

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 5, 2018

@sormuras
Is there a guarantee that the junit-platform-launcher is backwards compatible across versions?
If it is and will be guaranteed then the trick with engine would help the users in Surefire documentation. Maybe it would be a good way to provide a link to https://github.com/junit-team/junit5-samples as well.

@sormuras
Copy link
Contributor

sormuras commented Sep 5, 2018

Solved for meecrowave via apache/openwebbeans-meecrowave#11

At least two things are still open:

  1. Improve the documentation to address this setup
  2. Evaluate whether this PR improves Surefire's dependency handling - if yes, apply it, if not, close it.

@sormuras
Copy link
Contributor

sormuras commented Sep 6, 2018

@mureinik reported a side-effect with reuseForks=false over at junit-team/junit5#1579 -- does this PR also fixes this issue?

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 6, 2018 via email

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 18, 2018

@rmannibucau
I would like to reproduce this issue. Can you write a small project in your GitHub repository? Thx

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 18, 2018

@rmannibucau
@sormuras
I will try to test @rmannibucau's test project and scope=provided dependency junit-jupiter-engine in our provider surefire-junit-platform. I think personally that the provider should be only compiled with a help of 3rd party dependencies but the runtime classpath should not use these dependencies because they are subject of declaring by user in POM. This fix would be easy and since every user has to use either junit-jupiter-engine or junit-vintage-engine the transitive dependency junit-platform-launcher appears in both. Is there any engine where the junit-platform-launcher does not exist?

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 18, 2018

@rmannibucau
@sormuras
According to this picture [1] it is obvious that the junit-platform-launcher is always available in any engine. When looking into Surefire's providers like TestNG provider or JUnit4 provider, one can see that the 3rd party dependencies always have scope provided. So I think we should continue in this tradition and I hope the scope would fix our problem.
[1]: https://dzone.com/storage/temp/7348826-dzone1.gif

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 18, 2018

Somebody wants to write a parameterized IT with versions 5.2.0, 5.3.0, 5.3.1?
It is worth to utilize @sormuras' code asserting the classpath 95fa1d8

@filiphr
Copy link

filiphr commented Sep 21, 2018

Just to chime in as we are as well hit by this exact problem in https://github.com/flowable/flowable-engine/tree/master/modules/flowable-cxf. I've tried doing the exact thing that @sormuras did with apache/openwebbeans-meecrowave#11, but it didn't work.

I have not tried it but I think that if one puts reuseForks=true and forkCount=1 (we have forkMode=always which according to the surefire documentation leads to the reuseForks and forkCount that I mentioned) in https://github.com/junit-team/junit5-samples/blob/master/junit5-migration-maven/pom.xml the problem would be shown.

filiphr added a commit to filiphr/flowable-engine that referenced this pull request Sep 21, 2018
@Tibor17
Copy link
Contributor

Tibor17 commented Oct 7, 2018

@rmannibucau
Thx for contributing. We have fixed this issue. Please close this PR.

@rmannibucau rmannibucau closed this Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants