gnodet commented on code in PR #13059:
URL: https://github.com/apache/maven/pull/13059#discussion_r3945566725
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java:
##########
@@ -423,6 +423,47 @@ int getRunningJdkMajor() {
assertEquals(1, result.modifiedPoms().size());
assertTrue(strategy.hasToolchainsPluginWithSelectGoal(doc));
+
+ // Verify that a warning about toolchain JDK availability was
emitted
+ String xml = doc.toXml();
+ assertTrue(xml.contains("select-jdk-toolchain"), "POM should
contain select-jdk-toolchain goal");
+ }
+
+ @Test
+ @DisplayName("should emit warning about JDK availability when adding
toolchains plugin")
+ void shouldEmitJdkAvailabilityWarning() {
+ // Simulate running JDK 21, project targets source 6
+ ToolchainPluginStrategy strategy = new ToolchainPluginStrategy() {
+ @Override
+ int getRunningJdkMajor() {
+ return 21;
+ }
+ };
+
+ String pomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>com.example</groupId>
+ <artifactId>test</artifactId>
+ <version>1.0</version>
+ <properties>
+ <maven.compiler.release>6</maven.compiler.release>
+ </properties>
+ </project>
+ """;
+ Document doc = Document.of(pomXml);
+ UpgradeContext context = TestUtils.createMockContext();
+
+ UpgradeResult result = strategy.doApply(context, Map.of(POM_PATH,
doc));
+
+ assertEquals(1, result.modifiedPoms().size());
+ assertTrue(strategy.hasToolchainsPluginWithSelectGoal(doc));
+
+ // The output should contain the toolchains plugin and version
constraint
+ String xml = doc.toXml();
+ assertTrue(xml.contains("select-jdk-toolchain"), "POM should
contain select-jdk-toolchain goal");
+ // The warning is emitted through the context logger — verified by
integration tests
}
@Test
Review Comment:
💡 **Observation:** This test is named `shouldEmitJdkAvailabilityWarning` but
doesn't actually verify the warning was emitted — it only checks POM
modification (same assertions as the test above). The mock context doesn't
capture `warning()` calls.
The comment on line 469 acknowledges this (`"verified by integration
tests"`), so this is more of a naming/clarity nit — but if you wanted to verify
it here, you could use `Mockito.verify(logger).warn(contains("must be
installed"))` on the mock logger.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -1568,6 +1568,160 @@ void shouldNotModifyPomWithoutMigratablePlugins()
throws Exception {
assertEquals("4.9.5", version, "version should remain 4.9.5");
}
+ @Test
+ @DisplayName("should upgrade exec-maven-plugin in submodule with
explicit version")
+ void shouldUpgradeExecPluginInSubmodule() throws Exception {
+ // Simulates hbase-assembly declaring exec-maven-plugin:3.1.0
explicitly
+ String parentPomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>org.example</groupId>
+ <artifactId>parent</artifactId>
+ <version>1.0.0</version>
+ <packaging>pom</packaging>
+ <modules>
+ <module>assembly</module>
+ </modules>
+ </project>
+ """;
+
+ String submodulePomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.example</groupId>
+ <artifactId>parent</artifactId>
+ <version>1.0.0</version>
+ </parent>
+ <artifactId>assembly</artifactId>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>exec-maven-plugin</artifactId>
+ <version>3.1.0</version>
+ </plugin>
+ </plugins>
+ </build>
+ </project>
+ """;
+
+ Path tempDir = Files.createTempDirectory("mvnup-test-");
+ try {
+ Files.createDirectories(tempDir.resolve(".mvn"));
+ Path parentPomPath = tempDir.resolve("pom.xml");
+ Files.writeString(parentPomPath, parentPomXml);
+ Path assemblyDir = tempDir.resolve("assembly");
+ Files.createDirectories(assemblyDir);
+ Path submodulePomPath = assemblyDir.resolve("pom.xml");
+ Files.writeString(submodulePomPath, submodulePomXml);
+
+ Document parentDoc = Document.of(parentPomXml);
+ Document submoduleDoc = Document.of(submodulePomXml);
+ Map<Path, Document> pomMap = Map.of(
+ parentPomPath, parentDoc,
+ submodulePomPath, submoduleDoc);
+
+ UpgradeContext context = createMockContext();
+ UpgradeResult result = strategy.doApply(context, pomMap);
+
+ assertTrue(result.success(), "Plugin upgrade should succeed");
+
+ // The submodule's exec-maven-plugin should be upgraded to
3.5.0
+ Editor editor = new Editor(submoduleDoc);
+ String version = editor.root()
+ .path("build", "plugins", "plugin", "version")
+ .map(Element::textContentTrimmed)
+ .orElse(null);
+ assertEquals("3.5.0", version, "exec-maven-plugin 3.1.0 should
be upgraded to 3.5.0 in submodule");
+ assertFalse(submoduleDoc.toXml().contains("3.1.0"), "Old
version 3.1.0 should not remain");
+ } finally {
+ // Cleanup
+ Files.walk(tempDir)
+ .sorted(java.util.Comparator.reverseOrder())
+ .map(Path::toFile)
+ .forEach(java.io.File::delete);
+ }
Review Comment:
⚠️ **Resource leak:** `Files.walk()` returns a `Stream` that wraps a
`DirectoryStream` — it must be closed. Without try-with-resources, the file
handle leaks if an exception is thrown during traversal.
The existing tests (line ~956) use the correct pattern. This should be
consistent:
```suggestion
try (var walk = Files.walk(tempDir)) {
walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> {
try {
Files.delete(p);
} catch (IOException ignored) {
}
});
}
```
Also note the existing pattern uses `Files.delete(p)` directly instead of
`Path::toFile` + `File::delete`, which gives better error diagnostics.
--
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]
