Fix failing integration tests (#442) - #499
Conversation
Maven 4.0.0-rc-5 introduced a bug in EnhancedCompositeBeanHelper where field accessibility state was cached globally, which could cause plugin configuration injection to fail when the same configuration field is accessed multiple times or in different contexts during a build (e.g., when both 'resources' and 'testResources' goals run in the same build). This caused 4 integration test failures: - escapeInterpolation, filter-test-resources, MRESOURCES-77: build filters were not applied to test resources because the buildFilters field was not properly injected for TestResourcesMojo - MRESOURCES-131: the skip configuration did not work for testResources because TestResourcesMojo.skip was not properly injected Fixes: 1. TestResourcesMojo: Remove the private 'skip' field that shadows the parent class field (ResourcesMojo.skip), which triggered the rc-5 field accessibility caching bug. Replace with isTestSkip() method that checks: - The inherited isSkip() (handles <skip>true</skip> in plugin config) - The 'maven.test.skip' session property (handles -Dmaven.test.skip=true) 2. ResourcesMojo: Add fallback in getCombinedFiltersList() to read build filters directly from the project model when the buildFilters field was not properly injected. This makes filter loading robust against field injection failures in Maven 4 when processing class hierarchies. 3. Upgrade mavenVersion from 4.0.0-rc-4 to 4.0.0-rc-5 so the CI tests against the Maven version that exposed these issues. The Maven-side fix is in 4.0.0-rc-6 (apache/maven#11433), but these plugin-side changes ensure correct behavior with Maven 4.0.0-rc-5 as well. Closes #442
There was a problem hiding this comment.
Pull request overview
This PR addresses Maven 4.0.0-rc-5 integration test failures by making test-resources execution more resilient to Maven’s field-injection edge cases and by aligning the project’s target Maven version with the failing environment.
Changes:
- Removes
TestResourcesMojo’s shadowingskipfield and replaces it withisTestSkip()that checks both the inherited pluginskipparameter andmaven.test.skipfrom the session. - Adds a fallback in
ResourcesMojo#getCombinedFiltersList()to read build filters directly from the project model whenbuildFiltersinjection is missing. - Bumps
<mavenVersion>to4.0.0-rc-5inpom.xml.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java | Avoids skip field shadowing and adds session-based maven.test.skip handling. |
| src/main/java/org/apache/maven/plugins/resources/ResourcesMojo.java | Makes build filter resolution robust when buildFilters is not injected. |
| pom.xml | Updates the Maven prerequisite/target version to 4.0.0-rc-5. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * and the {@code maven.test.skip} property from the session. | ||
| * | ||
| * @return {@code true} if test resources should not be copied | ||
| * @since 3.x |
| * Its use is NOT RECOMMENDED, but quite convenient on occasion. | ||
| * @since 2.6 | ||
| */ | ||
| @Parameter(property = "maven.test.skip", defaultValue = "false") |
There was a problem hiding this comment.
For compatibility reasons, the parameter can't be removed here, even if it's use is a no-op
| * @return {@code true} if test resources should not be copied | ||
| * @since 3.x | ||
| */ | ||
| protected boolean isTestSkip() { |
There was a problem hiding this comment.
protected --> private
| * and the {@code maven.test.skip} property from the session. | ||
| * | ||
| * @return {@code true} if test resources should not be copied | ||
| * @since 3.x |
|
|
||
| <properties> | ||
| <mavenVersion>4.0.0-rc-4</mavenVersion> | ||
| <mavenVersion>4.0.0-rc-5</mavenVersion> |
There was a problem hiding this comment.
can we jump to 4.0.0-rc-6?
RC4 and RC5 compatibility are not required
There was a problem hiding this comment.
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 workflow scope on the token), so a newer mavenVersion fails the plugin prerequisite check during the ITs. To keep CI green I reverted mavenVersion to 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 bump mavenVersion + 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.
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, buildFilters and useBuildFilters were 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 need setAccessible and 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 mavenVersion at 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 (no workflow scope). Happy to bump to rc-6 (and the CI workflow) as soon as it is published.
There was a problem hiding this comment.
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 mavenVersion on 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 bump mavenVersion and the CI workflow (which currently pins rc-4).
Keep the maven.test.skip parameter for backward compatibility, make isTestSkip() private, and drop the non-specific @SInCE 3.x tag. Also revert mavenVersion to 4.0.0-rc-4 so the plugin prerequisite matches the Maven version CI uses to run the integration tests.
|
retest this please |
|
I think the problem at root is that maven-resources-plugin is tripping over a bug in RC5 and Jenkins does not have RC4 installed. TBH we should drop jenkins. |
Summary
Keeps
mavenVersionon 4.0.0-rc-4 and makestestResourcesskip handling robust. All 27 invoker ITs pass under Maven 4.0.0-rc-4 (mvn -P run-its verify).Context
The 4 failing integration tests from #442 (
escapeInterpolation,filter-test-resources,MRESOURCES-77,MRESOURCES-131) fail only when the plugin is run with Maven 4.0.0-rc-5. That is caused by a Maven bug in the enhanced configurator (apache/maven#11425, fixed in apache/maven#11433 / 4.0.0-rc-6): private mojo fields are silently not populated when configured via direct field injection. Workarounds in the plugin would be needed to cope with that rc-5 bug, but the same convention used by sibling plugins (e.g. maven-jar-plugin, which closed its rc-5 bump waiting for rc-6) is to stay on a working version and upgrade once rc-6 is released. rc-6 is not yet published to Maven Central.Changes
TestResourcesMojo: robust skip handlingexecute()now skips when either themaven.test.skipparameter is set orisTestSkip()returns true.isTestSkip()delegates to the inheritedmaven.resources.skipparameter and also honors amaven.test.skipuser/system property. This is version-independent and fixes the case where skipping test resources is configured in ways the bare field binding misses.Verification