gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069168835
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutorTest.java:
##########
@@ -156,6 +166,115 @@ void errorIsStillFatalWhenASecondFailureJoinsIt() throws
Exception {
+ session.getResult().getExceptions());
}
+ /**
+ * When a phase is listed in {@code --skip-phases}, no mojo bound to that
phase must appear in the
+ * concurrent build plan after the PLAN step executes. The existing mojos
list on the BuildStep for
+ * that phase must remain empty.
+ */
+ @Test
+ void skippedPhasesMojoIsNotAddedToPlan() throws Exception {
+ MavenProject project = newProject();
+ MavenSession session = newSession(project);
+ session.getRequest().setSkippedPhases(List.of("validate"));
+
+ // Attach a plugin execution bound explicitly to "validate"
+ PluginDescriptor pluginDescriptor = new PluginDescriptor();
+ pluginDescriptor.setGroupId("org.apache.maven.plugins");
+ pluginDescriptor.setArtifactId("maven-skip-test-plugin");
+ pluginDescriptor.setVersion("1.0");
+
+ MojoDescriptor mojoDescriptor = new MojoDescriptor();
+ mojoDescriptor.setGoal("run");
+ mojoDescriptor.setPluginDescriptor(pluginDescriptor);
+
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("org.apache.maven.plugins");
+ plugin.setArtifactId("maven-skip-test-plugin");
+ plugin.setVersion("1.0");
+ PluginExecution execution = new PluginExecution();
+ execution.setId("default-run");
+ execution.setPhase("validate");
+ execution.addGoal("run");
+ plugin.addExecution(execution);
+
+ Build build = new Build();
+ build.addPlugin(plugin);
+ project.setBuild(build);
+
+ MavenPluginManager pluginManager = mock(MavenPluginManager.class);
+ when(pluginManager.getMojoDescriptor(eq(plugin), eq("run"), any(),
any()))
+ .thenReturn(mojoDescriptor);
+
+ ReactorContext reactorContext = newReactorContext(session);
+ newExecutorWithPluginManager(pluginManager, (BeforeProjectExecution)
event -> {})
+ .execute(session, reactorContext, List.of(newTaskSegment()));
+ assertTrue(
+ session.getResult().getExceptions().isEmpty(),
+ "No exceptions expected when a phase is skipped: "
Review Comment:
**Weak assertion — test doesn't verify the mojo is actually skipped.**
`assertTrue(session.getResult().getExceptions().isEmpty())` only confirms
the execution didn't crash. It does NOT assert that the mojo bound to
`validate` was suppressed. A bug that skips the skip-logic entirely would make
this test pass trivially.
The negative assertion is missing: `verify(pluginManager,
never()).getMojoDescriptor(...)`. The sibling test
`emptySkippedPhasesDoesNotFilterMojos` correctly uses
`verify(pluginManager).getMojoDescriptor(...)` to assert the mojo was called —
this test needs the inverse.
```suggestion
verify(pluginManager, never()).getMojoDescriptor(any(), any(),
any(), any());
}
```
(Add the `never()` verify after the `assertTrue` block, and add `import
static org.mockito.Mockito.never;`.)
--
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]
