Copilot commented on code in PR #3523:
URL: https://github.com/apache/maven-surefire/pull/3523#discussion_r4346513007
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ModulePathJpmsArguments.java:
##########
@@ -174,6 +183,21 @@ static List<String[]> of(ResolvePathsResult<String>
result, String mainModule, C
return args;
}
+ static Set<String> moduleNamesOnModulepath(ResolvePathsResult<String>
result) {
+ if (result == null || result.getModulepathElements() == null ||
result.getPathElements() == null) {
+ return java.util.Collections.emptySet();
+ }
+
+ Set<String> names = new HashSet<>();
+ for (String path : result.getModulepathElements().keySet()) {
+ JavaModuleDescriptor descriptor =
result.getPathElements().get(path);
+ if (descriptor != null && descriptor.name() != null) {
+ names.add(descriptor.name());
+ }
+ }
+ return names;
+ }
Review Comment:
This helper returns a mutable `HashSet`, while other call sites immediately
wrap/copy it to enforce immutability. Returning an unmodifiable set directly
(and using the existing `emptySet()` import rather than the fully-qualified
`java.util.Collections.emptySet()`) would make the contract clearer and reduce
the chance of accidental mutation by future callers.
##########
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/ModulePathJpmsArgumentsTest.java:
##########
@@ -146,6 +147,19 @@ public void shouldPutEverythingOnModulePathForTestModule()
{
assertThat(layout.getJpmsArguments()).isEmpty();
}
+ @Test
+ public void shouldKeepModuleNamesImmutable() {
+ HashSet<String> moduleNames = new
HashSet<>(singleton("org.junit.platform.commons"));
+ ModulePathJpmsArguments.Layout layout = new
ModulePathJpmsArguments.Layout(
+ Classpath.emptyClasspath(), Classpath.emptyClasspath(),
emptyList(), moduleNames);
+
+ moduleNames.add("org.example.changed");
+
+
assertThat(layout.getModuleNames()).containsExactly("org.junit.platform.commons");
Review Comment:
`layout.getModuleNames()` returns a `Set`, and `containsExactly(...)` is
order-sensitive. Even though it’s a single element today, this becomes brittle
if the test is extended. Prefer an order-insensitive assertion (e.g.,
`containsOnly(...)`) to avoid coupling the test to `Set` iteration order.
##########
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:
##########
@@ -2101,21 +2102,31 @@ private StartupConfiguration
newStartupConfigWithModularPath(
providerClasspath =
classpathCache.setCachedClasspath(providerName, providerArtifacts);
}
- // the Maven 3 guesses about what goes on the module path, see
ModulePathJpmsArguments
+ final Set<String> testModuleNames;
final ModulePathJpmsArguments.Layout layout;
if (isMainDescriptor) {
+ // the Maven 3 guesses about what goes on the module path, see
ModulePathJpmsArguments
for (String className : scanResult.getClasses()) {
packages.add(substringBeforeLast(className, "."));
}
layout = ModulePathJpmsArguments.forMainModule(
getLocationManager(), testClasspath, javaModuleDescriptor,
javaHome, packages, getConsoleLogger());
+ testModuleNames = layout.getModuleNames();
} else {
+ ResolvePathsRequest<String> req =
ResolvePathsRequest.ofStrings(testClasspath.getClassPath())
+ .setIncludeAllProviders(true)
+ .setJdkHome(javaHome)
+ .setIncludeStatic(true)
+ .setModuleDescriptor(javaModuleDescriptor);
+ testModuleNames = ModulePathJpmsArguments.moduleNamesOnModulepath(
+ getLocationManager().resolvePaths(req));
layout = ModulePathJpmsArguments.forTestModule(testClasspath);
Review Comment:
This introduces an additional `locationManager.resolvePaths(...)` call on
the test-descriptor path. Since this is potentially expensive and will run for
every modular test execution (even when the selected provider may not need
JUnit Platform opens), consider deferring the resolution until it’s actually
needed (e.g., only when constructing/using the JUnit Platform provider info) or
reusing an already-available resolution result if this information is computed
elsewhere in the same flow.
--
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]
