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-1396: Provider class path is incorrect for custom provider in Failsafe #161

Merged
merged 1 commit into from Aug 29, 2017

Conversation

Projects
None yet
4 participants
@jon-bell
Contributor

jon-bell commented Jul 29, 2017

Hi,
When using a custom Surefire provider with Surefire (not Failsafe), the "provider classpath" contains only the provider and surefire-api. However, when using a custom provider with Failsafe, the provider class path ends up including a lot more... it seems like perhaps all plugins that are loaded? This has caused some mayhem for me when using a custom provider in projects that use a specific version of SLF4J... because then failsafe forces 1.5.6 to be loaded (from this process of incorrectly finding the custom provider), causing a crash.

This PR contains an integration test case showing the bug and a patch to solve it.

<name>Test provider</name>
<properties>
<surefire.version>2.21-SNAPSHOT</surefire.version>

This comment has been minimized.

@Tibor17

Tibor17 Jul 31, 2017

Contributor

Pls do not use this property. It comes from parent pom.
Did you run the build like this mvn install -P run-its?

This comment has been minimized.

@jon-bell

jon-bell Jul 31, 2017

Contributor

I did a mvn verify from the top level. I will do a mvn install -P run-its now.

This comment has been minimized.

@Tibor17

Tibor17 Jul 31, 2017

Contributor

You should specify parent pom

<parent>
    <groupId>org.apache.maven.surefire</groupId>
    <artifactId>it-parent</artifactId>
    <version>1.0</version>
    <relativePath>../pom.xml</relativePath>
  </parent>

This comment has been minimized.

@jon-bell

jon-bell Aug 1, 2017

Contributor

Sorry about that - hadn't noticed that it wasn't specified. Added it to both the provider and the test project.

<dependencies>
<dependency>
<groupId>org.apache.maven.surefire</groupId>
<artifactId>surefire-api</artifactId>

This comment has been minimized.

@Tibor17

Tibor17 Jul 31, 2017

Contributor

Why?
See the build.log in target and you will see project version and all necessary dependencies hidden in the fork transitive deps.

<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>

This comment has been minimized.

@Tibor17

Tibor17 Jul 31, 2017

Contributor

This plugin is again in parent pom. Not necessary here.

This comment has been minimized.

@jon-bell

jon-bell Jul 31, 2017

Contributor

Thanks. I will clean this up. FYI, the entire pom.xml is copied from the SUREFIRE-141 IT pom.xml.

Artifact plugin = pluginArtifactMap.get( "org.apache.maven.plugins:maven-surefire-plugin" );
Class<?> c = AbstractSurefireMojo.this.getClass();
Artifact plugin;
if ( c.getName().equals( "org.apache.maven.plugin.failsafe.IntegrationTestMojo" ) )

This comment has been minimized.

@Tibor17

Tibor17 Jul 31, 2017

Contributor

I guess this is in method getProviderClasspath()
Please split the method in two. First it would call protected abstract Artifact getMojoArtifact() and then the original statement return dependencyResolver.addProviderToClasspath( pluginArtifactMap, plugin ); where plugin is Artifact. Then force both subclasse to implement getMojoArtifact which means surefire mojo will implement it as follows:
final Map<String, Artifact> pluginArtifactMap = getPluginArtifactMap(); Artifact plugin = pluginArtifactMap.get( "org.apache.maven.plugins:maven-surefire-plugin" );.

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Jul 31, 2017

Thanks - I made the fixes you suggested above, and confirmed that the build passed on mvn clean install -P run-its

@Tibor17

This comment has been minimized.

Contributor

Tibor17 commented Aug 1, 2017

@jon-bell
Please squash your commits into one single commit and override your branch. Name the final commit
[SUREFIRE-1396] Provider class path is incorrect for custom provider in Failsafe

@jon-bell jon-bell force-pushed the jon-bell:SUREFIRE-1396 branch from 0acd7e4 to f68ddab Aug 1, 2017

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Aug 1, 2017

Done

@Tibor17

This comment has been minimized.

Contributor

Tibor17 commented Aug 3, 2017

@jon-bell
I will investigate this issue:
Could not find artifact org.apache.maven.surefire:it-parent:pom:1.0 in central (https://repo.maven.apache.org/maven2)

@jon-bell jon-bell force-pushed the jon-bell:SUREFIRE-1396 branch from f68ddab to 7effbc9 Aug 5, 2017

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Aug 5, 2017

@Tibor17 It seems like it had worked for me because I had previously executed a mvn install - I see on a clean maven repo the problem.

It looks like the SurefireLauncher does not unpack/install the parent pom into the it-repo, and hence, the forked maven process can't find the parent pom (once it's installed). I am hesitant to poke at this further, because I suspect a fix might involve some changes to SurefireLauncher to coerce it to install the parent pom into the it-repo as well. This seems to not be a problem for any test packages, but is for a provider (which gets the forked Maven classloader instead of the test classloader). Given that the only other provider in the repo for integration tests does not reference the parent pom, I assume that this is a problem that someone came upon before and sidestepped.

I've removed the parent reference that you had suggested above. But, we are properly picking up surefire.version from the maven system property. On a clean maven repo the mvn clean install -P run-its completed with this version (now again squashed). I agree that referencing the parent pom is a cleaner solution, but I'm not sure if you/we want to go down the road of patching the integration test harness to support that (for what amounted to a ~3 line patch).

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Aug 10, 2017

@Tibor17 Let me know if there is anything else I can do on this. My new patch (rebased into a single commit) works fine on a mvn clean install -P run-its on a clean m2 repo. It does not reference the parent pom, but also does not define the surefire.version property.

Thanks
Jonathan

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Aug 23, 2017

@Tibor17 Is there anything else I can do to help get this merged?

@Tibor17

This comment has been minimized.

Contributor

Tibor17 commented Aug 28, 2017

@jon-bell
Ok, now your IT passed. I will run whole test set now.

@asfbot

This comment has been minimized.

asfbot commented Aug 28, 2017

Tibor Digana on dev@maven.apache.org replies:
Check your branch if it is up to date with master.

@jon-bell jon-bell force-pushed the jon-bell:SUREFIRE-1396 branch from 7effbc9 to 99cd4f6 Aug 29, 2017

@jon-bell

This comment has been minimized.

Contributor

jon-bell commented Aug 29, 2017

@Tibor17 I have rebased master on top of my branch, it is again up to date.

@asfgit asfgit merged commit 99cd4f6 into apache:master Aug 29, 2017

@Tibor17

This comment has been minimized.

Contributor

Tibor17 commented Aug 29, 2017

@jon-bell
Thx for contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment