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

Remove shaded JUnit platform #82

Merged
merged 1 commit into from
May 19, 2023
Merged

Conversation

Vampire
Copy link
Contributor

@Vampire Vampire commented May 8, 2023

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.

@Vampire Vampire force-pushed the remove-shaded-platform branch from e058bc3 to 3d68697 Compare May 9, 2023 13:09
@hcoles
Copy link
Contributor

hcoles commented May 9, 2023

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 Vampire force-pushed the remove-shaded-platform branch from 3d68697 to 82ac4b0 Compare May 9, 2023 14:00
@Vampire
Copy link
Contributor Author

Vampire commented May 9, 2023

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
Copy link
Contributor Author

Vampire commented May 12, 2023

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
Copy link
Contributor Author

Vampire commented May 12, 2023

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

@hcoles
Copy link
Contributor

hcoles commented May 18, 2023

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 Vampire force-pushed the remove-shaded-platform branch from 82ac4b0 to dbc7a86 Compare May 19, 2023 12:26
@Vampire
Copy link
Contributor Author

Vampire commented May 19, 2023

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?

@hcoles hcoles merged commit 0c843e2 into pitest:master May 19, 2023
@Vampire Vampire deleted the remove-shaded-platform branch May 19, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants