gnodet-bot commented on code in PR #13559:
URL: https://github.com/apache/maven/pull/13559#discussion_r4315710256
##########
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: `"READY"` (uppercase) does not exist** — the lifecycle
phase is named `"ready"` (lowercase, from `Lifecycle.Phase.READY = "ready"`).
`plan.requiredStep(p2, "READY")` will throw `NoSuchElementException` at
runtime, so this test never reaches its assertions.
Use the constant or lowercase string:
```suggestion
plan.requiredStep(p2, Lifecycle.Phase.READY).skip.get(),
```
(Add `import static org.apache.maven.api.Lifecycle.Phase.READY;` or use the
literal `"ready"`.
Same fix needed on line 509: `plan.requiredStep(p2, Lifecycle.Phase.READY)`.
##########
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 method — the skip-clearing fix is unreachable from
`calculateLifecycleMappings`.** The fix that clears `skip` on reactor plugin
predecessors lives in `BuildPlanExecutor.buildInitialPlan()` (lines 278–290 of
the executor). The test helper at line 227–233 constructs a `BuildContext` and
calls `context.calculateLifecycleMappings(projects, phase)` directly, which
**never executes the reactor-plugin detection block**. Even after fixing the
`"READY"` / `"ready"` string, the assertion will fail because
`calculateLifecycleMappings` marks all phases after `initialize` as skipped and
the fix never runs to un-skip them.
The test must exercise `buildInitialPlan`. Two options:
1. **Preferred**: Refactor `testReactorPluginPreCompilePhase` as a
`BuildContext.calculateLifecycleMappings` test that directly validates the
**skip behaviour without the fix** (i.e. assert that READY is skipped —
baseline), then add a separate test that exercises `buildInitialPlan` via a
mocked `MavenSession` to verify the skip-clearing.
2. **Simpler**: Add a test that instantiates a `BuildContext`, builds the
plan via `calculateLifecycleMappings`, then manually simulates what
`buildInitialPlan` does — apply the reactor-plugin detection logic and assert
the result. But this duplicates production logic in test code.
Until this is fixed, `testReactorPluginPreCompilePhase` does not protect the
fix from regression.
--
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]
