search

pitest / pitest-junit5-plugin

JUnit 5 test framework support for Pitest
Apache License 2.0
74 stars 27 forks source link

Remove shaded JUnit platform #82

Closed Vampire closed 1 year ago

Vampire commented 1 year ago

It is present as dependency anyway and even if not it makes no sense to depend on this module without platform being present already anywy and shading it without relocation easily leads to conflicts.

hcoles commented 1 year ago

This is not as straight forward as it first appears. Currently, if the junit-5 plugin is added as a dependency for pitest in a maven project, it will work without having to explicitly any other depencies to pitest (e.g jupiter). Without the shaded platform it will fail with a runtime error.

Unfortunately, I can't remember the details of why jupiter gets picked up from the classpath automatically but platform doesn't. Would need some digging before this could be merged.

Vampire commented 1 year ago

Hm, ok, this sounds strange. I think I tested it with a Gradle build and it worked fine. But then, it was 3 AM, who knows what I did. :-D

Vampire commented 1 year ago

No, I did test with Gradle and there it works perfectly fine. Only in Maven it does not work. You add the complete test classpath to the Minions classpath. Additionally, you check for ClientClasspathPlugin implementations, but there only add the concrete JAR to the Minion classpath, requiring the plugin to have Maven group as implementation vendor manifest attribute and the Maven artifact as implementation title manifest attribute to identify the jar from the class.

But by this you completely ignore the dependencies those plugins might need which is why you currently need to shade the platform launcher dependency which effectively is the only external dependency. Jupiter is found because it is part of the test classpath which is added to the Minion classpath completely.

I see four (at least) possible ways to mitigate the need for the shading:

  1. Declare the necessary dependencies also as manifest attributes, so they can also be added to the Minion classpath
  2. Have a method in ClientClasspathPlugin that returns a list of necessary dependencies, so they can also be added to the Minion classpath
  3. Require the plugin to be added to the test classpath instead, then it is added to the Minion classpath including its dependencies and it just works even without the shading
  4. Determine the necessary dependencies from the Mojo data that is already present
Vampire commented 1 year ago

I took the approach 4: hcoles/pitest#1209 So to work with Maven nicely, hcoles/pitest#1209 is a prerequisite for this change.

hcoles commented 1 year ago

To function correctly with the SUT version of JUnit 5, the shading needs to be removed and the junit platform dependency needs to be changed to a provided dependency. The maven and gradle plugins then need to be updated to autoadd junit-platform-launcher in the same manner that surefire does.

The maven part of this will be released in pitest 1.14.0. I'm not sure how easy/difficuly this will be for gradle, but the dependency can be added manually until the functionality is available.

Vampire commented 1 year ago

As discussed in szpak/gradle-pitest-plugin#337 I adapted the commit message and added a compatibility note to PIT 1.14.0 to the readme, so I guess this PR should now be fine for 1.2.0?