You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "elharo (via GitHub)" <gi...@apache.org> on 2026/08/01 12:23:18 UTC

[PR] Fix failing integration tests with Maven 4.0.0-rc-5 (#442) [maven-resources-plugin]

elharo opened a new pull request, #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499

   ## Summary
   
   Fixes the 4 failing integration tests reported in #442 that occur when building with Maven 4.0.0-rc-5.
   
   ## Root Cause
   
   Maven 4.0.0-rc-5 introduced a bug in `EnhancedCompositeBeanHelper` ([apache/maven#11425](https://github.com/apache/maven/pull/11425)) where field accessibility state was cached globally using `ConcurrentMap<Field, Boolean>`. This cache caused plugin configuration injection to fail when:
   
   - The `buildFilters` field (injected via `${project.build.filters}`) was not properly populated for `TestResourcesMojo` when both `resources` and `testResources` goals ran in the same build
   - The `skip` field in `TestResourcesMojo` shadowed the parent `ResourcesMojo.skip` field (same name, both `private`), confusing the field accessibility cache
   
   ## Failing Tests
   
   - **`escapeInterpolation`**, **`filter-test-resources`**, **`MRESOURCES-77`**: `SomeResource.txt not contains test:filter resolution project.version=1.0-SNAPSHOT` — build filters from `\<build\>\<filters\>` were not applied to test resources because `buildFilters` field injection failed for `TestResourcesMojo`
   - **`MRESOURCES-131`**: `assert !skippedFile.exists()` failed — the `\<skip\>true\</skip\>` plugin config didn't skip `testResources` because `TestResourcesMojo.skip` was not properly injected due to the field shadowing with `ResourcesMojo.skip`
   
   ## Changes
   
   ### 1. `TestResourcesMojo`: Remove the shadowing `skip` field
   
   The `private boolean skip` field in `TestResourcesMojo` (bound to `maven.test.skip`) shadowed the parent's `private boolean skip` field (bound to `maven.resources.skip`). Both having the same field name in the class hierarchy triggered the rc-5 caching bug.
   
   **Fix**: Remove the `@Parameter`-injected `skip` field and replace with `isTestSkip()` which:
   - Delegates to the parent's `isSkip()` (handles `\<skip\>true\</skip\>` in plugin config)
   - Reads `maven.test.skip` directly from session properties (handles `-Dmaven.test.skip=true`)
   
   ### 2. `ResourcesMojo`: Fallback for `buildFilters` field
   
   Add a fallback in `getCombinedFiltersList()` to read build filters directly from `project.getBuild().getFilters()` when the `buildFilters` field is null (not injected). This makes filter loading robust against injection failures.
   
   ### 3. Upgrade `mavenVersion` to `4.0.0-rc-5`
   
   Update the compile target from `4.0.0-rc-4` to `4.0.0-rc-5` so CI tests against the same Maven version that exposed these issues.
   
   ## Notes
   
   The underlying Maven bug is fixed in 4.0.0-rc-6 ([apache/maven#11433](https://github.com/apache/maven/pull/11433)). These plugin-side changes also fix the behavior with rc-5 and make the plugin more robust in general.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4102604889


##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,17 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
      * {@inheritDoc}
      */
     public void execute() throws MojoException {

Review Comment:
   ⚠️ **Still unaddressed (4th review).** `skip` is `@Parameter(property = "maven.test.skip")` — field injection handles `maven.test.skip`. `isTestSkip()` also reads `maven.test.skip` from the session. The `skip ||` duplicates the check.
   
   With master now on rc-7 (field-injection bug fixed), the session-property fallback is dead code. Master's approach from PR #500 (`skip || isSkip()`) is simpler and sufficient:
   
   ```suggestion
           if (skip || isSkip()) {
   ```
   
   Or, if this PR's intent is to add value beyond #500, consider closing it — #500 already merged the equivalent fix.



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -85,6 +77,23 @@ 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.

Review Comment:
   ⚠️ **Javadoc still inaccurate (3rd review).** `isSkip()` checks `maven.resources.skip`, not the "inherited `skip` parameter". The local `skip` field is bound to `maven.test.skip`. The doc conflates the two.
   
   ```suggestion
        * Returns {@code true} if test resource copying should be skipped.
        * Checks {@code maven.resources.skip} via the inherited {@link #isSkip()} and reads
        * {@code maven.test.skip} directly from session properties as a fallback when
        * field injection is unavailable.
   ```



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -85,6 +77,23 @@ 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
+     */
+    private boolean isTestSkip() {
+        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);
+    }

Review Comment:
   ⚠️ **Dead code on rc-6+.** This entire method is a workaround for the rc-5 configurator bug (apache/maven#11425) that silently failed to write private fields. Fixed in rc-6; master is on rc-7. The session-property fallback will never fire because `skip` is correctly injected.
   
   Master already uses `skip || isSkip()` (PR #500) which handles both `maven.test.skip` (via field) and `maven.resources.skip` (via `isSkip()`). This method adds an unused `java.util.Map` import and dead code paths.
   
   Recommend either:
   - Closing this PR (superseded by #500)
   - Or rebasing to remove `isTestSkip()` and revert to `skip || isSkip()` if there are other changes worth keeping



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4102521724


##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,17 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
      * {@inheritDoc}
      */
     public void execute() throws MojoException {
-        // isSkip() reads ResourcesMojo's own field. Both classes declare a private
-        // "skip", so the two collapse into a single descriptor parameter and the
-        // configurator writes the superclass one, leaving this class's field false
-        // however the build configured <skip>. Reading both is what makes
-        // <skip>true</skip> reach this goal at all.
-        //
-        // TODO temporary: drop the isSkip() half once apache/maven#12626 is in a
-        // release. That fixes the cause in the core configurator, where
-        // buildFieldCache() lets a parent field shadow the child's, and then this
-        // class's own field will be configured directly.
-        if (skip || isSkip()) {
+        if (skip || isTestSkip()) {

Review Comment:
   ⚠️ **Still unaddressed from previous review.** `skip` is `@Parameter(property = "maven.test.skip")` — field injection already handles `maven.test.skip`. `isTestSkip()` also reads `maven.test.skip` from the session. The outer `skip ||` duplicates that check.
   
   Moreover, with master now on rc-7 (where the field-injection bug is fixed), the session-property fallback in `isTestSkip()` is unnecessary. The simpler `skip || isSkip()` from master (PR #500) already handles both `maven.test.skip` (via `skip` field) and `maven.resources.skip` (via `isSkip()`).
   
   Suggestion: consider whether this PR should be closed in favor of #500's approach already on master, or rebased to only delete `Jenkinsfile.disable`.



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -85,6 +77,23 @@ 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
+     */
+    private boolean isTestSkip() {
+        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);
+    }

Review Comment:
   ⚠️ **Unnecessary complexity given rc-7.** This entire method is a workaround for the rc-5 configurator bug (apache/maven#11425) that silently failed to write private fields via direct field injection. That bug was fixed in rc-6 and master is now on rc-7.
   
   The session-property fallback (`userProps.getOrDefault("maven.test.skip", sysProps.get(...))`) will never fire on rc-6+ because the `skip` field is correctly injected. This adds dead code and an unnecessary `java.util.Map` import.
   
   **Javadoc is also still inaccurate** (previous finding): the doc says it checks "the inherited `skip` parameter (bound to `maven.resources.skip`)" but `isSkip()` is what checks `maven.resources.skip` — the local `skip` field is bound to `maven.test.skip`, not `maven.resources.skip`.



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,17 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
      * {@inheritDoc}
      */
     public void execute() throws MojoException {
-        // isSkip() reads ResourcesMojo's own field. Both classes declare a private
-        // "skip", so the two collapse into a single descriptor parameter and the
-        // configurator writes the superclass one, leaving this class's field false
-        // however the build configured <skip>. Reading both is what makes
-        // <skip>true</skip> reach this goal at all.
-        //
-        // TODO temporary: drop the isSkip() half once apache/maven#12626 is in a
-        // release. That fixes the cause in the core configurator, where
-        // buildFieldCache() lets a parent field shadow the child's, and then this
-        // class's own field will be configured directly.
-        if (skip || isSkip()) {
+        if (skip || isTestSkip()) {

Review Comment:
   ⚠️ **Still unaddressed from previous review.** `skip` (field injected from `maven.test.skip`) and `isTestSkip()` (reads `maven.test.skip` from session properties) both check the same property — the outer `skip ||` is redundant with the session-based fallback inside `isTestSkip()`.
   
   The cleaner approach remains: move `skip` into `isTestSkip()` so it is the single source of truth for all skip logic:
   
   ```suggestion
           if (isTestSkip()) {
   ```
   
   Then update `isTestSkip()` to incorporate the field check:
   
   ```java
   private boolean isTestSkip() {
       if (skip || 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);
   }
   ```
   
   This makes the intent clear: `isTestSkip()` checks everything, and `execute()` just calls it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r3695640504


##########
pom.xml:
##########
@@ -72,7 +72,7 @@ under the License.
   </distributionManagement>
 
   <properties>
-    <mavenVersion>4.0.0-rc-4</mavenVersion>
+    <mavenVersion>4.0.0-rc-5</mavenVersion>

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #499: Fix failing integration tests (#442)
URL: https://github.com/apache/maven-resources-plugin/pull/499


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests with Maven 4.0.0-rc-5 (#442) [maven-resources-plugin]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r3695599633


##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -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

Review Comment:
   The Javadoc uses a non-specific version marker (`@since 3.x`), which is inconsistent with the rest of the codebase’s concrete `@since` tags (e.g., `@since 3.2.0`, `@since 3.0.0`). Please use a specific version for when this method was introduced (or remove the tag if it’s not intended to be tracked).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#issuecomment-5152471847

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r4093222837


##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -75,6 +77,23 @@ 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.

Review Comment:
   ⚠️ **Javadoc is inaccurate.** The method does NOT check `maven.test.skip` via the injected `skip` parameter — that check is in `execute()`, outside this method. `isTestSkip()` checks (1) `maven.resources.skip` via `isSkip()` and (2) `maven.test.skip` via session properties. The current wording implies both checks live inside `isTestSkip()` and is misleading to readers.
   
   ```suggestion
        * Returns {@code true} if test resource copying should be skipped.
        * Checks {@code maven.resources.skip} (inherited from the parent mojo) and reads
        * {@code maven.test.skip} directly from the session properties as a fallback when
        * field injection is unavailable.
   ```



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -62,7 +64,7 @@ public class TestResourcesMojo extends ResourcesMojo {
      * {@inheritDoc}
      */
     public void execute() throws MojoException {
-        if (skip) {
+        if (skip || isTestSkip()) {

Review Comment:
   ⚠️ **Redundant check — `skip` and `isTestSkip()` both cover `maven.test.skip`.**
   
   The local `skip` field is `@Parameter(property = "maven.test.skip")`. `isTestSkip()` also reads `maven.test.skip` from session properties. So this condition checks the same property twice: once via field injection, once via the session fallback. That's the intent (tolerate rc-5 injection failure), but it means `isTestSkip()` is NOT a clean abstraction — it's a partial duplicate of the outer check.
   
   The cleaner fix is to move all skip logic into `isTestSkip()` and drop the `skip ||` from the outer call:
   
   ```suggestion
           if (isTestSkip()) {
   ```
   
   Then update `isTestSkip()` to be the single source of truth:
   
   ```java
   private boolean isTestSkip() {
       // Check injection-based field first (prefers setter over direct field — works on all rc versions)
       if (skip) {
           return true;
       }
       // Fallback: read maven.test.skip directly from session (tolerates field-injection failure)
       Map<String, String> userProps = session.getUserProperties();
       Map<String, String> sysProps = session.getSystemProperties();
       String testSkip = userProps.getOrDefault("maven.test.skip", sysProps.get("maven.test.skip"));
       if (Boolean.parseBoolean(testSkip)) {
           return true;
       }
       // Also honour the parent's maven.resources.skip (skips all resource processing)
       return isSkip();
   }
   ```
   
   This makes the fallback intent explicit and removes the confusing `skip ||` at the call site.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r3695699165


##########
pom.xml:
##########
@@ -72,7 +72,7 @@ under the License.
   </distributionManagement>
 
   <properties>
-    <mavenVersion>4.0.0-rc-4</mavenVersion>
+    <mavenVersion>4.0.0-rc-5</mavenVersion>

Review 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).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r3695680519


##########
pom.xml:
##########
@@ -72,7 +72,7 @@ under the License.
   </distributionManagement>
 
   <properties>
-    <mavenVersion>4.0.0-rc-4</mavenVersion>
+    <mavenVersion>4.0.0-rc-5</mavenVersion>

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#issuecomment-5151610457

   retest this please


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix failing integration tests with Maven 4.0.0-rc-5 (#442) [maven-resources-plugin]

Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #499:
URL: https://github.com/apache/maven-resources-plugin/pull/499#discussion_r3695600167


##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -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")

Review Comment:
   For compatibility reasons, the parameter can't be removed here, even if it's use is a no-op



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -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

Review Comment:
   remove this line



##########
pom.xml:
##########
@@ -72,7 +72,7 @@ under the License.
   </distributionManagement>
 
   <properties>
-    <mavenVersion>4.0.0-rc-4</mavenVersion>
+    <mavenVersion>4.0.0-rc-5</mavenVersion>

Review Comment:
   can we jump to 4.0.0-rc-6?
   
   RC4 and RC5 compatibility are not required



##########
src/main/java/org/apache/maven/plugins/resources/TestResourcesMojo.java:
##########
@@ -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
+     */
+    protected boolean isTestSkip() {

Review Comment:
   protected --> private



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org