[gnodet]

Problem

Fixes #308 / MJAR-138.

The test-jar goal's skip parameter was bound to ${maven.test.skip}. This meant passing -Dmaven.test.skip=true — which also skips test compilation — silently prevented the test-jar from being built. In multi-module projects where a downstream module has a test-jar dependency, this broke the build.

This is a long-standing (2010) tension between two intentional decisions:

• MJAR-90: skip the test-jar when maven.test.skip=true to avoid producing an empty test-jar (tests weren't compiled)
• MJAR-138: but skipping the test-jar breaks cross-module test-jar dependencies

Solution

Rebind skip from ${maven.test.skip} to ${maven.test.jar.skip} (default false).

The MJAR-90 concern (empty test-jar) is already handled by skipIfEmpty in modern versions. There is no longer a reason to couple test-jar packaging to maven.test.skip.

Users who want to skip test-jar packaging should now use -Dmaven.test.jar.skip=true (or <skip>true</skip> in POM config — unchanged).

Users who want to skip test execution without breaking downstream modules should use:

• -DskipTests (Maven 3+)
• --skip-tests (Maven 4.1+, see #13233)

Changes

• TestJarMojo: rebind skip property from maven.test.skip → maven.test.jar.skip
• New IT mjar-138: multi-module build with -DskipTests — verifies model test-jar is built and client can compile against it via the reactor

[gnodet-bot]

Solid fix — decoupling skip from maven.test.skip is the right call now that skipIfEmpty handles the empty-test-jar concern from MJAR-90. The IT covers the core scenario well.

One finding on the @since tag.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-reviewed after 7dee44e — the @since tag is now correctly set to 4.0.0-beta-2. No other issues found.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-reviewed after 7dee44e. Previous finding (incorrect @since 2.2 → @since 4.0.0-beta-2) is addressed.

No new issues. The property rebinding (maven.test.skip → maven.test.jar.skip) is clean, the Javadoc accurately describes the new behavior, and the IT covers the core scenario.

Metadata: no milestone set — suggest 4.0.0-beta-2.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-reviewed after 1a571bc. desruisseaux's isFile() suggestions are both addressed — verify.groovy now uses !testJar.isFile() and !clientTestClasses.isFile(). No new issues.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-reviewed after ec583d2. desruisseaux's assert style suggestion is addressed — verify.groovy now uses idiomatic Groovy assertions instead of the if/return false pattern, consistent with newer ITs. All previous findings resolved, no new issues.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[ascheman]

Thanks — the direction is good: a dedicated maven.test.jar.skip property instead of overloading maven.test.skip, plus the clearer Javadoc. I'm fine with the code change itself.

I do want to push back fairly hard on the integration test, though, because as written it does not cover the fix at all.

src/it/mjar-138 runs clean package -DskipTests. But -DskipTests sets neither the old maven.test.skip binding nor the new maven.test.jar.skip one — so skip is false either way, the model test-jar is built and client compiles with and without this change. Revert the one-line property change on TestJarMojo.skip and re-run the IT: it stays green. So the test exercises a path that was never broken by MJAR-138 and does not distinguish the fixed plugin from the unfixed one — a green-by-design IT that quietly stops guarding the behavior it is named after.

The honest difficulty here is worth confronting in the test rather than side-stepping:

• -Dmaven.test.skip=true is what actually triggered MJAR-138, but it also skips test-compilation, so model's test-jar would be empty and client could not resolve TestComponent — the build would fail even with the fix.
• On master skipIfEmpty defaults to false, so after this change -Dmaven.test.skip=true now produces an empty test-jar where before it produced none. The description says skipIfEmpty already handles the empty-test-jar case; with the default that is not so.

Could we please get an IT that actually fails on the unpatched plugin before this lands — e.g. assert that -Dmaven.test.jar.skip=true suppresses the test-jar while a normal build (or -DskipTests) still produces it — and have both the test and the PR description pin down the maven.test.skip + empty-test-jar contract explicitly?

Happy to write that IT myself and hand it to you (patch against this branch, or a follow-up) if that helps move it along — just say the word. The property and doc change are fine; I'd just really prefer not to merge it with a test that proves nothing about the change.