Fix failing integration tests (#442) - #499

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

protected --> private

remove this line

can we jump to 4.0.0-rc-6?

RC4 and RC5 compatibility are not required

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.

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.

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

⚠️ 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:

Then update isTestSkip() to be the single source of truth:

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.

⚠️ 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.

⚠️ 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.

⚠️ 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.

⚠️ 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:

Then update isTestSkip() to incorporate the field check:

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.

⚠️ 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:

Or, if this PR's intent is to add value beyond #500, consider closing it — #500 already merged the equivalent fix.

⚠️ 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.

⚠️ 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 Build against Maven 4.0.0-rc-6, and honour <skip> on testResources #500)
• Or rebasing to remove isTestSkip() and revert to skip || isSkip() if there are other changes worth keeping