gnodet-bot commented on code in PR #13587:
URL: https://github.com/apache/maven/pull/13587#discussion_r4311829575
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng6255FixConcatLines.java:
##########
@@ -70,8 +70,20 @@ void testJvmConfigFileCRLF() throws Exception {
}
protected void runWithLineEndings(String lineEndings, String test) throws
Exception {
- Path baseDir = extractResources("mng-6255");
+ // Each line-ending variant gets its own copy of the test project so
that
+ // concurrent or sequential test variants do not overwrite each
other's jvm.config.
+ Path baseDir = extractResources("mng-6255").resolveSibling("mng-6255-"
+ test);
+ Files.createDirectories(baseDir);
+
+ // Copy pom.xml from the shared resource directory populated during
test-resources phase
+ Path sharedDir = extractResources("mng-6255");
Review Comment:
💡 `extractResources("mng-6255")` is called twice — once to derive `baseDir`
and once to get `sharedDir`. Since `extractResources` just computes a path
under `maven.test.tmpdir`, deriving `baseDir` from `sharedDir` avoids the
redundancy and makes the intent clearer:
```suggestion
// Each line-ending variant gets its own copy of the test project so
that
// concurrent or sequential test variants do not overwrite each
other's jvm.config.
Path sharedDir = extractResources("mng-6255");
Path baseDir = sharedDir.resolveSibling("mng-6255-" + test);
Files.createDirectories(baseDir);
// Copy pom.xml from the shared resource directory populated during
test-resources phase
```
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng6255FixConcatLines.java:
##########
@@ -70,8 +70,20 @@ void testJvmConfigFileCRLF() throws Exception {
}
protected void runWithLineEndings(String lineEndings, String test) throws
Exception {
- Path baseDir = extractResources("mng-6255");
+ // Each line-ending variant gets its own copy of the test project so
that
+ // concurrent or sequential test variants do not overwrite each
other's jvm.config.
+ Path baseDir = extractResources("mng-6255").resolveSibling("mng-6255-"
+ test);
+ Files.createDirectories(baseDir);
+
+ // Copy pom.xml from the shared resource directory populated during
test-resources phase
+ Path sharedDir = extractResources("mng-6255");
+ Path pom = sharedDir.resolve("pom.xml");
+ if (Files.exists(pom)) {
+ Files.copy(pom, baseDir.resolve("pom.xml"),
java.nio.file.StandardCopyOption.REPLACE_EXISTING);
+ }
Review Comment:
💡 The `Files.exists(pom)` guard isn't needed — `pom.xml` is always present
in the resource directory (it's a mandatory test fixture), and if it were
missing the copy would fail with a clear `NoSuchFileException` rather than
silently skipping the copy (which would cause a confusing downstream failure
when Maven can't find the POM).
Also, `java.nio.file.StandardCopyOption` should be imported at the top of
the file alongside the other `java.nio.file.*` imports — using the FQCN inline
is inconsistent with the file's existing style.
```suggestion
Files.copy(pom, baseDir.resolve("pom.xml"),
StandardCopyOption.REPLACE_EXISTING);
```
--
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]
