gnodet-bot commented on code in PR #13193:
URL: https://github.com/apache/maven/pull/13193#discussion_r4052492832
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java:
##########
@@ -512,23 +513,13 @@ int getRunningJdkMajor() {
assertEquals(1, result.modifiedPoms().size());
assertTrue(strategy.hasToolchainsPluginWithSelectGoal(doc));
-
- // Verify that a warning about toolchain JDK availability was
emitted
- String xml = doc.toXml();
- assertTrue(xml.contains("select-jdk-toolchain"), "POM should
contain select-jdk-toolchain goal");
+ assertTrue(doc.toXml().contains("select-jdk-toolchain"), "POM
should contain select-jdk-toolchain goal");
}
@Test
@DisplayName("should emit warning about JDK availability when adding
toolchains plugin")
void shouldEmitJdkAvailabilityWarning() {
Review Comment:
**[medium]** `addsPluginForRetiredSourceLevel` and
`shouldEmitJdkAvailabilityWarning` are identical: same POM (`--source 6`), same
mock context, same strategy instance. The only difference is one asserts
`verify(...).warn(...)`. Two tests that duplicate every line of setup are test
noise — merge them into a single test that asserts both POM modification and
warning emission:
```suggestion
void addsPluginForRetiredSourceLevel() {
// source 6 is retired (last supported by JDK 11) — toolchain
must be added
// regardless of the JDK running mvnup, and a warning about JDK
availability must be emitted
```
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java:
##########
@@ -548,25 +539,14 @@ int getRunningJdkMajor() {
assertEquals(1, result.modifiedPoms().size());
assertTrue(strategy.hasToolchainsPluginWithSelectGoal(doc));
-
- // The output should contain the toolchains plugin and version
constraint
- String xml = doc.toXml();
- assertTrue(xml.contains("select-jdk-toolchain"), "POM should
contain select-jdk-toolchain goal");
- // Verify the warning about JDK availability was emitted
+ assertTrue(doc.toXml().contains("select-jdk-toolchain"), "POM
should contain select-jdk-toolchain goal");
verify(context.logger).warn(contains("must be installed"));
}
@Test
- @DisplayName("should not modify POM when source level is compatible")
- void noModificationWhenCompatible() {
- // Simulate running JDK 17, project targets source 11
- ToolchainPluginStrategy strategy = new ToolchainPluginStrategy() {
- @Override
- int getRunningJdkMajor() {
- return 17;
- }
- };
-
+ @DisplayName("should not modify POM when source level is current (not
retired)")
+ void noModificationForCurrentSourceLevel() {
Review Comment:
**[high]** The `noToolchainInjectionWhenSourceLevelTooNew` test was the
regression pin for #13189: it explicitly asserted
`assertFalse(doc.toXml().contains(",-1]"))` against `--source 21` (the exact
value that triggered the bug). That assertion is now gone.
The new code still prevents the regression — `latestJdkForSourceLevel(21) =
-1 ≤ 0 → continue` before `addToolchainsPlugin` — but there's no test that
would fail if someone accidentally removed or inverted that guard in the
future. `noModificationForCurrentSourceLevel` uses `--source 11` which
exercises a different entry (`latestJdkForSourceLevel` returns `-1` for both,
but 11 was never the failure case).
Add the direct regression assertion. Either change this test's source level
to `21` and add the `assertFalse`, or add a dedicated test:
```suggestion
void noModificationForCurrentSourceLevel() {
// source 21 is not retired — must not generate '(,-1]' version
range (regression: #13189)
```
--
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]
