gnodet-bot commented on code in PR #13193:
URL: https://github.com/apache/maven/pull/13193#discussion_r4050770821
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java:
##########
@@ -658,6 +658,42 @@ int getRunningJdkMajor() {
assertEquals(0, result.modifiedPoms().size());
}
+
+ @Test
+ @DisplayName("should not inject toolchain plugin when source level is
too new for running JDK (fixes #13189)")
+ void noToolchainInjectionWhenSourceLevelTooNew() {
+ // Simulate running JDK 17, project targets source 21 (too new,
not retired).
+ // latestJdkForSourceLevel(21) returns -1 — must NOT generate
'(,-1]' constraint.
+ ToolchainPluginStrategy strategy = new ToolchainPluginStrategy() {
+ @Override
+ int getRunningJdkMajor() {
+ return 17;
+ }
+ };
+
+ String pomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>com.example</groupId>
+ <artifactId>test</artifactId>
+ <version>1.0</version>
+ <properties>
+ <maven.compiler.release>21</maven.compiler.release>
+ </properties>
+ </project>
+ """;
+ Document doc = Document.of(pomXml);
+ UpgradeContext context = TestUtils.createMockContext();
+
+ UpgradeResult result = strategy.doApply(context, Map.of(POM_PATH,
doc));
+
+ // POM must not be modified — toolchain cannot help when the
source level
+ // is simply newer than the running JDK (no older JDK supports
--source 21)
+ assertEquals(0, result.modifiedPoms().size());
+ // The invalid (,-1] version range must never appear in the POM
+ assertFalse(doc.toXml().contains(",-1]"), "POM must not contain
invalid version range (,-1]");
Review Comment:
**[low]** The test asserts the POM is unchanged and the invalid string is
absent, but doesn't assert that `context.warning()` was called. The warning is
the only user-visible signal explaining why mvnup silently skipped the
toolchain injection — if it were removed or changed to `context.info()`, no
test would catch it.
`TestUtils.createMockContext()` currently only captures `logger.error()`
calls. To verify the warning, either:
1. Have `createMockContext()` also capture `warn()` calls and expose them, or
2. Use a Mockito spy on the returned context and verify the call after
`doApply()`.
Example approach (option 2):
```java
UpgradeContext context = spy(TestUtils.createMockContext());
UpgradeResult result = strategy.doApply(context, Map.of(POM_PATH, doc));
assertEquals(0, result.modifiedPoms().size());
assertFalse(doc.toXml().contains(",-1]"), "POM must not contain invalid
version range (,-1]");
verify(context, atLeastOnce()).warning(contains("too old"));
```
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java:
##########
@@ -658,6 +658,42 @@ int getRunningJdkMajor() {
assertEquals(0, result.modifiedPoms().size());
}
+
+ @Test
+ @DisplayName("should not inject toolchain plugin when source level is
too new for running JDK (fixes #13189)")
Review Comment:
**[nit]** `@DisplayName` strings are for human-readable test report labels —
embedding `(fixes #13189)` is non-standard here. The issue reference is already
in the commit message. Suggest dropping it:
```suggestion
@DisplayName("should not inject toolchain plugin when source level
is too new for running JDK")
```
--
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]
