gnodet-bot commented on code in PR #1125:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1125#discussion_r4084558280
##########
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:
⚠️ **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:
```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:
🔍 **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.
```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]
