gnodet-bot commented on code in PR #1202:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1202#discussion_r4475558909
##########
src/test/java/org/apache/maven/plugin/compiler/TestCompilerMojoTest.java:
##########
@@ -172,6 +178,57 @@ void testCompileSkipTest(TestCompilerMojo
testCompilerMojo) throws Exception {
assertFalse(testClass.exists());
}
+ /**
+ * Tests that test sources of a module are compiled as a patch of the main
module: the mojo records
+ * {@code --patch-module} and {@code --add-reads} in {@code
META-INF/jpms.args}.
+ */
+ @Test
+ @EnabledForJreRange(min = JRE.JAVA_9)
+ @InjectMojo(goal = TEST_COMPILE, pom =
"classpath:/unit/compiler-jpms-patch-module/plugin-config.xml")
+ void testJpmsPatchModule(TestCompilerMojo testCompilerMojo, @TempDir Path
mainClasses) throws Exception {
+ setUpCompilerMojoTestEnv(testCompilerMojo);
+
+ // the test mojo reads the main module descriptor from the compiled
main classes
+ Path fixture =
Paths.get(testCompilerMojo.getCompileSourceRoots().get(0))
+ .getParent()
+ .getParent()
+ .getParent();
+ Path mainSources = fixture.resolve("src/main/java");
+ JavaCompiler javac = ToolProvider.getSystemJavaCompiler();
Review Comment:
🛑 **NPE risk:** `ToolProvider.getSystemJavaCompiler()` returns `null` on
JRE-only environments (no JDK tools). `@EnabledForJreRange(min = JRE.JAVA_9)`
only checks the runtime version — it does **not** guarantee the compiler tool
is available. The very next line calls `javac.run(...)` which will throw
`NullPointerException` on any Java 9+ JRE without a JDK.
Add an assumption guard after getting the compiler:
```suggestion
JavaCompiler javac = ToolProvider.getSystemJavaCompiler();
assumeTrue(javac != null, "No system Java compiler available —
skipping on JRE-only runtime");
assertEquals(
```
(Import `org.junit.jupiter.api.Assumptions.assumeTrue`.)
##########
src/test/java/org/apache/maven/plugin/compiler/CompilerMojoTest.java:
##########
@@ -337,6 +341,29 @@ void testCompileSkipTest(CompilerMojo compilerMojo) throws
Exception {
assertTrue(testClass.exists());
}
+ /**
+ * Tests that runtime options such as {@code --add-exports} are recorded
in {@code META-INF/jpms.args} when the
+ * project is a module.
+ *
+ * @throws Exception
+ */
+ @Test
+ @EnabledForJreRange(min = JRE.JAVA_9)
+ @InjectMojo(goal = COMPILE, pom =
"classpath:/unit/compiler-jpms-add-exports/plugin-config.xml")
+ void testJpmsAddExports(CompilerMojo compilerMojo) throws Exception {
+ setUpCompilerMojoTestEnv(compilerMojo);
+
+ // The mojo only writes jpms.args when it compiles, and only logs a
warning when it cannot write it, while
+ // target/ keeps the classes of earlier runs (the next run would find
nothing to compile). Start from an empty
+ // output directory so the assertion reads this run's file.
+ FileUtils.deleteDirectory(compilerMojo.getOutputDirectory());
Review Comment:
⚠️ **Inconsistency:** `FileUtils.deleteDirectory` (plexus-utils, deprecated)
is used here, but the companion test `testJpmsPatchModule` in the same PR uses
`Files.deleteIfExists` (JDK). Use JDK APIs consistently — `Files.walk` +
`Files.delete` covers recursive deletion without the plexus dependency:
```suggestion
deleteRecursively(compilerMojo.getOutputDirectory().toPath());
```
And add a private helper (or use the test framework's existing cleanup
utilities if available):
```java
private static void deleteRecursively(Path path) throws IOException {
if (Files.exists(path)) {
Files.walk(path)
.sorted(Comparator.reverseOrder())
.forEach(p -> { try { Files.delete(p); } catch (IOException e)
{ throw new UncheckedIOException(e); } });
}
}
```
##########
src/test/java/org/apache/maven/plugin/compiler/CompilerMojoTest.java:
##########
@@ -337,6 +341,29 @@ void testCompileSkipTest(CompilerMojo compilerMojo) throws
Exception {
assertTrue(testClass.exists());
}
+ /**
+ * Tests that runtime options such as {@code --add-exports} are recorded
in {@code META-INF/jpms.args} when the
+ * project is a module.
+ *
+ * @throws Exception
+ */
+ @Test
+ @EnabledForJreRange(min = JRE.JAVA_9)
+ @InjectMojo(goal = COMPILE, pom =
"classpath:/unit/compiler-jpms-add-exports/plugin-config.xml")
+ void testJpmsAddExports(CompilerMojo compilerMojo) throws Exception {
+ setUpCompilerMojoTestEnv(compilerMojo);
+
+ // The mojo only writes jpms.args when it compiles, and only logs a
warning when it cannot write it, while
+ // target/ keeps the classes of earlier runs (the next run would find
nothing to compile). Start from an empty
+ // output directory so the assertion reads this run's file.
+ FileUtils.deleteDirectory(compilerMojo.getOutputDirectory());
+ Path jpmsArgs =
compilerMojo.getOutputDirectory().toPath().resolve("META-INF/jpms.args");
+
+ compilerMojo.execute();
+
+ assertEquals(Arrays.asList("--add-exports",
"app/org.maven.test=ALL-UNNAMED"), Files.readAllLines(jpmsArgs));
Review Comment:
⚠️ **Missing compilation guard:** The comment above already acknowledges
that `execute()` only logs a warning on failure — meaning `jpms.args` may never
be written. If that happens, `Files.readAllLines(jpmsArgs)` throws
`NoSuchFileException`, which surfaces as an obscure test failure (file not
found) rather than a meaningful assertion message. Add an explicit check:
```suggestion
compilerMojo.execute();
assertTrue(Files.exists(jpmsArgs), "jpms.args was not written — did
compilation succeed?");
assertEquals(Arrays.asList("--add-exports",
"app/org.maven.test=ALL-UNNAMED"), Files.readAllLines(jpmsArgs));
```
--
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]
