gnodet-bot commented on code in PR #1127:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1127#discussion_r4084550309
##########
src/test/java/org/apache/maven/plugin/compiler/CompilerMojoTestCase.java:
##########
@@ -18,6 +18,8 @@
*/
package org.apache.maven.plugin.compiler;
Review Comment:
⚠️ **Import ordering:** `javax.tools.ToolProvider` is placed before the
`java.*` block, which violates the project's import ordering convention
(`java.*` → `javax.*` → third-party). Move it after the `java.util.Map` import.
```suggestion
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.tools.ToolProvider;
```
##########
src/test/java/org/apache/maven/plugin/compiler/CompilerMojoTestCase.java:
##########
@@ -420,6 +422,61 @@ public void testCompileSkipTest(
assertOutputFileDoesNotExist(compileMojo, "foo",
"TestSkipTestCompile0Test.class");
}
+ @Test
+ @Basedir("${basedir}/target/test-classes/unit/compiler-existing-output")
+ public void testMainOutput(@InjectMojo(goal = "compile", pom =
"plugin-config.xml") CompilerMojo mojo)
+ throws Exception {
+ compileWithExistingOutput(mojo, false);
+ }
+
+ @Test
+ @Basedir("${basedir}/target/test-classes/unit/compiler-existing-output")
+ public void testMainOutputForked(@InjectMojo(goal = "compile", pom =
"plugin-config.xml") CompilerMojo mojo)
+ throws Exception {
+ compileWithExistingOutput(mojo, true);
+ }
+
+ @Test
+ @Basedir("${basedir}/target/test-classes/unit/compiler-existing-output")
+ public void testTestOutput(
+ @InjectMojo(goal = "testCompile", pom = "plugin-config.xml")
+ @MojoParameter(name = "compileSourceRoots", value =
"${project.basedir}/src/test/java")
+ TestCompilerMojo mojo)
+ throws Exception {
+ compileWithExistingOutput(mojo, false);
+ }
+
+ @Test
+ @Basedir("${basedir}/target/test-classes/unit/compiler-existing-output")
+ public void testTestOutputForked(
+ @InjectMojo(goal = "testCompile", pom = "plugin-config.xml")
+ @MojoParameter(name = "compileSourceRoots", value =
"${project.basedir}/src/test/java")
+ TestCompilerMojo mojo)
+ throws Exception {
+ compileWithExistingOutput(mojo, true);
+ }
+
+ private static void compileWithExistingOutput(AbstractCompilerMojo mojo,
boolean fork) throws Exception {
+ mojo.fork = fork;
+ if (fork) {
+ mojo.executable =
+ Path.of(System.getProperty("java.home"), "bin",
"javac").toString();
+ }
+ Path output = Files.createDirectories(mojo.getOutputDirectory());
+ String helperName = mojo instanceof TestCompilerMojo ? "TestHelper" :
"MainHelper";
+ Path helper = mojo.basedir.resolve(helperName + ".java");
+ Files.writeString(
+ helper, "public class " + helperName + " { public static
String value() { return \"existing\"; } }");
+ assertEquals(
+ 0,
+ ToolProvider.getSystemJavaCompiler().run(null, null, null,
"-d", output.toString(), helper.toString()));
+ Files.delete(helper);
+ Files.deleteIfExists(output.resolve("Consumer.class"));
+ mojo.execute();
+ assertTrue(Files.isRegularFile(output.resolve(helperName + ".class")));
+ assertTrue(Files.isRegularFile(output.resolve("Consumer.class")));
+ }
+
@Provides
@Singleton
@SuppressWarnings("unused")
Review Comment:
⚠️ **Missing regression test for the stale-class problem:** The test
currently only covers the happy path — a class pre-compiled before the mojo
runs is visible via the classpath during the mojo's own compilation. This
proves `outputDirectory` is on the classpath, but it does **not** cover the
correctness regression the PR description warns about.
The regression case is: `MainHelper.java` is compiled in one build, then
`MainHelper.java` is **deleted**, then the mojo runs a full clean build —
`MainHelper.class` should be absent from the output (or the build should fail
because `Consumer.java` can no longer resolve it). Without this test, the known
stale-class regression ships untested.
Suggested additional test (schematic):
```java
private static void compileWithDeletedSource(AbstractCompilerMojo mojo)
throws Exception {
Path output = Files.createDirectories(mojo.getOutputDirectory());
// Step 1: pre-populate a stale class file (simulating a previous build
artifact)
Path helper = mojo.basedir.resolve("StaleHelper.java");
Files.writeString(helper, "public class StaleHelper {}");
ToolProvider.getSystemJavaCompiler().run(null, null, null, "-d",
output.toString(), helper.toString());
Files.delete(helper); // source is gone — class file is stale
// Step 2: run the mojo — Consumer.java does NOT reference StaleHelper
Files.deleteIfExists(output.resolve("Consumer.class"));
mojo.execute();
// Stale class MUST be removed or the build must fail
assertFalse("Stale StaleHelper.class should not survive a full rebuild",
Files.isRegularFile(output.resolve("StaleHelper.class")));
}
```
This is exactly the scenario described in the PR body ("after removing a
source or a secondary class declaration, a full rebuild can incorrectly succeed
by resolving the old class file from the output directory"). If the regression
is a known blocker that prevents merging, capturing it as a **failing test**
would make the blocker explicit and trackable.
--
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]
