gnodet-bot commented on code in PR #1125:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1125#discussion_r4122897262
##########
src/main/java/org/apache/maven/plugin/compiler/DependencyState.java:
##########
@@ -138,6 +138,9 @@ private static Path addDependencies(
* The directory digest covers each relevant file's relative path, size
and modification time.
Review Comment:
🔍 **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()`.
```suggestion
* 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.
```
##########
src/test/java/org/apache/maven/plugin/compiler/DependencyStateTest.java:
##########
@@ -114,6 +114,22 @@ void preservesRepeatedDependencyEntriesAndPathKinds()
throws Exception {
assertTrue(state.get(2).startsWith("modulepath:"));
}
+ @Test
+ void toleratesMissingClasspathDirectory() throws Exception {
+ Path missingMainOutput = temporaryDirectory.resolve("target/classes");
+
+ 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));
+ }
Review Comment:
⚠️ **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.
```suggestion
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));
}
```
##########
src/test/java/org/apache/maven/plugin/compiler/DependencyStateTest.java:
##########
@@ -114,6 +114,22 @@ void preservesRepeatedDependencyEntriesAndPathKinds()
throws Exception {
assertTrue(state.get(2).startsWith("modulepath:"));
}
+ @Test
+ void toleratesMissingClasspathDirectory() throws Exception {
+ Path missingMainOutput = temporaryDirectory.resolve("target/classes");
+
+ 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));
+ }
Review Comment:
⚠️ **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.
```suggestion
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));
}
```
##########
src/main/java/org/apache/maven/plugin/compiler/DependencyState.java:
##########
@@ -138,6 +138,9 @@ private static Path addDependencies(
* The directory digest covers each relevant file's relative path, size
and modification time.
Review Comment:
🔍 **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.
```suggestion
* 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.
```
--
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]
