Run test-scope dependencies as named modules with module-info-patch.args (#3090) - #3395
Run test-scope dependencies as named modules with module-info-patch.args (#3090)#3395ascheman wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Surefire’s JPMS fork-argument generation for Maven 4 module-source-hierarchy projects so that module-info-patch.args (from maven-compiler-plugin 4.x) becomes the authoritative runtime handoff, including moving test-scope dependencies referenced by --add-modules onto the module path to run as named modules (fixing #3090).
Changes:
- Introduce
ModuleInfoPatchArgsFileand wire it intoAbstractSurefireMojo+ModularClasspathForkConfigurationto pass handoff directives through verbatim and adjust fallback JPMS argument generation. - Implement module-path migration for test-scope modules (plus transitive/reverse-requires closure) when the handoff file is present, and generate
--add-openstargets accordingly. - Add/extend IT fixtures and integration tests covering Maven 4 module source hierarchy (single + multi-module) and the #3090 “test deps as named modules” behavior.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/pom.xml | Maven 4.1.0 multi-module-source-hierarchy IT project definition. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/main/java/module-info.java | Core module descriptor (requires transitive jakarta.json). |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/main/java/com/example/core/Calculator.java | Core exported API used by the extra module/tests. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/main/java/com/example/core/internal/MathHelper.java | Core non-exported internal helper for whitebox tests. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/test/java/module-info-patch.maven | Core test patch file to drive compiler handoff args. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/test/java/com/example/core/CalculatorTest.java | Core tests including module-path dependency exercise (jakarta.json). |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.core/test/java/com/example/core/internal/MathHelperWhiteboxTest.java | Core whitebox test for non-exported package access. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/main/java/module-info.java | Extra module descriptor (requires core). |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/main/java/com/example/extra/Doubler.java | Extra module class calling into core and internal helper. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/main/java/com/example/extra/internal/TwiceHelper.java | Extra module internal helper for whitebox testing. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/test/java/module-info-patch.maven | Extra test patch file to drive compiler handoff args. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/test/java/com/example/extra/DoublerTest.java | Extra module unit test. |
| surefire-its/src/test/resources/surefire-3393-multi-module-source-hierarchy/src/com.example.extra/test/java/com/example/extra/internal/TwiceHelperWhiteboxTest.java | Extra module whitebox test for non-exported access. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/pom.xml | Maven 4.1.0 single-module-source-hierarchy IT project definition. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/main/java/module-info.java | Single module descriptor for Maven 4 source-hierarchy layout. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/main/java/com/example/Calculator.java | Single-module main code used by tests. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/main/java/com/example/internal/MathHelper.java | Single-module internal helper for whitebox tests. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/test/java/module-info-patch.maven | Test patch file producing compiler handoff args. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/test/java/com/example/CalculatorTest.java | Single-module test coverage. |
| surefire-its/src/test/resources/surefire-3345-module-source-hierarchy/src/com.example/test/java/com/example/internal/MathHelperWhiteboxTest.java | Single-module whitebox test coverage. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/pom.xml | Maven 4.1.0 IT project to assert test deps run as named modules. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/main/java/module-info.java | Probe module descriptor. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/main/java/com/example/probe/Widget.java | Exported API used by probe tests. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/main/java/com/example/probe/internal/Secret.java | Non-exported internal helper for whitebox test assertion. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/test/java/module-info-patch.maven | Patch file that expands TEST-MODULE-PATH in handoff args. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/test/java/com/example/probe/ModulePlacementTest.java | Behavioral assertions for named-module placement of JUnit API. |
| surefire-its/src/test/resources/surefire-3090-test-deps-on-module-path/src/com.example.probe/test/java/com/example/probe/internal/SecretWhiteboxTest.java | Whitebox access assertion under patched module. |
| surefire-its/src/test/resources/modulepath-whitebox/pom.xml | Standard-layout modulepath whitebox fixture (Maven 3 compatible). |
| surefire-its/src/test/resources/modulepath-whitebox/src/main/java/module-info.java | Standard-layout module descriptor. |
| surefire-its/src/test/resources/modulepath-whitebox/src/main/java/com/example/Calculator.java | Standard-layout main code used by tests. |
| surefire-its/src/test/resources/modulepath-whitebox/src/main/java/com/example/internal/MathHelper.java | Standard-layout internal helper for whitebox tests. |
| surefire-its/src/test/resources/modulepath-whitebox/src/test/java/com/example/CalculatorTest.java | Standard-layout modular test coverage. |
| surefire-its/src/test/resources/modulepath-whitebox/src/test/java/com/example/internal/MathHelperWhiteboxTest.java | Standard-layout modular whitebox test coverage. |
| surefire-its/src/test/java/org/apache/maven/surefire/its/ModulePathWhiteboxIT.java | IT wrapper for the standard-layout whitebox fixture. |
| surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire3345ModuleSourceHierarchyIT.java | Maven 4-only IT for module source hierarchy support (#3345). |
| surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire3393MultiModuleSourceHierarchyIT.java | Maven 4-only IT for multi-module-in-one-POM source hierarchy (#3393). |
| surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire3090TestDepsOnModulePathIT.java | Maven 4-only IT for #3090 dependency placement and handoff pass-through. |
| surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/MavenLauncher.java | Default to forked IT launcher for Maven 4+ (embedded supports Maven 3 only). |
| maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/ModuleInfoPatchArgsFileTest.java | Unit tests for parsing module-info-patch.args (single-line/two-line forms). |
| maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfigurationTest.java | Unit tests asserting verbatim handoff pass-through vs fallback behavior. |
| maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoJava7PlusTest.java | Unit test updates for new modular-path behavior and handoff probing. |
| maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ResolvePathResultWrapper.java | Carry additional module descriptor results for multi-module source hierarchy. |
| maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ModuleInfoPatchArgsFile.java | New parser/holder for META-INF/maven/module-info-patch.args. |
| maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ModularClasspathForkConfiguration.java | Fork argfile generation updated to honor the handoff file as authoritative. |
| maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java | Module descriptor resolution, classpath/module-path split, and module moving logic. |
| // once for the patch directory, once probing for module-info-patch.args | ||
| verify(mojo, times(2)).getTestClassesDirectory(); |
| // Use forked mode for Maven 4+ (Embedded3xLauncher only supports Maven 3.x) | ||
| this.forkJvm = isMaven4Plus(); |
| args.append(line).append(NL); | ||
| } | ||
| // the patched module itself must still be a resolution root | ||
| args.append("--add-modules").append(NL).append(moduleName).append(NL); |
There was a problem hiding this comment.
Maybe replace the first NL by a space for consistency with the options added above. The goal is to make the file not only machine readable, but also human readable. I think that it is a little bit easier to understand when we have one option (including its value) per line.
|
Please correct me if I'm wrong, but I have the impression that the strategy implemented in this pull request is to analyse the classpath and determine automatically which dependencies to move to the module path, based on whether the dependency is a module referenced by the project. This is indeed the only strategy possible with Maven 3, but Maven 4 offers a quite different approach in the "give control to developer" spirit. If the Maven Surefire community agrees, I think that the time has come to create a Then, I suggest to do the pull requests in the opposite order: focus on the class-path versus modul-path issue first (the precondition in the third bullet of this issue description) because, as the description rightfully said, it is a critical prerequisite on which all other work will be based on. In the current state of #3392 and #3395, the class-path versus module-path issue could not be resolved first because, if done automatically (as my understanding on the current state of this pull request), this process is outside developer's control and would therefore break existing projects. But if done in the Maven 4 way, this process is fully under developer's control, and therefore can be done first without breaking any existing projects or tests. The Maven 4 way is to not try to guess where to put the dependencies (except in a compatibility mode discussed below), but instead invite the developers to said explicitly what they want. It can be done with a code like below (I'm copying some lines from the Maven Compiler Plugin with some editions). Note that the same code can be used for both modular and non-modular project, this is the purpose of the var allowedTypes = EnumSet.of(JavaPathType.CLASSES);
if (isModular) {
allowedTypes.add(JavaPathType.MODULES);
}
DependencyResolver resolver = session.getService(DependencyResolver.class);
DependencyResolverResult dependencies = resolver.resolve(DependencyResolverRequest.builder()
.session(session)
.project(project)
.requestType(DependencyResolverRequest.RequestType.RESOLVE)
.pathScope(PathScope.TEST_RUNTIME)
.pathTypeFilter(allowedTypes)
.build());
Map<PathType, List<Path>> dependencies = dependencyResolution.getDispatchedPaths();With above code, the Maven 4 core makes its decision based on the |
The IT fixture defaults to embedded launching via Embedded3xLauncher
(maven-shared-verifier), which looks up org.apache.maven.cli.MavenCli.
Maven 4 renamed the entry point to org.apache.maven.cling.MavenCling
with a different signature, so every embedded launch against Maven 4
aborts with NoSuchMethodException before any test runs.
Detect Maven 4 by the presence of maven-api-core in ${maven.home}/lib
and default forkJvm to true in that case; Maven 3 keeps embedded mode.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maven 4 with maven-compiler-plugin 4.x compiles module source hierarchy projects to a nested layout (target/classes/<module>/, target/test-classes/<module>/) and emits a runtime handoff file META-INF/maven/module-info-patch.args derived from module-info-patch.maven. Surefire handled neither: - findModuleDescriptor() only looked for module-info.class at the build output root, so detection failed and execution silently fell back to the classpath. - DirectoryScanner read the module directory as a package prefix, producing doubled FQCNs and "Tests run: 0" or "Unable to create test class" failures. - The compiler-generated module-info-patch.args was ignored, dropping the --add-exports directives declared in module-info-patch.maven. Detect the nested module directory, scan its test classes, patch the module with target/test-classes/<module>, and merge the handoff file into the fork argfile. --add-reads/--add-modules lines from the file are skipped (one-line and two-line argfile forms): they may reference named modules that surefire places on the classpath; surefire keeps generating --add-reads <module>=ALL-UNNAMED, --add-opens for test packages and --add-modules ALL-MODULE-PATH itself. The nested layout is decided by the MAIN build output only (no root module-info.class plus a module subdirectory containing one): a classic modular project whose module is named after its root package (module "it", package "it") has target/test-classes/it/ as a package directory, which must not be mistaken for a nested test output and patched into the module (would break class loading of every test). Covered by unit tests and two ITs: ModulePathWhiteboxIT (classic layout, runs on Maven 3+4) and Surefire3345ModuleSourceHierarchyIT (nested layout, skips itself below Maven 4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address Copilot review feedback on apache#3392: - Reword the inline comment and the appendModuleInfoPatchArgs Javadoc: the method forwards ALL directives from module-info-patch.args except --add-reads/--add-modules, not only --add-exports. - Sort the nested module directory candidates so the picked module no longer depends on filesystem iteration order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The compiler-generated handoff file is UTF-8; FileReader uses the platform default charset and could misparse non-ASCII module or package names. Addresses Copilot review feedback on apache#3394. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address desruisseaux's review comments on apache#3392: - Javadoc for createArgsFile, documenting in particular the patchFile parameter (test output directory, or its per-module subdirectory for a Maven 4 module source hierarchy build). - Correct the findModuleInfoPatchArgs documentation: there is exactly one module-info-patch.args per project, at target/test-classes/META-INF/maven/; the parent lookup exists because the patch directory is the nested per-module directory in a module source hierarchy build. - Drop the explicit charset: Files.newBufferedReader(Path) already reads UTF-8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A Maven 4 module source hierarchy build may declare several Java modules in one POM; the single-module support from apache#3345 detected only the first nested module, scanned only its tests, and decided classpath-vs-module-path dependency placement with one descriptor, so the fork died at the Java Modules boot layer when a sibling module required a dependency that stayed on the classpath. - findModuleDescriptor() resolves ALL nested module descriptors; the primary module (driving scanning and --patch-module) is the first one with a nested test output directory, siblings travel as additional results in ResolvePathResultWrapper. - scanDirectories() unions the scan over every nested target/test-classes/<module>/ directory. - The classpath/module-path split is the union over all module descriptors: an element required on the module path by ANY module goes there, since the fork has a single boot layer. - The primary module only opens its own test packages; each sibling module with tests gets --patch-module, --add-reads and per-package --add-opens, passed through the existing StartupConfiguration#getJpmsArguments() channel - no change to the surefire-booter API or its serialization. Covered by a unit test for the multi-descriptor detection and the IT Surefire3393MultiModuleSourceHierarchyIT: two modules in one POM (com.example.extra requires com.example.core, core requires transitive jakarta.json to cover the boot-layer failure), whitebox tests in both modules, five tests in one execution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Compute the nested module directories once and pass them down instead of the isNestedModuleLayout/findNestedModuleDescriptor double invocation, switch the module-info-patch.args helpers from File to java.nio.file.Path, and create test File objects before the try block so the finally block deletes them directly. Addresses review comments by @desruisseaux on apache#3392. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When maven-compiler-plugin 4.x hands off module-info-patch.args, treat it as the single source of truth for the forked JVM: emit its directives verbatim and drop surefire's auto-generated --add-reads ALL-UNNAMED / --add-modules ALL-MODULE-PATH, which remain as fallback when the file is absent. Precondition: the modules listed by the file's --add-modules (the test-scope dependencies, e.g. the JUnit engine) must be named modules in the fork's boot layer, so their elements move from the class-path to the module path - together with their transitive requires closure, every test or provider classpath element requiring one of the moved modules, and an explicit resolution root per moved module. Surefire's --add-opens for reflective test access now targets ALL-UNNAMED plus the moved named modules instead of ALL-UNNAMED alone. The new ModuleInfoPatchArgsFile parses the handoff file (same-line and two-line option form) and exposes its --add-modules names. Verified: maven-surefire-common 857/857 unit tests green; ITs green on Maven 3.10.0-rc-1 (fallback byte-identical) and 4.0.0-rc-5; acceptance: jigsaw example_test/m4 (1/1) and Vidocq champollion-json (602/602) zero-config under the enforced module system. Fixes apache#3090. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract three private helpers: resolveModulePathSplit (single-module resolution vs multi-module union split), resolveModularPatchDirectory (classic vs nested test output layout) and createHandoffPassThroughArgs (resolution roots + --add-opens for the verbatim module-info-patch.args case). No behavior change; maven-surefire-common tests 857/857 green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New fixture surefire-3090-test-deps-on-module-path: a Maven 4 module source hierarchy project with module-info-patch.maven whose tests assert behaviorally that the JUnit test-scope dependencies run as named modules on the module path (Test.class.getModule() must be org.junit.jupiter.api) and that whitebox access to a non-exported package keeps working. The IT additionally asserts surefire's debug line listing the moved module closure and skips on Maven 3. Verified: green under Maven 4.0.0-rc-5 (3/3 fixture tests, moved closure = api, engine, apiguardian, platform.commons, opentest4j, platform.engine, launcher), skipped under Maven 3.10.0-rc-1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0c81585 to
4ea4f28
Compare
When the dispatch provider is available, resolveModulePathSplit uses the class-path/module-path separation decided by Maven 4 core instead of the plugin-side plexus-java resolution; the project's own build outputs are added around it and the reverse-closure/opens machinery is seeded with the modules already dispatched to the module path. Under Maven 4 this supersedes the hand-rolled forward move of apache#3395. Verified: 3345/3393/3090 ITs green under Maven 4.0.x-SNAPSHOT with the typed delegate dispatching; ModulePathWhiteboxIT green under 3.10.0-rc-1 (fallback unchanged); unit suite 857/857. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This is the follow-up to #3392 announced there and in #3090 — it is stacked on #3392 and stays a draft until #3392 lands (only the last three commits are new).
What this changes
When maven-compiler-plugin 4.x writes the
module-info-patch.argshandoff file, surefire now treats it as the single source of truth for the forked JVM:--add-reads/--add-moduleswere filtered out and replaced by generated fallbacks).--add-reads <module>=ALL-UNNAMEDand--add-modules ALL-MODULE-PATHare gone in this case; they remain as fallback when no handoff file is present (behavior without the file is unchanged).add-modules(the test-scope dependencies, e.g. the JUnit engine) become named modules on the module path. Surefire moves their classpath elements over, together with their transitiverequiresclosure, every classpath element requiring one of the moved modules (they must not be split across the named/unnamed worlds), and an explicit resolution root per moved module.--add-opensfor reflective test access now targetsALL-UNNAMEDplus the moved named modules, and is generated on the plugin side, where the moved module closure is known.This addresses the review discussion on #3392 (@desruisseaux):
module-info-patch.mavenis used exactly as-is, tests run with the same module options as the test compilation.Verification
maven-surefire-common857/857 green.Surefire3090TestDepsOnModulePathIT: asserts behaviorally thatTest.class.getModule()is the named moduleorg.junit.jupiter.api(fails when the engine is left on the classpath), that whitebox access to a non-exported package keeps working, and that the moved module closure is logged. Green under Maven 4.0.0-rc-5, skipped under Maven 3.example_test/m4project (1/1) and the Vidocqchampollion-jsonpilot (3 modules in one POM, 602/602 tests) run zero-config under the enforced module system.Fixes #3090.
🤖 Generated with Claude Code