gnodet-bot commented on code in PR #13559:
URL: https://github.com/apache/maven/pull/13559#discussion_r4315754947
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution
mojoExecution) {}
pred -> assertTrue(plan.step(pred.project,
pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g.
"initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all
its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the
skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
+
+ // The READY step of the plugin project (p2) must NOT be skipped, even
though
+ // "initialize" is before "compile" and most of p2's lifecycle steps
would normally be skipped.
+ assertFalse(
+ plan.requiredStep(p2, "READY").skip.get(),
Review Comment:
❌ **Wrong step name — test will throw `NoSuchElementException`.**
`Lifecycle.Phase.READY` has the value `"ready"` (lowercase). Step names are
keyed by `phase.name()` which returns lowercase phase names. `requiredStep(p2,
"READY")` will throw `NoSuchElementException`, so the assertion never executes
and the test is silently broken.
Fix: use the constant directly.
```suggestion
plan.requiredStep(p2, READY).skip.get(),
```
And add `import static org.apache.maven.api.Lifecycle.Phase.READY;` at the
top of the test file (it's already imported in `BuildPlanExecutor`).
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution
mojoExecution) {}
pred -> assertTrue(plan.step(pred.project,
pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g.
"initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all
its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the
skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
Review Comment:
❌ **Test calls the wrong entry point — the skip-clearing fix is never
exercised.** The reactor plugin skip-clearing logic
(`readyStep.allPredecessors().filter(...).forEach(s -> s.skip.set(false))`)
lives in `buildInitialPlan` (lines 278-294 of `BuildPlanExecutor`), not in
`calculateLifecycleMappings`. This test calls `calculateLifecycleMappings`
directly, which only builds the phase graph and marks steps beyond the end
phase as skipped — it never runs the reactor plugin handling code.
Even with the step name fixed, `plan.requiredStep(p2, READY).skip.get()`
would return `true` (the step IS skipped by `calculateLifecycleMappings` since
`initialize` is before `compile`), and `assertFalse` would fail — not because
the fix is broken, but because the test never called the code that clears the
skip flag.
The test needs to also invoke the reactor plugin handling from
`buildInitialPlan`. The simplest fix is to extract that loop into a
package-private helper on `BuildContext` and call it from the test after
`calculateLifecycleMappings`.
--
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]
