Copilot commented on code in PR #3523:
URL: https://github.com/apache/maven-surefire/pull/3523#discussion_r4323888770
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -2162,6 +2173,23 @@ private StartupConfiguration
newStartupConfigWithModularPath(
providerName, classpathConfiguration,
classLoaderConfiguration, processCheckerType, jpmsArgs);
}
+ private Set<String> getModuleNamesOnModulepath(ResolvePathsResult<String>
resolvedPaths) {
+ if (resolvedPaths == null
+ || resolvedPaths.getModulepathElements() == null
+ || resolvedPaths.getPathElements() == null) {
+ return Collections.emptySet();
+ }
+
+ Set<String> moduleNames = new HashSet<>();
+ for (String path : resolvedPaths.getModulepathElements().keySet()) {
+ JavaModuleDescriptor descriptor =
resolvedPaths.getPathElements().get(path);
+ if (descriptor != null && descriptor.name() != null) {
+ moduleNames.add(descriptor.name());
+ }
+ }
+ return moduleNames;
+ }
Review Comment:
This method duplicates the logic added in `ModulePathJpmsArguments`
(`moduleNamesOnModulepath(...)`). To avoid divergence over time, consider
consolidating into a single shared helper (e.g., make the
`ModulePathJpmsArguments` helper package-private and reuse it here, or move the
logic into a small internal utility class).
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -3236,7 +3264,14 @@ public void addProviderProperties() throws
MojoExecutionException {
@Nonnull
@Override
public List<String[]> getJpmsArguments(@Nonnull ProviderRequirements
forkRequirements) {
- return
ModulePathJpmsArguments.junitPlatformArguments(forkRequirements);
+ // Provider dependencies stay on the classpath, so only open
Commons when it is a test module.
+ boolean junitPlatformCommonsOnModulePath = forkRequirements
+ .getTestModuleNames()
+ .contains("org.junit.platform.commons")
+ ||
testClasspath.getTestDependencies().containsKey(PROVIDER_DEP_GID +
":junit-platform-commons");
+ return junitPlatformCommonsOnModulePath
+ ?
ModulePathJpmsArguments.junitPlatformArguments(forkRequirements)
+ : Collections.emptyList();
Review Comment:
This condition mixes two sources of truth (resolved module names vs.
dependency presence). Since the goal is specifically “only when Commons is on
the *effective test module path*”, consider relying solely on
`forkRequirements.getTestModuleNames()` (and ensure it is populated in all
relevant layouts). This reduces chances of reintroducing the warning if the
dependency is present but still not placed on the module path in a given
resolution scenario.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ModulePathJpmsArguments.java:
##########
@@ -60,11 +62,13 @@ static final class Layout {
private final Classpath classpath;
private final Classpath modulepath;
private final List<String[]> jpmsArguments;
+ private final Set<String> moduleNames;
- Layout(Classpath classpath, Classpath modulepath, List<String[]>
jpmsArguments) {
+ Layout(Classpath classpath, Classpath modulepath, List<String[]>
jpmsArguments, Set<String> moduleNames) {
this.classpath = classpath;
this.modulepath = modulepath;
this.jpmsArguments = jpmsArguments;
+ this.moduleNames = moduleNames;
}
Review Comment:
`Layout` stores `moduleNames` directly and exposes it via
`getModuleNames()`, which allows callers to mutate internal state
inadvertently. Consider defensively copying and/or wrapping with
`Collections.unmodifiableSet(...)` in the constructor to keep `Layout`
immutable.
##########
maven-surefire-common/src/main/java/org/apache/maven/surefire/providerapi/ProviderRequirements.java:
##########
@@ -27,11 +31,21 @@ public final class ProviderRequirements {
private final boolean modularPath;
private final boolean mainModuleDescriptor;
private final boolean testModuleDescriptor;
+ private final Set<String> testModuleNames;
public ProviderRequirements(boolean modularPath, boolean
mainModuleDescriptor, boolean testModuleDescriptor) {
+ this(modularPath, mainModuleDescriptor, testModuleDescriptor,
Collections.emptySet());
+ }
+
+ public ProviderRequirements(
+ boolean modularPath,
+ boolean mainModuleDescriptor,
+ boolean testModuleDescriptor,
+ Set<String> testModuleNames) {
this.modularPath = modularPath;
this.mainModuleDescriptor = mainModuleDescriptor;
this.testModuleDescriptor = testModuleDescriptor;
+ this.testModuleNames = Collections.unmodifiableSet(new
HashSet<>(testModuleNames));
}
Review Comment:
The new public constructor will throw a `NullPointerException` if
`testModuleNames` is `null`, but the API doesn’t document nullability. Consider
either (a) explicitly documenting/annotating `testModuleNames` as non-null, or
(b) treating null as `Collections.emptySet()` to make the API more robust for
external callers.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
