gnodet-bot commented on code in PR #13657:
URL: https://github.com/apache/maven/pull/13657#discussion_r4329663618
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4559SpacesInJvmOptsTest.java:
##########
@@ -48,6 +48,14 @@ void testIt() throws Exception {
verifier.verifyErrorFreeLog();
Properties props = verifier.loadProperties("target/pom.properties");
+ // On Windows CI, mvn.cmd occasionally fails to read .mvn/jvm.config
on the first
+ // attempt due to filesystem flush delays, leaving properties as
unresolved placeholders.
+ String jvmOpts =
props.getProperty("project.properties.pom.prop.jvm-opts");
+ if (jvmOpts != null && jvmOpts.contains("${")) {
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+ props = verifier.loadProperties("target/pom.properties");
+ }
Review Comment:
Same concerns apply here as in `MavenITmng4559MultipleJvmArgsTest`: the
retry is platform-unconditional, fires immediately without a delay, and would
silently suppress a real launcher regression. Please address both tests
consistently.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4559MultipleJvmArgsTest.java:
##########
@@ -53,6 +53,14 @@ void testMultipleJvmArgs() throws Exception {
verifier.verifyErrorFreeLog();
Properties props = verifier.loadProperties("target/jvm.properties");
+ // On Windows CI, mvn.cmd occasionally fails to read .mvn/jvm.config
on the first
+ // attempt due to filesystem flush delays, leaving properties as
unresolved placeholders.
+ String prop1 = props.getProperty("project.properties.pom.test.prop1");
+ if (prop1 != null && prop1.contains("${")) {
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+ props = verifier.loadProperties("target/jvm.properties");
+ }
Review Comment:
⚠️ **Retry masks a real launcher regression and fires on any platform.**
The comment says "On Windows CI" but the `if` condition has no OS check — it
fires on any platform where `${` appears in the property value. More
importantly, a silent re-execute means a real regression in mvn.cmd (or mvnw)
that causes placeholder leakage will pass CI undetected: the retry silently
swallows it on the second attempt instead of catching it.
PR #13652 appears to address the root cause in mvn.cmd itself. Is this
workaround intended as a temporary bridge until that fix lands, or is it a
permanent safety net? If temporary, please add a `TODO: remove once #13652 is
merged` comment. If permanent, the condition should at minimum be gated on
Windows (e.g. `System.getProperty("os.name").contains("Windows")`) so it cannot
suppress failures on Linux/macOS CI, and a log message should be added so the
retry is visible in the build output.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4559MultipleJvmArgsTest.java:
##########
@@ -53,6 +53,14 @@ void testMultipleJvmArgs() throws Exception {
verifier.verifyErrorFreeLog();
Properties props = verifier.loadProperties("target/jvm.properties");
+ // On Windows CI, mvn.cmd occasionally fails to read .mvn/jvm.config
on the first
+ // attempt due to filesystem flush delays, leaving properties as
unresolved placeholders.
+ String prop1 = props.getProperty("project.properties.pom.test.prop1");
+ if (prop1 != null && prop1.contains("${")) {
+ verifier.execute();
Review Comment:
⚠️ **No delay before retry.**
The stated root cause is a filesystem flush race (the file is not yet
flushed when first read). Retrying immediately — with zero delay — gives the OS
no time to complete the flush. If the race condition is real, the retry has a
high probability of failing again under load. Consider adding a brief
`Thread.sleep(500)` or similar before `verifier.execute()`.
--
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]
