You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by gn...@apache.org on 2025/07/15 09:35:12 UTC
(maven) branch master updated: Fix mvnup tool issues #7934-#7938 (#9311)
This is an automated email from the ASF dual-hosted git repository.
gnodet pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven.git
The following commit(s) were added to refs/heads/master by this push:
new adf2c49d18 Fix mvnup tool issues #7934-#7938 (#9311)
adf2c49d18 is described below
commit adf2c49d18058a072646b77107fac6b63c853c98
Author: Guillaume Nodet <gn...@gmail.com>
AuthorDate: Tue Jul 15 11:35:08 2025 +0200
Fix mvnup tool issues #7934-#7938 (#9311)
* Fix mvnup tool issues #7934-#7938
This commit addresses five critical issues with the Maven upgrade tool (mvnup):
**Issue #7934: External parent preservation**
- Fixed InferenceStrategy to detect external parents vs reactor parents
- External parents now preserve required groupId, artifactId, and version elements
- Prevents 'parent.groupId is missing' errors when using META-POMs
**Issue #7935: .mvn directory creation for all model versions**
- Modified AbstractUpgradeGoal to create .mvn directory for both 4.0.0 and 4.1.0
- Helps Maven find project root and avoids root directory warnings
**Issue #7936: Downgrade error handling**
- Changed ModelUpgradeStrategy to fail with errors instead of warnings for invalid downgrades
- Tool now properly exits with error code when attempting 4.1.0 → 4.0.0 downgrade
**Issue #7937: Unicode icon fallback**
- Enhanced UpgradeContext to detect Unicode support using terminal.stdoutEncoding()
- Falls back to ASCII characters ([OK], [ERROR], etc.) when Unicode not supported
- Improves compatibility with Windows CMD/PowerShell and other terminals
**Issue #7938: Child version removal in 4.1.0**
- Enhanced inference logic to properly remove matching child versions in subprojects
- Ensures child project versions are removed when they match parent version
**Testing:**
- Updated existing tests to reflect correct behavior
- Added new tests for downgrade handling and Unicode detection
- All mvnup tests passing with comprehensive coverage
Fixes #7934, #7935, #7936, #7937, #7938
* Improve parent reactor detection logic
- Replace heuristic-based approach with direct GAV matching against pomMap
- More accurate detection of external vs reactor parents
- Added comprehensive test coverage for both scenarios
- Suggested by code review feedback
* Refactor to use GAVUtils.extractGAVWithParentResolution
- Replace manual GAV extraction with existing GAVUtils method
- More robust handling of parent resolution and inheritance
- Leverages existing tested utility for GAV extraction
- Cleaner and more maintainable code
* Address review feedback: Implement ConsoleIcon enum with charset detection
This commit addresses all review feedback from @elharo and @Bukama:
**Icon Management Improvements:**
- Created ConsoleIcon enum with Unicode/ASCII fallback pairs
- Moved all icon logic into the enum for consistency and reusability
- Implemented ConsoleIcon.getIcon(Terminal) for clean integration
**Unicode Detection Enhancements:**
- Use Charset.newEncoder().canEncode(char) for accurate character testing
- Use terminal.encoding() instead of deprecated stdoutEncoding()
- Simplified fallback to Charset.defaultCharset()
- Removed complex heuristics and exception handling
**Code Quality Improvements:**
- UpgradeContext methods are now clean one-liners
- Better separation of concerns between icon rendering and context management
- Comprehensive test coverage for all scenarios
- Future-proof design for adding new icons
**Review Comments Addressed:**
- @elharo: 'maybe this should be a field that can be shared?' → Icons now shared via enum
- @Bukama: 'maybe an enum with icon and fallback text?' → Implemented with Terminal integration
- @elharo: 'There are others that also support Unicode, e.g. UCS-2., UCS-4' → Now tests actual encoding capability
- @elharo: 'Please be specific about exceptions' → No longer needed with simplified approach
- @elharo: 'toLowerCase(Locale.ENGLISH)' → No longer needed with canEncode() approach
**Technical Benefits:**
- More accurate Unicode detection per character
- Works with any charset automatically
- Cleaner and more maintainable code
- Better test coverage and reliability
---
.../maven/cling/invoker/mvnup/ConsoleIcon.java | 108 +++++++++++++++
.../maven/cling/invoker/mvnup/UpgradeContext.java | 10 +-
.../invoker/mvnup/goals/AbstractUpgradeGoal.java | 7 +-
.../invoker/mvnup/goals/InferenceStrategy.java | 101 +++++++++-----
.../invoker/mvnup/goals/ModelUpgradeStrategy.java | 4 +-
.../maven/cling/invoker/mvnup/ConsoleIconTest.java | 154 +++++++++++++++++++++
.../cling/invoker/mvnup/UpgradeContextTest.java | 87 ++++++++++++
.../mvnup/goals/AbstractUpgradeGoalTest.java | 11 +-
.../invoker/mvnup/goals/InferenceStrategyTest.java | 63 ++++++++-
.../mvnup/goals/ModelUpgradeStrategyTest.java | 59 ++++++++
.../goals/UpgradeWorkflowIntegrationTest.java | 9 +-
11 files changed, 559 insertions(+), 54 deletions(-)
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/ConsoleIcon.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/ConsoleIcon.java
new file mode 100644
index 0000000000..1066c04f81
--- /dev/null
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/ConsoleIcon.java
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnup;
+
+import java.nio.charset.Charset;
+
+import org.jline.terminal.Terminal;
+
+/**
+ * Console icons for Maven upgrade tool output.
+ * Each icon has a Unicode character and an ASCII fallback.
+ * The appropriate representation is chosen based on the terminal's charset capabilities.
+ */
+public enum ConsoleIcon {
+ /**
+ * Success/completion icon.
+ */
+ SUCCESS('✓', "[OK]"),
+
+ /**
+ * Error/failure icon.
+ */
+ ERROR('✗', "[ERROR]"),
+
+ /**
+ * Warning icon.
+ */
+ WARNING('⚠', "[WARNING]"),
+
+ /**
+ * Detail/bullet point icon.
+ */
+ DETAIL('•', "-"),
+
+ /**
+ * Action/arrow icon.
+ */
+ ACTION('→', ">");
+
+ private final char unicodeChar;
+ private final String asciiFallback;
+
+ ConsoleIcon(char unicodeChar, String asciiFallback) {
+ this.unicodeChar = unicodeChar;
+ this.asciiFallback = asciiFallback;
+ }
+
+ /**
+ * Returns the appropriate icon representation for the given terminal.
+ * Tests if the terminal's charset can encode the Unicode character,
+ * falling back to ASCII if not.
+ *
+ * @param terminal the terminal to get the icon for
+ * @return the Unicode character if supported, otherwise the ASCII fallback
+ */
+ public String getIcon(Terminal terminal) {
+ Charset charset = getTerminalCharset(terminal);
+ return charset.newEncoder().canEncode(unicodeChar) ? String.valueOf(unicodeChar) : asciiFallback;
+ }
+
+ /**
+ * Gets the charset used by the terminal for output.
+ * Falls back to the system default charset if terminal charset is not available.
+ *
+ * @param terminal the terminal to get the charset from
+ * @return the terminal's output charset or the system default charset
+ */
+ private static Charset getTerminalCharset(Terminal terminal) {
+ if (terminal != null && terminal.encoding() != null) {
+ return terminal.encoding();
+ }
+ return Charset.defaultCharset();
+ }
+
+ /**
+ * Returns the Unicode character for this icon.
+ *
+ * @return the Unicode character
+ */
+ public char getUnicodeChar() {
+ return unicodeChar;
+ }
+
+ /**
+ * Returns the ASCII fallback text for this icon.
+ *
+ * @return the ASCII fallback text
+ */
+ public String getAsciiFallback() {
+ return asciiFallback;
+ }
+}
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java
index bef4e344fd..eef2e59274 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java
@@ -118,35 +118,35 @@ public void println() {
* Logs a successful operation with a checkmark icon.
*/
public void success(String message) {
- logger.info(getCurrentIndent() + "✓ " + message);
+ logger.info(getCurrentIndent() + ConsoleIcon.SUCCESS.getIcon(terminal) + " " + message);
}
/**
* Logs an error with an X icon.
*/
public void failure(String message) {
- logger.error(getCurrentIndent() + "✗ " + message);
+ logger.error(getCurrentIndent() + ConsoleIcon.ERROR.getIcon(terminal) + " " + message);
}
/**
* Logs a warning with a warning icon.
*/
public void warning(String message) {
- logger.warn(getCurrentIndent() + "⚠ " + message);
+ logger.warn(getCurrentIndent() + ConsoleIcon.WARNING.getIcon(terminal) + " " + message);
}
/**
* Logs detailed information with a bullet point.
*/
public void detail(String message) {
- logger.info(getCurrentIndent() + "• " + message);
+ logger.info(getCurrentIndent() + ConsoleIcon.DETAIL.getIcon(terminal) + " " + message);
}
/**
* Logs a performed action with an arrow icon.
*/
public void action(String message) {
- logger.info(getCurrentIndent() + "→ " + message);
+ logger.info(getCurrentIndent() + ConsoleIcon.ACTION.getIcon(terminal) + " " + message);
}
/**
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoal.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoal.java
index dfd14967cc..b36740613f 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoal.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoal.java
@@ -208,10 +208,9 @@ protected int doUpgrade(UpgradeContext context, String targetModel, Map<Path, Do
try {
UpgradeResult result = orchestrator.executeStrategies(context, pomMap);
- // Create .mvn directory if needed (when not upgrading to 4.1.0)
- if (!MODEL_VERSION_4_1_0.equals(targetModel)) {
- createMvnDirectoryIfNeeded(context);
- }
+ // Create .mvn directory if needed to avoid root directory warnings
+ // This is needed for both 4.0.0 and 4.1.0 to help Maven find the project root
+ createMvnDirectoryIfNeeded(context);
return result.success() ? 0 : 1;
} catch (Exception e) {
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategy.java
index 6ef7671b24..4f053f2d60 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategy.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategy.java
@@ -388,46 +388,85 @@ private boolean trimParentElementFull(
Map<Path, Document> pomMap) {
boolean hasChanges = false;
- // First apply limited inference (child elements)
- hasChanges |= trimParentElementLimited(context, root, parentElement, namespace);
-
- // Get child GAV
+ // Get child GAV before applying any changes
String childGroupId = getChildText(root, GROUP_ID, namespace);
String childVersion = getChildText(root, VERSION, namespace);
- // Remove parent groupId if child has no explicit groupId
- if (childGroupId == null) {
- Element parentGroupIdElement = parentElement.getChild(GROUP_ID, namespace);
- if (parentGroupIdElement != null) {
- removeElementWithFormatting(parentGroupIdElement);
- context.detail("Removed: parent groupId (child has no explicit groupId)");
- hasChanges = true;
+ // First apply limited inference (child elements) - this removes matching child groupId/version
+ hasChanges |= trimParentElementLimited(context, root, parentElement, namespace);
+
+ // Only remove parent elements if the parent is in the same reactor (not external)
+ if (isParentInReactor(parentElement, namespace, pomMap, context)) {
+ // Remove parent groupId if child has no explicit groupId
+ if (childGroupId == null) {
+ Element parentGroupIdElement = parentElement.getChild(GROUP_ID, namespace);
+ if (parentGroupIdElement != null) {
+ removeElementWithFormatting(parentGroupIdElement);
+ context.detail("Removed: parent groupId (child has no explicit groupId)");
+ hasChanges = true;
+ }
}
- }
- // Remove parent version if child has no explicit version
- if (childVersion == null) {
- Element parentVersionElement = parentElement.getChild(VERSION, namespace);
- if (parentVersionElement != null) {
- removeElementWithFormatting(parentVersionElement);
- context.detail("Removed: parent version (child has no explicit version)");
- hasChanges = true;
+ // Remove parent version if child has no explicit version
+ if (childVersion == null) {
+ Element parentVersionElement = parentElement.getChild(VERSION, namespace);
+ if (parentVersionElement != null) {
+ removeElementWithFormatting(parentVersionElement);
+ context.detail("Removed: parent version (child has no explicit version)");
+ hasChanges = true;
+ }
}
- }
- // Remove parent artifactId if it can be inferred from relativePath
- if (canInferParentArtifactId(parentElement, namespace, pomMap)) {
- Element parentArtifactIdElement = parentElement.getChild(ARTIFACT_ID, namespace);
- if (parentArtifactIdElement != null) {
- removeElementWithFormatting(parentArtifactIdElement);
- context.detail("Removed: parent artifactId (can be inferred from relativePath)");
- hasChanges = true;
+ // Remove parent artifactId if it can be inferred from relativePath
+ if (canInferParentArtifactId(parentElement, namespace, pomMap)) {
+ Element parentArtifactIdElement = parentElement.getChild(ARTIFACT_ID, namespace);
+ if (parentArtifactIdElement != null) {
+ removeElementWithFormatting(parentArtifactIdElement);
+ context.detail("Removed: parent artifactId (can be inferred from relativePath)");
+ hasChanges = true;
+ }
}
}
return hasChanges;
}
+ /**
+ * Determines if the parent is part of the same reactor (multi-module project)
+ * vs. an external parent POM by checking if the parent exists in the pomMap.
+ */
+ private boolean isParentInReactor(
+ Element parentElement, Namespace namespace, Map<Path, Document> pomMap, UpgradeContext context) {
+ // If relativePath is explicitly set to empty, parent is definitely external
+ String relativePath = getChildText(parentElement, RELATIVE_PATH, namespace);
+ if (relativePath != null && relativePath.trim().isEmpty()) {
+ return false;
+ }
+
+ // Extract parent GAV
+ String parentGroupId = getChildText(parentElement, GROUP_ID, namespace);
+ String parentArtifactId = getChildText(parentElement, ARTIFACT_ID, namespace);
+ String parentVersion = getChildText(parentElement, VERSION, namespace);
+
+ if (parentGroupId == null || parentArtifactId == null || parentVersion == null) {
+ // Cannot determine parent GAV, assume external
+ return false;
+ }
+
+ GAV parentGAV = new GAV(parentGroupId, parentArtifactId, parentVersion);
+
+ // Check if any POM in our reactor matches the parent GAV using GAVUtils
+ for (Document pomDocument : pomMap.values()) {
+ GAV pomGAV = GAVUtils.extractGAVWithParentResolution(context, pomDocument);
+ if (pomGAV != null && pomGAV.equals(parentGAV)) {
+ return true;
+ }
+ }
+
+ // Parent not found in reactor, must be external
+ return false;
+ }
+
/**
* Determines if parent artifactId can be inferred from relativePath.
*/
@@ -438,11 +477,9 @@ private boolean canInferParentArtifactId(Element parentElement, Namespace namesp
relativePath = DEFAULT_PARENT_RELATIVE_PATH; // Maven default
}
- // For now, we use a simple heuristic: if relativePath is the default "../pom.xml"
- // and we have parent POMs in our pomMap, we can likely infer the artifactId.
- // A more sophisticated implementation would resolve the actual path and check
- // if the parent POM exists in pomMap.
- return DEFAULT_PARENT_RELATIVE_PATH.equals(relativePath) && !pomMap.isEmpty();
+ // Only infer artifactId if relativePath is the default and we have multiple POMs
+ // indicating this is likely a multi-module project
+ return DEFAULT_PARENT_RELATIVE_PATH.equals(relativePath) && pomMap.size() > 1;
}
/**
diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java
index 8099300443..aeff300581 100644
--- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java
+++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java
@@ -114,7 +114,9 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> pomMap)
context.success("Model upgrade completed");
modifiedPoms.add(pomPath);
} else {
- context.warning("Cannot upgrade from " + currentVersion + " to " + targetModelVersion);
+ // Treat invalid upgrades (including downgrades) as errors, not warnings
+ context.failure("Cannot upgrade from " + currentVersion + " to " + targetModelVersion);
+ errorPoms.add(pomPath);
}
} catch (Exception e) {
context.failure("Model upgrade failed: " + e.getMessage());
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/ConsoleIconTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/ConsoleIconTest.java
new file mode 100644
index 0000000000..7b7aadfb2d
--- /dev/null
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/ConsoleIconTest.java
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnup;
+
+import java.nio.charset.StandardCharsets;
+
+import org.jline.terminal.Terminal;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for the {@link ConsoleIcon} enum.
+ * Tests icon rendering with different terminal charsets and fallback behavior.
+ */
+@DisplayName("ConsoleIcon")
+class ConsoleIconTest {
+
+ @Test
+ @DisplayName("should return Unicode icons when terminal supports UTF-8")
+ void shouldReturnUnicodeWhenTerminalSupportsUtf8() {
+ Terminal mockTerminal = mock(Terminal.class);
+ when(mockTerminal.encoding()).thenReturn(StandardCharsets.UTF_8);
+
+ assertEquals("✓", ConsoleIcon.SUCCESS.getIcon(mockTerminal));
+ assertEquals("✗", ConsoleIcon.ERROR.getIcon(mockTerminal));
+ assertEquals("⚠", ConsoleIcon.WARNING.getIcon(mockTerminal));
+ assertEquals("•", ConsoleIcon.DETAIL.getIcon(mockTerminal));
+ assertEquals("→", ConsoleIcon.ACTION.getIcon(mockTerminal));
+ }
+
+ @Test
+ @DisplayName("should return ASCII fallback when terminal uses US-ASCII")
+ void shouldReturnAsciiFallbackWhenTerminalUsesAscii() {
+ Terminal mockTerminal = mock(Terminal.class);
+ when(mockTerminal.encoding()).thenReturn(StandardCharsets.US_ASCII);
+
+ assertEquals("[OK]", ConsoleIcon.SUCCESS.getIcon(mockTerminal));
+ assertEquals("[ERROR]", ConsoleIcon.ERROR.getIcon(mockTerminal));
+ assertEquals("[WARNING]", ConsoleIcon.WARNING.getIcon(mockTerminal));
+ assertEquals("-", ConsoleIcon.DETAIL.getIcon(mockTerminal));
+ assertEquals(">", ConsoleIcon.ACTION.getIcon(mockTerminal));
+ }
+
+ @Test
+ @DisplayName("should handle null terminal gracefully")
+ void shouldHandleNullTerminal() {
+ // Should fall back to system default charset
+ for (ConsoleIcon icon : ConsoleIcon.values()) {
+ String result = icon.getIcon(null);
+ assertNotNull(result, "Icon result should not be null for " + icon);
+
+ // Result should be either Unicode or ASCII fallback depending on default charset
+ String expectedUnicode = String.valueOf(icon.getUnicodeChar());
+ String expectedAscii = icon.getAsciiFallback();
+ assertTrue(
+ result.equals(expectedUnicode) || result.equals(expectedAscii),
+ "Result should be either Unicode or ASCII fallback for " + icon + ", got: " + result);
+ }
+ }
+
+ @Test
+ @DisplayName("should handle terminal with null encoding")
+ void shouldHandleTerminalWithNullEncoding() {
+ Terminal mockTerminal = mock(Terminal.class);
+ when(mockTerminal.encoding()).thenReturn(null);
+
+ // Should fall back to system default charset
+ for (ConsoleIcon icon : ConsoleIcon.values()) {
+ String result = icon.getIcon(mockTerminal);
+ assertNotNull(result, "Icon result should not be null for " + icon);
+
+ // Result should be either Unicode or ASCII fallback depending on default charset
+ String expectedUnicode = String.valueOf(icon.getUnicodeChar());
+ String expectedAscii = icon.getAsciiFallback();
+ assertTrue(
+ result.equals(expectedUnicode) || result.equals(expectedAscii),
+ "Result should be either Unicode or ASCII fallback for " + icon + ", got: " + result);
+ }
+ }
+
+ @Test
+ @DisplayName("should return correct Unicode characters")
+ void shouldReturnCorrectUnicodeCharacters() {
+ assertEquals('✓', ConsoleIcon.SUCCESS.getUnicodeChar());
+ assertEquals('✗', ConsoleIcon.ERROR.getUnicodeChar());
+ assertEquals('⚠', ConsoleIcon.WARNING.getUnicodeChar());
+ assertEquals('•', ConsoleIcon.DETAIL.getUnicodeChar());
+ assertEquals('→', ConsoleIcon.ACTION.getUnicodeChar());
+ }
+
+ @Test
+ @DisplayName("should return correct ASCII fallbacks")
+ void shouldReturnCorrectAsciiFallbacks() {
+ assertEquals("[OK]", ConsoleIcon.SUCCESS.getAsciiFallback());
+ assertEquals("[ERROR]", ConsoleIcon.ERROR.getAsciiFallback());
+ assertEquals("[WARNING]", ConsoleIcon.WARNING.getAsciiFallback());
+ assertEquals("-", ConsoleIcon.DETAIL.getAsciiFallback());
+ assertEquals(">", ConsoleIcon.ACTION.getAsciiFallback());
+ }
+
+ @Test
+ @DisplayName("should handle different charset encodings correctly")
+ void shouldHandleDifferentCharsetEncodingsCorrectly() {
+ Terminal mockTerminal = mock(Terminal.class);
+
+ // Test with ISO-8859-1 (Latin-1) - should support some but not all Unicode chars
+ when(mockTerminal.encoding()).thenReturn(StandardCharsets.ISO_8859_1);
+
+ for (ConsoleIcon icon : ConsoleIcon.values()) {
+ String result = icon.getIcon(mockTerminal);
+ assertNotNull(result, "Icon result should not be null for " + icon);
+
+ // Result should be consistent with charset's canEncode capability
+ boolean canEncode = StandardCharsets.ISO_8859_1.newEncoder().canEncode(icon.getUnicodeChar());
+ String expected = canEncode ? String.valueOf(icon.getUnicodeChar()) : icon.getAsciiFallback();
+ assertEquals(expected, result, "Icon should match charset encoding capability for " + icon);
+ }
+ }
+
+ @Test
+ @DisplayName("should be consistent across multiple calls")
+ void shouldBeConsistentAcrossMultipleCalls() {
+ Terminal mockTerminal = mock(Terminal.class);
+ when(mockTerminal.encoding()).thenReturn(StandardCharsets.UTF_8);
+
+ for (ConsoleIcon icon : ConsoleIcon.values()) {
+ String firstCall = icon.getIcon(mockTerminal);
+ String secondCall = icon.getIcon(mockTerminal);
+ assertEquals(firstCall, secondCall, "Icon should be consistent across calls for " + icon);
+ }
+ }
+}
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/UpgradeContextTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/UpgradeContextTest.java
new file mode 100644
index 0000000000..c1c7d82ff5
--- /dev/null
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/UpgradeContextTest.java
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker.mvnup;
+
+import java.nio.file.Paths;
+
+import org.apache.maven.cling.invoker.mvnup.goals.TestUtils;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Unit tests for the {@link UpgradeContext} class.
+ * Tests console output formatting and Unicode icon fallback behavior.
+ */
+@DisplayName("UpgradeContext")
+class UpgradeContextTest {
+
+ @Test
+ @DisplayName("should create context successfully")
+ void shouldCreateContextSuccessfully() {
+ // Use existing test utilities to create a context
+ UpgradeContext context = TestUtils.createMockContext(Paths.get("/test"));
+
+ // Verify context is created and basic methods work
+ assertNotNull(context, "Context should be created");
+ assertNotNull(context.options(), "Options should be available");
+
+ // Test that icon methods don't throw exceptions
+ // (The actual icon choice depends on terminal charset capabilities)
+ context.success("Test success message");
+ context.failure("Test failure message");
+ context.warning("Test warning message");
+ context.detail("Test detail message");
+ context.action("Test action message");
+ }
+
+ @Test
+ @DisplayName("should handle indentation correctly")
+ void shouldHandleIndentationCorrectly() {
+ UpgradeContext context = TestUtils.createMockContext(Paths.get("/test"));
+
+ // Test indentation methods don't throw exceptions
+ context.indent();
+ context.indent();
+ context.info("Indented message");
+
+ context.unindent();
+ context.unindent();
+ context.unindent(); // Should not go below 0
+ context.info("Unindented message");
+ }
+
+ @Test
+ @DisplayName("should handle icon rendering based on terminal capabilities")
+ void shouldHandleIconRenderingBasedOnTerminalCapabilities() {
+ UpgradeContext context = TestUtils.createMockContext(Paths.get("/test"));
+
+ // Test that icon rendering doesn't throw exceptions
+ // The actual icons used depend on the terminal's charset capabilities
+ context.success("Icon rendering test");
+ context.failure("Icon rendering test");
+ context.warning("Icon rendering test");
+ context.detail("Icon rendering test");
+ context.action("Icon rendering test");
+
+ // We just verify the methods work without throwing exceptions
+ // The specific icons (Unicode vs ASCII) depend on terminal charset
+ }
+}
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoalTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoalTest.java
index 59a4b720e8..fba9ce3e46 100644
--- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoalTest.java
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoalTest.java
@@ -39,7 +39,6 @@
import org.mockito.Mockito;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -189,8 +188,8 @@ void shouldCreateMvnDirectoryWhenModelVersionNot410() throws Exception {
}
@Test
- @DisplayName("should not create .mvn directory when model version is 4.1.0")
- void shouldNotCreateMvnDirectoryWhenModelVersion410() throws Exception {
+ @DisplayName("should create .mvn directory when model version is 4.1.0")
+ void shouldCreateMvnDirectoryWhenModelVersion410() throws Exception {
Path projectDir = tempDir.resolve("project");
Files.createDirectories(projectDir);
@@ -200,11 +199,13 @@ void shouldNotCreateMvnDirectoryWhenModelVersion410() throws Exception {
when(mockOrchestrator.executeStrategies(Mockito.any(), Mockito.any()))
.thenReturn(UpgradeResult.empty());
- // Execute with target model 4.1.0 (should not create .mvn directory)
+ // Execute with target model 4.1.0 (should create .mvn directory to avoid root warnings)
upgradeGoal.testExecuteWithTargetModel(context, "4.1.0");
Path mvnDir = projectDir.resolve(".mvn");
- assertFalse(Files.exists(mvnDir), ".mvn directory should not be created for 4.1.0");
+ assertTrue(
+ Files.exists(mvnDir),
+ ".mvn directory should be created for 4.1.0 to avoid root directory warnings");
}
@Test
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategyTest.java
index 26e8d6fc2e..766c30be58 100644
--- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategyTest.java
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategyTest.java
@@ -484,9 +484,66 @@ void shouldNotTrimParentElementsWhenParentIsExternal() throws Exception {
strategy.apply(context, pomMap);
// Verify correct behavior for external parent:
- // - groupId should be removed (child doesn't have explicit groupId, can inherit from parent)
- // - version should be removed (child doesn't have explicit version, can inherit from parent)
- // - artifactId should be removed (Maven 4.1.0+ can infer from relativePath even for external parents)
+ // - groupId should NOT be removed (external parents need groupId to be located)
+ // - artifactId should NOT be removed (external parents need artifactId to be located)
+ // - version should NOT be removed (external parents need version to be located)
+ // This prevents the "parent.groupId is missing" error reported in issue #7934
+ assertNotNull(parentElement.getChild("groupId", childRoot.getNamespace()));
+ assertNotNull(parentElement.getChild("artifactId", childRoot.getNamespace()));
+ assertNotNull(parentElement.getChild("version", childRoot.getNamespace()));
+ }
+
+ @Test
+ @DisplayName("should trim parent elements when parent is in reactor")
+ void shouldTrimParentElementsWhenParentIsInReactor() throws Exception {
+ // Create parent POM
+ String parentPomXml =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.1.0">
+ <modelVersion>4.1.0</modelVersion>
+ <groupId>com.example</groupId>
+ <artifactId>parent-project</artifactId>
+ <version>1.0.0</version>
+ <packaging>pom</packaging>
+ </project>
+ """;
+
+ // Create child POM that references the parent
+ String childPomXml =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.1.0">
+ <modelVersion>4.1.0</modelVersion>
+ <parent>
+ <groupId>com.example</groupId>
+ <artifactId>parent-project</artifactId>
+ <version>1.0.0</version>
+ </parent>
+ <artifactId>child-project</artifactId>
+ <!-- No explicit groupId or version - would inherit from parent -->
+ </project>
+ """;
+
+ Document parentDoc = saxBuilder.build(new StringReader(parentPomXml));
+ Document childDoc = saxBuilder.build(new StringReader(childPomXml));
+
+ // Both POMs are in the reactor
+ Map<Path, Document> pomMap = Map.of(
+ Paths.get("pom.xml"), parentDoc,
+ Paths.get("child", "pom.xml"), childDoc);
+
+ Element childRoot = childDoc.getRootElement();
+ Element parentElement = childRoot.getChild("parent", childRoot.getNamespace());
+
+ // Apply inference
+ UpgradeContext context = createMockContext();
+ strategy.apply(context, pomMap);
+
+ // Verify correct behavior for reactor parent:
+ // - groupId should be removed (child has no explicit groupId, parent is in reactor)
+ // - artifactId should be removed (can be inferred from relativePath)
+ // - version should be removed (child has no explicit version, parent is in reactor)
assertNull(parentElement.getChild("groupId", childRoot.getNamespace()));
assertNull(parentElement.getChild("artifactId", childRoot.getNamespace()));
assertNull(parentElement.getChild("version", childRoot.getNamespace()));
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategyTest.java
index 4ca99a9301..2a0c3c1719 100644
--- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategyTest.java
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategyTest.java
@@ -323,4 +323,63 @@ void shouldProvideMeaningfulDescription() {
"Description should mention model or upgrade");
}
}
+
+ @Nested
+ @DisplayName("Downgrade Handling")
+ class DowngradeHandlingTests {
+
+ @Test
+ @DisplayName("should fail with error when attempting downgrade from 4.1.0 to 4.0.0")
+ void shouldFailWhenAttemptingDowngrade() throws Exception {
+ String pomXml =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.1.0">
+ <modelVersion>4.1.0</modelVersion>
+ <groupId>com.example</groupId>
+ <artifactId>test-project</artifactId>
+ <version>1.0.0</version>
+ </project>
+ """;
+
+ Document document = saxBuilder.build(new StringReader(pomXml));
+ Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document);
+
+ UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.0.0"));
+
+ UpgradeResult result = strategy.apply(context, pomMap);
+
+ // Should have errors (not just warnings)
+ assertTrue(result.errorCount() > 0, "Downgrade should result in errors");
+ assertFalse(result.success(), "Downgrade should not be successful");
+ assertEquals(1, result.errorCount(), "Should have exactly one error");
+ }
+
+ @Test
+ @DisplayName("should succeed when upgrading from 4.0.0 to 4.1.0")
+ void shouldSucceedWhenUpgrading() throws Exception {
+ 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-project</artifactId>
+ <version>1.0.0</version>
+ </project>
+ """;
+
+ Document document = saxBuilder.build(new StringReader(pomXml));
+ Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document);
+
+ UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0"));
+
+ UpgradeResult result = strategy.apply(context, pomMap);
+
+ // Should succeed
+ assertTrue(result.success(), "Valid upgrade should be successful");
+ assertEquals(0, result.errorCount(), "Should have no errors");
+ assertEquals(1, result.modifiedCount(), "Should have modified one POM");
+ }
+ }
}
diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeWorkflowIntegrationTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeWorkflowIntegrationTest.java
index eb2386530c..5a652d557f 100644
--- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeWorkflowIntegrationTest.java
+++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/UpgradeWorkflowIntegrationTest.java
@@ -30,7 +30,6 @@
import org.junit.jupiter.api.io.TempDir;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
@@ -94,8 +93,8 @@ void shouldUpgradeModelVersionWith41Option() throws Exception {
}
@Test
- @DisplayName("should not create .mvn directory when upgrading to 4.1.0")
- void shouldNotCreateMvnDirectoryFor41Upgrade() throws Exception {
+ @DisplayName("should create .mvn directory when upgrading to 4.1.0")
+ void shouldCreateMvnDirectoryFor41Upgrade() throws Exception {
Path pomFile = tempDir.resolve("pom.xml");
String originalPom = PomBuilder.create()
.groupId("com.example")
@@ -110,7 +109,9 @@ void shouldNotCreateMvnDirectoryFor41Upgrade() throws Exception {
applyGoal.execute(context);
Path mvnDir = tempDir.resolve(".mvn");
- assertFalse(Files.exists(mvnDir), ".mvn directory should not be created for 4.1.0 upgrade");
+ assertTrue(
+ Files.exists(mvnDir),
+ ".mvn directory should be created for 4.1.0 upgrade to avoid root directory warnings");
}
}