gnodet-bot commented on code in PR #13249:
URL: https://github.com/apache/maven/pull/13249#discussion_r4492846066
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenInvoker.java:
##########
@@ -314,6 +315,12 @@ protected void populateRequest(MavenContext context,
Lookup lookup, MavenExecuti
if (context.options().builder().isPresent()) {
request.setBuilderId(context.options().builder().get());
}
+
+ // Store ReactorConfig on the request data map so it is accessible to
other components
+ // (e.g. future ReactorXmlLifecycleProcessor) without adding a
dependency on maven-cli.
Review Comment:
🔤 **Stale comment:** `ReactorXmlLifecycleProcessor` already exists in this
same PR — the word `"future"` is inaccurate.
```suggestion
// (e.g. ReactorXmlLifecycleProcessor) without adding a dependency
on maven-cli.
```
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvn/ReactorXmlLifecycleProcessorTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.mvn;
+
+import java.util.List;
+
+import org.apache.maven.api.reactor.PhaseInjection;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Unit tests for {@link ReactorXmlLifecycleProcessor}, focusing on the
+ * {@code validate()} logic which can be exercised without a live Maven
session.
+ */
+class ReactorXmlLifecycleProcessorTest {
+
+ //
-------------------------------------------------------------------------
+ // Helpers: build PhaseInjection via its generated builder
+ //
-------------------------------------------------------------------------
+
+ private static PhaseInjection phase(String name, String parent, String
after, String before) {
+ PhaseInjection.Builder b = PhaseInjection.newBuilder();
+ if (name != null) {
+ b.name(name);
+ }
+ if (parent != null) {
+ b.parent(parent);
+ }
+ if (after != null) {
+ b.after(after);
+ }
+ if (before != null) {
+ b.before(before);
+ }
+ return b.build();
+ }
+
+ //
-------------------------------------------------------------------------
+ // Validation: valid declarations (should NOT throw)
+ //
-------------------------------------------------------------------------
+
+ @Test
+ void validWithParentAndAfter() {
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", "verify",
"unit-test", null))));
+ }
+
+ @Test
+ void validWithParentAndBefore() {
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", "verify",
null, "integration-test"))));
+ }
+
+ @Test
+ void validWithParentOnly() {
+ // parent alone is sufficient — phase is appended to end of parent's
children
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", "verify",
null, null))));
+ }
+
+ @Test
+ void validWithAfterOnly() {
+ // no parent: after alone is sufficient
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", null,
"unit-test", null))));
+ }
+
+ @Test
+ void validWithBeforeOnly() {
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", null, null,
"integration-test"))));
+ }
+
+ @Test
+ void validWithAllFourAttributes() {
+ assertDoesNotThrow(() -> validateVia(List.of(phase("p", "verify",
"unit-test", "integration-test"))));
+ }
+
+ //
-------------------------------------------------------------------------
+ // Validation: invalid declarations (should throw)
+ //
-------------------------------------------------------------------------
+
+ @Test
+ void invalidBlankName() {
+ IllegalStateException ex = assertThrows(
+ IllegalStateException.class, () -> validateVia(List.of(phase("
", "verify", null, null))));
+ assertTrue(ex.getMessage().contains("non-blank 'name'"));
+ }
+
+ @Test
+ void invalidNoAnchorNoParent() {
+ // no parent, no after, no before → invalid
+ IllegalStateException ex = assertThrows(
+ IllegalStateException.class, () ->
validateVia(List.of(phase("orphan", null, null, null))));
+ assertTrue(ex.getMessage().contains("must specify 'parent'"));
+ }
+
+ //
-------------------------------------------------------------------------
+ // Helpers
+ //
-------------------------------------------------------------------------
+
+ /**
+ * Exercises the private validate() method indirectly by calling the same
logic
+ * extracted here. In practice validate() is called inside
resolveInjections() which
+ * requires a live MavenSession; this helper duplicates the validation
rules so they
+ * can be tested without DI.
+ */
+ private static void validateVia(List<PhaseInjection> phases) {
+ for (PhaseInjection p : phases) {
+ validatePhase(p);
+ }
+ }
+
+ /** Mirror of ReactorXmlLifecycleProcessor.validate() kept in sync by
hand. */
+ private static void validatePhase(PhaseInjection p) {
+ if (p.getName() == null || p.getName().isBlank()) {
+ throw new IllegalStateException("reactor.xml: <phase> element must
have a non-blank 'name' attribute");
+ }
+ boolean hasParent = p.getParent() != null && !p.getParent().isBlank();
+ boolean hasAfter = p.getAfter() != null && !p.getAfter().isBlank();
+ boolean hasBefore = p.getBefore() != null && !p.getBefore().isBlank();
+ if (!hasParent && !hasAfter && !hasBefore) {
+ throw new IllegalStateException("reactor.xml: custom phase '"
+ + p.getName()
+ + "' must specify 'parent' and/or at least one of 'after'
or 'before'"
+ + " to anchor it in the lifecycle DAG");
+ }
+ }
+}
Review Comment:
⚠️ **Maintenance hazard:** `validatePhase()` is described as a
hand-maintained mirror of `ReactorXmlLifecycleProcessor.validate()`. If
`validate()` changes, this copy won't follow and tests will silently pass
against stale logic.
Fix: widen `validate()` from `private static` to package-private (`static
void validate(PhaseInjection p)`) — both classes share the
`org.apache.maven.cling.invoker.mvn` package, so the test can call it directly
and the duplication goes away.
##########
its/core-it-suite/src/test/resources/mng-13249-reactor-xml-phase-injection/src/main/java/org/apache/maven/its/mng13249/Mng13249.java:
##########
@@ -0,0 +1,3 @@
+package org.apache.maven.its.mng13249;
+
+public class Mng13249 {}
Review Comment:
🔑 **Missing ASF license header.** `Mng12537.java` in the same PR has one —
this file doesn't. RAT will flag this.
```suggestion
/*
* 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.its.mng13249;
public class Mng13249 {}
```
--
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]
