Add the PIT plugin dependencies to the SUT classpath

[Vampire]

Before only the artifact with the plugin itself was added to the SUT classpath.
Due to that plugins needed to shade their dependencies.
Now the transitive dependencies are properly determined from the Maven model
and added to the SUT so that the shading is not necessary anymore.

[hcoles]

I think this PR is solving the wrong problem.

Including the dependencies of the plugins is not desirable in the general case. If a plugin depends on version X of a library and the SUT depends on version Y, we do not want to pollute the classpath of the SUT with version X.

So, generally shading is the correct soloution for pitest plugins. We want them to be able to depend on other without any danger of them breaking things when included in a project with the same dependency.

JUnit 5 is a special case. If we don't want to shade it we'd have to cherry pick the version that the SUT is using off its classpath.

[Vampire]

Well, then the shading should be done with relocation though.
Otherwise you still pollute the classpath with the same classes, just hidden in the plugin's jar.
That would have the same negative effect than if the plugin dependencies would be added to the end of the SUT classpath.

I'm not sure whether including a JUnit 5 special-case here would be a good idea as long as the JUnit 5 plugins is a separate thing.

So maybe then this PR should be closed and in the JUnit 5 plugin instead of removing the shaded classes they should additionally be relocated?

And then for consistency, the Gradle plugin by @szpak should most probably also switch to non-transitive inclusion of the plugin artifacts. Because currently you do have that pollution for Gradle projects, but not for Maven projects.

[hcoles]

Dependencies for other plugins are relocated, but JUnit platform can't be. It needs to be able to interact with the engine installed by the SUT.

The "correct" soloutions are

1. Cherry pick the platform off the SUT classpath.
2. Don't shade platform and require the user to double declare it (once for surefire, and again as a dependency for the plugin)

The current soloution is a fudge, but I think still preferable to 2.

And yes, if the gradle plugin is polluting the classpath that should probably be fixed (although in practice since the only plugins in common use are the junit5 and arcmutate, which properly relocates its dependencies this shouldn't matter).

[Vampire]

I don't think cherry-picking the platform off the SUT classpath would work.
The platform actually is already on the SUT classpath or the tests would not run anyway.
The only thing missing as far as I have seen is the junit-platform-launcher as that is not part of the SUT classpath but only part of the JUnit 5 plugin.
So this dependency could not be picked off the SUT classpath.

[hcoles]

@Vampire I think you're probably correct, in which case launcher ought to be relocatable and everything should work.

[Vampire]

So what's the plan now?

• change in the Gradle plugin to use it non-transitive
• close this PR unmerged
• change in the JUnit 5 plugin to only shade the launcher but with relocation?

[hcoles]

I've just confirmed that everything runs fine with a shaded launcher, so I think that is the plan (although the decision on whether to modifying the gradle plugin is up to @szpak).

The "more correct" thing to do would still be to pull the launcher off the SUT classpath and add it to the classpath of the jvms pitest launches. Although it is there, I think it gets added artifically by surefire without being declared as a proper dependency, which is why it's not already being added (this is from memory, so I could be wrong).

Fixing that would be a lot more complicated however. Running with just a shaded launcher is a quick easy improvement.

[Vampire]

It probably is just used by surefire from its plugin classpath, without it being on the SUT classpath.
As I said, the SUT classpath - i.e. the test class path - is completely added to the Minion classpath which is why Jupiter e.g. is present just fine.
You would need to pull the launcher from the pitest plugin classpath and add it to the Minon classpath, which is exactly what this PR does, just generically for all declared dependencies of the plugin.

[hcoles]

Surefire adds the launcher to the SUT classpath, you can confirm this by adding a test like

    @Test
    void launcher() {
        assertThatCode(() ->
                Class.forName("org.junit.platform.launcher.TestIdentifier")
        ).doesNotThrowAnyException();
    }

To a maven project.

I forget quite how it makes it onto the SUT classpath, but it is not via a standard route, which is why pitest is not able to add it by reading the dependency graph.

Ideally, the junit 5 plugin would be using the same version of the launcher as the rest of the project as there may be an api incompatibility if the project's version is later than pitest's. This will result in some sort of error whether we are using our own shaded version, or using the SUT version, but it is less likely to be a silent error if we use the SUT one.

Pulling the launcher from the pitest plugin classpath just changes the way in which we depend on the wrong version of the launcher, with the added disadvantage that a plugin could pollute the classpath with other things if we don't limit it to the launcher only. Shading the launcher looks wrong. Pulling the dependency from the plugin classpath looks less wrong, but doesn't actually improve the situation, it just makes it less obvious.

[Vampire]

Oh, interesting.
I wonder whether this is intended, or also just an unnice side-effect.

[Vampire]

From a quick cursory look I'd say Surefire searches for org.junit.platform:junit-platform-engine or if absent org.junit.platform:junit-platform-commons and then uses the org.junit.platform:junit-platform-launcher artifact with the same version as the found one.

PIT could of course do the same, but that would mean JUnit 5 specific code in the Maven and Gradle plugin, unless the plugin interface is extended so that somehow the JUnit 5 plugin could provide that information somehow to both.

[hcoles]

@Vampire given that this is causing noise again with people upgrading to the milestone release of 1.10 I'm going to put in the easiest form of the "correct" fix.

The maven plugin will auto-add a matching version of platform-launcher in a junit5 specific cludge built into pitest (can look at somehow expressing dependency requirements in the test plugin api later).

Gradle users will need to add platform-launcher to the test classpath explicitly until the same functionality is implemented in the gradle plugin.

[Vampire]

I don't think Gradle users will immediately be impacted.
If you remove the shading of the launcher, e.g. by merging my PR, the launcher will be in the POM as dependency.
As the Gradle plugin does not yet use the plugin non-transitively, it will get the launcher dependency the plugin is built against.
Except of course if we also change that dependency to "provided" which would probably make sense ultimately.

So for Gradle users there will probably be no change to the current situation, unless it is changed to "provided", with it working if the launcher is compatible.
Until the Gradle plugin switches to also do the logic you implemented now for using the "correct" launcher version.

For reference, this is the according Gradle plugin issue: szpak/gradle-pitest-plugin#337