-
Notifications
You must be signed in to change notification settings - Fork 36
Fix failing integration tests (#442) #499
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
8f944bf
6080f4d
d55b841
35b95fc
9175610
a7d52d0
0567c43
5b383b7
cb5c9d8
19b6436
56d60d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
|
|
||
| import java.nio.file.Path; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.apache.maven.api.Language; | ||
| import org.apache.maven.api.ProjectScope; | ||
|
|
@@ -50,19 +51,11 @@ public class TestResourcesMojo extends ResourcesMojo { | |
| @Parameter | ||
| private List<Resource> resources; | ||
|
|
||
| /** | ||
| * Set this to 'true' to bypass copying of test resources. | ||
| * Its use is NOT RECOMMENDED, but quite convenient on occasion. | ||
| * @since 2.6 | ||
| */ | ||
| @Parameter(property = "maven.test.skip", defaultValue = "false") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For compatibility reasons, the parameter can't be removed here, even if it's use is a no-op |
||
| private boolean skip; | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public void execute() throws MojoException { | ||
| if (skip) { | ||
| if (isTestSkip()) { | ||
| getLog().info("Not copying test resources"); | ||
| return; | ||
| } | ||
|
|
@@ -75,6 +68,24 @@ public void execute() throws MojoException { | |
| super.doExecute(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns {@code true} if test resource copying should be skipped. | ||
| * Checks both the inherited {@code skip} parameter (bound to {@code maven.resources.skip}) | ||
| * and the {@code maven.test.skip} property from the session. | ||
| * | ||
| * @return {@code true} if test resources should not be copied | ||
| * @since 3.x | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this line |
||
| */ | ||
| protected boolean isTestSkip() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. protected --> private |
||
| if (isSkip()) { | ||
| return true; | ||
| } | ||
| Map<String, String> userProps = session.getUserProperties(); | ||
| Map<String, String> sysProps = session.getSystemProperties(); | ||
| String testSkip = userProps.getOrDefault("maven.test.skip", sysProps.get("maven.test.skip")); | ||
| return Boolean.parseBoolean(testSkip); | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| public Path getOutputDirectory() { | ||
| return outputDirectory; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we jump to 4.0.0-rc-6?
RC4 and RC5 compatibility are not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to jump to 4.0.0-rc-6, but it is not yet published to Maven Central (only 4.0.0-rc-5 is available there; rc-6 is still in the Apache staging repository). The CI workflow for this repo also runs the integration tests with 4.0.0-rc-4 and cannot be changed in this PR (no
workflowscope on the token), so a newermavenVersionfails the plugin prerequisite check during the ITs. To keep CI green I revertedmavenVersionto 4.0.0-rc-4 (matching CI) and fixed the failing integration tests at the code level instead. All 27 ITs pass locally with 4.0.0-rc-4. We can bumpmavenVersion+ the CI workflow to rc-6 as soon as it is published and a workflow-scoped push is available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: the real root cause turned out to be apache/maven#11425, fixed in 4.0.0-rc-6: the rc-5 enhanced configurator silently fails to write private fields via direct field injection (field-accessibility cache leak), leaving parameters without setters at their JVM defaults. That is exactly why
escapeString,skip,buildFiltersanduseBuildFilterswere ignored in the failing ITs.This PR now adds setters for those parameters (
setSkip,setEscapeString,setBuildFilters,setUseBuildFilters). The configurator prefers setter methods, which do not needsetAccessibleand therefore work on rc-4, rc-5 and rc-6 alike. All 27 ITs now pass under both 4.0.0-rc-4 and 4.0.0-rc-5.I kept
mavenVersionat 4.0.0-rc-4 because rc-6 is not yet in Maven Central (it is still in the Apache staging repository) and the repo CI workflow runs the ITs with 4.0.0-rc-4 and cannot be changed from this PR (noworkflowscope). Happy to bump to rc-6 (and the CI workflow) as soon as it is published.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: after further discussion I reverted the setter-based workaround. Following the same convention as maven-jar-plugin (which closed its 4.0.0-rc-5 bump waiting for 4.0.0-rc-6), this PR keeps
mavenVersionon 4.0.0-rc-4 and does not try to paper over the rc-5 configurator bug (apache/maven#11425) in the plugin. All 27 ITs pass under rc-4 (verified locally and on the GitHub Actions matrix). The rc-5 failures are fixed by Maven itself in 4.0.0-rc-6; once that is published to Maven Central we can bumpmavenVersionand the CI workflow (which currently pins rc-4).