[MCOMPILER-1124] testCompile dependency state tolerates missing target/classes by arimu1 · Pull Request #1125 · apache/maven-compiler-plugin

arimu1 · 2026-09-12T23:04:25Z

Summary

Fix spurious [WARNING] I/O error reading dependency state: …/target/classes during testCompile on projects with no src/main/java (tests-only modules).
Missing classpath/module-path entries are treated like empty directories when persisting dependency state, so incremental testCompile stays stable without false "changed dependency" rebuilds.
Adds DependencyStateTest coverage and invoker IT MCOMPILER-1124_test-only-testcompile.

Root cause

Since 3.16.0 (#1102), dependency state scanning walks test classpath elements. Tests-only projects still list ${project.build.outputDirectory} on the classpath even when that directory was never created. A missing path was handled as a regular file read failure, which logged a warning and stored an unreadable fingerprint.

Test plan

mvn -Dtest=DependencyStateTest test
mvn -Prun-its verify -Dinvoker.test=MCOMPILER-1124_test-only-testcompile -DskipTests
Manual reproducer (lprimak/maven-test-compiler-reproducer) with compiler.plugin.version=3.16.1-SNAPSHOT: no dependency-state warning on clean test-compile / test-compile
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

When testCompile scans dependency state, the test classpath may list target/classes even if no main sources were compiled. Treat a missing path like an empty directory instead of logging I/O warnings and recording unstable dependency metadata. Fixes apache#1124

gnodet-bot

The fix is correct and the approach is sound — treating a missing classpath entry as an empty-directory fingerprint is the right abstraction, and the IT covers the primary regression. Two gaps to address:

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-23T16:05:20Z

+        assertFalse(hasChanged(
+                Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),
+                Collections.emptyList(),
+                BUILD_START,
+                0));
+    }


⚠️ Missing coverage: the missing→present transition is not tested.

The second assertFalse block is identical to the first — it only verifies idempotence (calling twice with the same missing dir gives the same result both times). That's already implicitly covered by the first call establishing a baseline and the second reading it back.

What's not tested is the scenario that matters most: if target/classes is subsequently created and a class file compiled into it, does hasChanged() correctly return true? Without that assertion, the implementation could be regressed (e.g. by making emptyDirectoryState() a static constant or by not writing the state file for missing paths) and this test would still pass.

Replace the duplicate assertFalse with a transition assertion:

Suggested change

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

// Once target/classes is created and populated, the change must be detected.

Files.createDirectories(missingMainOutput);

Path classFile = missingMainOutput.resolve("org/example/Foo.class");

Files.createDirectories(classFile.getParent());

Files.write(classFile, new byte[]{1});

assertTrue(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

Fixed in 9ecf77f.

gnodet-bot · 2026-09-23T16:05:20Z

@@ -138,6 +138,9 @@ private static Path addDependencies(
     * The directory digest covers each relevant file's relative path, size and modification time.


🔍 Javadoc doesn't cover the new missing-path case.

The doc currently says: "Returns size:mtime for a file, or relevant-file-count:metadata-sha256 for a directory." — it no longer describes the behaviour for a non-existent path, which now returns emptyDirectoryState(). A reader skimming this method will be confused about why the guard exists and what the returned value means.

Suggested change

* The directory digest covers each relevant file's relative path, size and modification time.

* Returns {@code size:mtime} for a file, {@code relevant-file-count:metadata-sha256} for a directory,

* or an empty-directory fingerprint for a path that does not exist (e.g. {@code target/classes} in a

* tests-only module that was never compiled).

* The directory digest covers each relevant file's relative path, size and modification time.

Fixed in 9ecf77f.

gnodet · 2026-09-28T13:46:28Z

The CI failure (on the jdk-17-zulu 3.10.0-rc-1 matrix job) was caused by maven.compiler.release=21 in the IT's pom.xml: JDK 17 cannot compile with --release 21, so the first invocation (clean test-compile) failed, leaving nothing compiled, and the verify.groovy assertion log.contains('Nothing to compile - all classes are up to date') then failed.

Fixed in 4602db2 by using release=8 instead, which works across all JDKs in the CI matrix (8, 17, 21, 25). The release version is irrelevant to the fix being tested (incremental compilation tolerance for a missing target/classes).

gnodet-bot

Re-review after 4602db2 (CI fix: release=21 → release=8). The CI fix is correct — JDK 17 can't compile with --release 21, and the release version is irrelevant to the bug being tested.

However, the two findings from the previous review are still outstanding. Re-raising them below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-28T13:48:06Z

@@ -138,6 +138,9 @@ private static Path addDependencies(
     * The directory digest covers each relevant file's relative path, size and modification time.


🔍 Still outstanding from previous review: Javadoc doesn't cover the new missing-path case.

The doc says: "Returns size:mtime for a file, or relevant-file-count:metadata-sha256 for a directory." — it no longer describes the full behavior since a non-existent path now returns emptyDirectoryState().

Suggested change

* The directory digest covers each relevant file's relative path, size and modification time.

* Returns {@code size:mtime} for a file, {@code relevant-file-count:metadata-sha256} for a directory,

* or an empty-directory fingerprint for a path that does not exist (e.g. {@code target/classes} in a

* tests-only module that was never compiled).

* The directory digest covers each relevant file's relative path, size and modification time.

Already fixed in 9ecf77f — the current Javadoc matches this suggestion exactly.

gnodet-bot · 2026-09-28T13:48:06Z

+                0));
+        assertFalse(hasChanged(
+                Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),
+                Collections.emptyList(),
+                BUILD_START,
+                0));
+    }


⚠️ Still outstanding from previous review: The second assertFalse block is identical to the first — it only verifies idempotence. The critical missing→present transition is not tested: if target/classes is subsequently created and populated, does hasChanged() correctly return true?

Without this, the implementation could regress (e.g. emptyDirectoryState() returns a static constant that accidentally matches a populated directory) and this test would still pass.

Suggested change

0));

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

// Once target/classes is created and populated, the change must be detected.

Files.createDirectories(missingMainOutput);

Path classFile = missingMainOutput.resolve("org/example/Foo.class");

Files.createDirectories(classFile.getParent());

Files.write(classFile, new byte[]{1});

assertTrue(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

Already fixed in 9ecf77f — the test now includes the missing→present transition assertion.

gnodet-bot

Re-review after 4602db2 (fix IT release=8 for JDK 17 CI). The CI fix itself is correct — release=8 works across all matrix JDKs and is irrelevant to the tested behavior.

However, both findings from the previous review remain unaddressed:

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-28T13:48:07Z

+                BUILD_START,
+                0));
+        assertFalse(hasChanged(
+                Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),
+                Collections.emptyList(),
+                BUILD_START,
+                0));
+    }


⚠️ Still unaddressed from previous review: The second assertFalse block is a duplicate of the first — it only tests idempotence (same missing dir → same result twice). The critical missing→present transition is not covered.

If target/classes is subsequently created and populated, does hasChanged() correctly detect it? Without this assertion, a regression (e.g. making emptyDirectoryState() a static constant cached per-JVM) could slip through.

Suggested change

BUILD_START,

0));

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

assertFalse(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

// Once target/classes is created and populated, the change must be detected.

Files.createDirectories(missingMainOutput);

Path classFile = missingMainOutput.resolve("org/example/Foo.class");

Files.createDirectories(classFile.getParent());

Files.write(classFile, new byte[]{1});

assertTrue(hasChanged(

Arrays.asList(missingMainOutput, dependency("dependency.jar", 1_000, 1)),

Collections.emptyList(),

BUILD_START,

0));

}

Already fixed in 9ecf77f — the current test code matches this suggestion.

gnodet-bot · 2026-09-28T13:48:07Z

@@ -138,6 +138,9 @@ private static Path addDependencies(
     * The directory digest covers each relevant file's relative path, size and modification time.


🔍 Still unaddressed from previous review: The Javadoc says "Returns size:mtime for a file, or relevant-file-count:metadata-sha256 for a directory" but no longer describes the full behavior — a non-existent path now returns emptyDirectoryState(). A reader won't understand why the Files.notExists guard exists.

Suggested change

* The directory digest covers each relevant file's relative path, size and modification time.

* Returns {@code size:mtime} for a file, {@code relevant-file-count:metadata-sha256} for a directory,

* or an empty-directory fingerprint for a path that does not exist (e.g. {@code target/classes} in a

* tests-only module that was never compiled).

* The directory digest covers each relevant file's relative path, size and modification time.

Already fixed in 9ecf77f — the Javadoc already includes the missing-path documentation.

gnodet-bot

Re-review after 9ecf77f — both previous findings are now addressed:

Javadoc ✅ — modificationState() doc now covers the non-existent path case.
Test coverage ✅ — The duplicate idempotence assertion is replaced with a missing→present transition test that verifies hasChanged() correctly detects when target/classes appears and gets populated.

The CI fix (release 21→8 in IT pom) is correct — JDK 17 matrix jobs can't compile with --release 21.

Milestone note: PR targets maven-compiler-plugin-3.x but milestone is set to 4.0.0-beta-6 (which tracks master). Should be 3.16.1.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after 9ecf77f — both previous findings addressed:

✅ Test coverage: the duplicate assertFalse replaced with a missing→present transition test (Files.createDirectories + Files.write + assertTrue(hasChanged(...))) — this correctly validates that hasChanged() detects when target/classes is later created and populated.
✅ Javadoc: modificationState now documents the missing-path case and the emptyDirectoryState() return.
✅ CI fix (4602db2): maven.compiler.release changed from 21 to 8 in the IT, so it runs on all matrix JDKs.

The fix is solid. Missing classpath entries are treated as empty-directory fingerprints, incremental testCompile stays stable, IT covers the regression, and the unit test now covers the critical transition.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot

Re-review after ae2416b — Spotless formatting fix (new byte[]{1} → new byte[] {1}). No functional change.

All previous findings remain addressed. The fix is solid — ready to merge.

Milestone note (still outstanding): PR targets maven-compiler-plugin-3.x but milestone is 4.0.0-beta-6 (tracks master). Should be 3.16.1.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot reviewed Sep 23, 2026

View reviewed changes

gnodet added this to the 4.0.0-beta-6 milestone Sep 28, 2026

Fix IT to use release=8 to allow running on JDK 17

4602db2

Address review: test missing→present transition, update javadoc

9ecf77f

gnodet-bot reviewed Sep 28, 2026

View reviewed changes

gnodet-bot approved these changes Sep 28, 2026

View reviewed changes

fix: spotless formatting (byte[] {1})

ae2416b

gnodet-bot approved these changes Sep 28, 2026

View reviewed changes

gnodet modified the milestones: 4.0.0-beta-6, 3.16.1 Sep 28, 2026

		@@ -138,6 +138,9 @@ private static Path addDependencies(
		* The directory digest covers each relevant file's relative path, size and modification time.

-     * The directory digest covers each relevant file's relative path, size and modification time.
+     * Returns {@code size:mtime} for a file, {@code relevant-file-count:metadata-sha256} for a directory,
+     * or an empty-directory fingerprint for a path that does not exist (e.g. {@code target/classes} in a
+     * tests-only module that was never compiled).
+     * The directory digest covers each relevant file's relative path, size and modification time.

Conversation

arimu1 commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Test plan

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet commented Sep 28, 2026

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arimu1 commented Sep 12, 2026 •

edited

Loading