You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/09/21 22:07:30 UTC
[PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
gnodet opened a new pull request, #13233:
URL: https://github.com/apache/maven/pull/13233
## Summary
Add `--skip-phases=<phase>[,<phase>...]` CLI option that suppresses all mojo executions bound to the listed lifecycle phases, without removing those phases from the lifecycle DAG.
See also #13232 (fixes the lifecycle DAG: `install`/`deploy` → `after(verify)`).
## Motivation
Resolves part of #13230. Plugin-agnostic replacement for ad-hoc skip properties (`-DskipTests`, `-DskipITs`, etc.) which only work for specific plugins and don't compose well across large multi-module builds.
**Use cases:**
- `mvn verify --skip-phases=test,integration-test` — skip all test execution plugin-agnostically
- `mvn install --skip-phases=test` — install without running tests (e.g. for a dependency snapshot)
- Release builds where CI already validated the commit: `mvn deploy --skip-phases=test,integration-test,verify`
## Semantics
Phase skipping is mojo-level inhibition, not graph surgery. The phases remain in the lifecycle DAG; only their bound mojos are suppressed. Downstream phases (verify, install, deploy) still execute normally.
## Implementation
- `MavenOptions`: `skippedPhases()` method
- `CommonsCliMavenOptions` + `CLIManager`: `--skip-phases` / `-sp` option (comma-separated values)
- `LayeredMavenOptions`: delegation via `collectListIfPresentOrEmpty`
- `MavenInvoker.populateRequest()`: wires option → `MavenExecutionRequest`
- `MavenExecutionRequest` / `DefaultMavenExecutionRequest`: `getSkippedPhases()` / `setSkippedPhases()`
- `BuildPlanExecutor.plan()`: filters mojos in the concurrent builder
- `DefaultLifecycleExecutionPlanCalculator.calculateMojoExecutions()`: filters mojos in the sequential builders (defensively handles null request for test compatibility)
_Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet_
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #13233:
URL: https://github.com/apache/maven/pull/13233#issuecomment-5772265672
**`BuildPlanExecutorTest` — concurrent skip-phases path untested.**
`BuildPlanExecutorTest` was not modified in this PR. The filtering logic in `BuildPlanExecutor.plan()` (the `if (skippedPhases.contains(resolvedPhase)) { continue; }` block added to the `plan()` inner class) is completely untested.
This is the more critical path to test: the concurrent builder is the default in Maven 4 when `-T` is used, and the `plan()` method runs inside a write-lock loop with complex predecessor/step tracking. A test should verify that a mojo bound to a skipped phase is not added to the plan's step list while mojos bound to non-skipped phases are still present.
Please add a test to `BuildPlanExecutorTest` covering at least:
1. A mojo bound to a skipped phase is excluded from the build plan.
2. A mojo bound to a non-skipped phase runs normally.
_This review was generated by an AI agent, Hermès on behalf of @gnodet._
<!-- reviewer: gnodet-bot -->
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069141528
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/CommonsCliMavenOptions.java:
##########
@@ -215,6 +215,14 @@ public Optional<String> atFile() {
return Optional.empty();
}
+ @Override
+ public Optional<List<String>> skippedPhases() {
+ if (commandLine.hasOption(CLIManager.SKIP_PHASES)) {
+ return Optional.of(Arrays.asList(commandLine.getOptionValues(CLIManager.SKIP_PHASES)));
Review Comment:
Fixed in e4a05137cb: using `Arrays.stream(...).map(String::strip).toList()` to strip whitespace from each token.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070907831
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -481,9 +481,9 @@ public Collection<Phase> phases() {
after(TEST_COMPILE),
after(TEST_RESOURCES),
dependencies(SCOPE_TEST, READY))),
- phase(INTEGRATION_TEST)),
- phase(INSTALL, after(PACKAGE)),
- phase(DEPLOY, after(PACKAGE)))));
+ phase(INTEGRATION_TEST, after(BUILD))),
+ phase(INSTALL, after(VERIFY)),
+ phase(DEPLOY, after(VERIFY)))));
Review Comment:
Two separate concerns here — addressing each:
**Concern 1: No IT for DAG topology change.**
The `e2de348d` commit added `MavenITmng13230SkipPhasesTest` but it tests skip-phases behavior, not the DAG topology. A dedicated IT verifying that `mvn install` now implicitly runs the `verify` phase (i.e. a mojo bound to `verify` executes) is still missing. This can be added as a separate test in the same IT class or a new IT.
**Concern 2: #13232 is still open.**
This is a design question for @gnodet: is the DAG fix intentionally bundled into this PR (in which case #13232 should be closed as superseded), or should the DAG changes be kept separate? If separate, the `DefaultLifecycleRegistry` hunk should be reverted from this PR and kept in #13232.
The skip-phases feature itself (the rest of the PR) is solid — only the DAG bundling and the missing DAG IT remain unresolved.
_This review was generated by an AI agent, Hermès on behalf of @gnodet._
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases and --skip-tests CLI options; fix lifecycle DAG [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #13233: Fix #13230: add --skip-phases and --skip-tests CLI options; fix lifecycle DAG
URL: https://github.com/apache/maven/pull/13233
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4068891515
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -619,6 +620,7 @@ private Clock getClock(Object key) {
private void plan() {
lock.writeLock().lock();
try {
+ Set<String> skippedPhases = new HashSet<>(session.getRequest().getSkippedPhases());
Review Comment:
**Missing null guard — inconsistent with the sequential path.**
`DefaultLifecycleExecutionPlanCalculator.calculateMojoExecutions()` added `session.getRequest() != null` before calling `getSkippedPhases()`. The concurrent path here does not. Both callers need the same defensive guard so that test-only usage paths (which call `calculateMojoExecutions` with a stub session where `getRequest()` is null) don't NPE at a different call site.
```suggestion
Set<String> skippedPhases = session.getRequest() != null
? new HashSet<>(session.getRequest().getSkippedPhases())
: Set.of();
```
##########
impl/maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java:
##########
@@ -121,6 +121,11 @@ public interface MavenExecutionRequest {
List<String> getGoals();
+ // Skipped phases
+ MavenExecutionRequest setSkippedPhases(List<String> skippedPhases);
+
+ List<String> getSkippedPhases();
Review Comment:
**Missing Javadoc on the new interface methods.**
Every other section in `MavenExecutionRequest` that adds public methods has at minimum a `@since` tag. The new `setSkippedPhases`/`getSkippedPhases` have only a bare inline comment. At minimum:
```suggestion
/**
* Sets the lifecycle phases whose mojo executions should be suppressed.
*
* @param skippedPhases list of phase names (e.g. {@code "test"}, {@code "integration-test"}),
* or {@code null} to clear
* @return this request
* @since 4.1.0
*/
MavenExecutionRequest setSkippedPhases(List<String> skippedPhases);
/**
* Returns the lifecycle phases whose mojo executions are suppressed.
*
* @return mutable list of phase names; never {@code null}
* @since 4.1.0
*/
List<String> getSkippedPhases();
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069207590
##########
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:
Fixed in 2ce1f45e9f: added `import static org.mockito.Mockito.never` and `verify(pluginManager, never()).getMojoDescriptor(any(), any(), any(), any())` after the assertTrue. Also promoted the qualified `org.mockito.Mockito.verify` in the sibling test to a static import for consistency.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070835027
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenInvoker.java:
##########
@@ -236,6 +236,17 @@ protected void populateRequest(MavenContext context, Lookup lookup, MavenExecuti
request.setNoSnapshotUpdates(context.options().suppressSnapshotUpdates().orElse(false));
request.setGoals(context.options().goals().orElse(List.of()));
+ request.setSkippedPhases(context.options().skippedPhases().orElse(List.of()));
+ if (context.options().skipTests().orElse(false)) {
+ List<String> phases = new ArrayList<>(request.getSkippedPhases());
+ if (!phases.contains("test")) {
+ phases.add("test");
+ }
+ if (!phases.contains("integration-test")) {
+ phases.add("integration-test");
+ }
+ request.setSkippedPhases(phases);
Review Comment:
Fixed in e2de348d3d: added `CommonsCliMavenOptionsTest` covering `--skip-phases` parsing (single value, comma-separated, whitespace stripping, short option, combined with `--skip-tests`) and `MavenITmng13230SkipPhasesTest` with four integration tests:
- `noSkipOptionRunsTestPhase` — baseline: surefire appears when no skip flag is given
- `skipPhasesSupressesMojosForSkippedPhase` — `--skip-phases=test` suppresses surefire
- `skipTestsOptionSuppressesTestPhase` — `--skip-tests` suppresses surefire via phase expansion
- `skipTestsIsIdempotentWhenTestAlreadyInSkipPhases` — combining `--skip-phases=test --skip-tests` doesn't duplicate or error
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070906180
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenInvoker.java:
##########
@@ -236,6 +236,17 @@ protected void populateRequest(MavenContext context, Lookup lookup, MavenExecuti
request.setNoSnapshotUpdates(context.options().suppressSnapshotUpdates().orElse(false));
request.setGoals(context.options().goals().orElse(List.of()));
+ request.setSkippedPhases(context.options().skippedPhases().orElse(List.of()));
+ if (context.options().skipTests().orElse(false)) {
+ List<String> phases = new ArrayList<>(request.getSkippedPhases());
+ if (!phases.contains("test")) {
+ phases.add("test");
+ }
+ if (!phases.contains("integration-test")) {
+ phases.add("integration-test");
+ }
+ request.setSkippedPhases(phases);
Review Comment:
Addressed by `e2de348d`. `CommonsCliMavenOptionsTest` covers all three requested cases: (1) `--skip-tests` alone produces `["test", "integration-test"]` (via `skipTestsOptionSuppressesTestPhase` IT + `skipPhasesAndSkipTestsCanCoexist` unit test), (2) combined with `--skip-phases` doesn't drop entries, (3) idempotency when `test` is already in the list. Thread resolved.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13233:
URL: https://github.com/apache/maven/pull/13233#issuecomment-5772423528
Addressed the two findings from the re-review (commit e4a05137cb):
- **Missing tests for concurrent path**: Added `skippedPhasesMojoIsNotAddedToPlan` and `emptySkippedPhasesDoesNotFilterMojos` to `BuildPlanExecutorTest`, covering both branches of the skip-phases filter in `BuildPlanExecutor.plan()`.
- **Whitespace sensitivity bug**: `CommonsCliMavenOptions.skippedPhases()` now uses `.map(String::strip)` before collecting, so `--skip-phases test, integration-test` strips leading/trailing spaces from each token.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4071674969
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java:
##########
@@ -211,6 +211,9 @@ public List<MojoExecution> calculateMojoExecutions(MavenSession session, MavenPr
MojoNotFoundException, NoPluginFoundForPrefixException, InvalidPluginDescriptorException,
PluginVersionResolutionException, LifecyclePhaseNotFoundException {
final List<MojoExecution> mojoExecutions = new ArrayList<>();
+ final Set<String> skippedPhases = session.getRequest() != null
+ ? new HashSet<>(session.getRequest().getSkippedPhases())
+ : Set.of();
Review Comment:
**Sub-phase expansion is asymmetric — sequential path still does exact-match only.**
Commit `8c5cf895` added `expandSkippedPhases()` to `BuildPlanExecutor.plan()`, so the concurrent path now correctly propagates `--skip-phases=verify` to sub-phases (`test`, `integration-test`, etc.).
But the sequential path here (`DefaultLifecycleExecutionPlanCalculator`) still builds `skippedPhases` as a bare `HashSet` with no expansion. The default Maven 4 build uses the `singlethreaded` builder (set in `MavenInvoker` line 113: `mavenExecutionRequest.setBuilderId("singlethreaded")`), which flows through `SingleThreadedBuilder` → `BuilderCommon` → this class. With no `-T` flag, `--skip-phases=verify` will filter only mojos whose execution phase is literally `"verify"` — it will NOT skip mojos bound to `test` (a child of `verify` in the Maven 4 DAG), even though the user reasonably expects it to.
With `-T2` (multithreaded → `BuildPlanExecutor`), expansion happens and `test` is skipped. The behavior is inconsistent across builders for the same option.
Fix: extract `expandSkippedPhases()` to a shared location (e.g. a static helper in a new utility class or in `MavenExecutionRequest` itself) and call it from both sites:
```suggestion
final Set<String> skippedPhases = session.getRequest() != null
? expandSkippedPhases(new HashSet<>(session.getRequest().getSkippedPhases()))
: Set.of();
```
where `expandSkippedPhases` must also be added to this class (inject `LifecycleRegistry` or pass the lifecycle context needed to walk the DAG). Alternatively, expand once in `MavenInvoker.populateRequest()` before the phases are stored in the request, so both paths see the already-expanded set — that would be simpler and avoids duplicating the expansion logic.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng13230SkipPhasesTest.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration tests for the {@code --skip-phases} and {@code --skip-tests} CLI options
+ * introduced by <a href="https://github.com/apache/maven/issues/13230">GH-13230</a>,
+ * and the lifecycle DAG change that makes {@code install} and {@code deploy} depend on
+ * {@code verify} (so that {@code mvn install} always runs {@code verify}).
+ *
+ * @since 4.1.0
+ */
+class MavenITmng13230SkipPhasesTest extends AbstractMavenIntegrationTestCase {
+
+ /**
+ * Verify that {@code --skip-phases=test} suppresses mojo executions bound to
+ * the {@code test} phase (e.g. maven-surefire-plugin) while still running
+ * the phases leading up to and after it.
+ */
+ @Test
+ void skipPhasesSupressesMojosForSkippedPhase() throws Exception {
Review Comment:
**Typo in method name:** `Supresses` → `Suppresses` (missing second `p`).
```suggestion
void skipPhasesSupressesMojosForSkippedPhase() throws Exception {
```
Wait — the method name itself has the typo. The correct spelling:
```suggestion
void skipPhasesSuppressesMojosForSkippedPhase() throws Exception {
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4071663521
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +707,32 @@ private void plan() {
}
}
+ /**
+ * Expands the set of explicitly skipped phase names to include all their descendant
+ * (child/sub) phases in the lifecycle DAG.
+ *
+ * <p>When a user specifies {@code --skip-phases=verify}, all mojos bound to sub-phases
+ * of {@code verify} (e.g. {@code test}, {@code unit-test}, {@code integration-test}) must
+ * also be skipped, since those sub-phases are part of the skipped phase.</p>
+ *
+ * @param explicit the set of phase names explicitly listed in {@code --skip-phases}
+ * @return a new set containing the original phase names plus all their descendants
+ */
+ private Set<String> expandSkippedPhases(Set<String> explicit) {
Review Comment:
**Behavioral asymmetry: `expandSkippedPhases` only exists in the concurrent path.**
`expandSkippedPhases` ensures that `--skip-phases=verify` also suppresses mojos bound to sub-phases of `verify` (e.g. `test`, `integration-test`). This is the correct semantic — skipping a parent phase should skip everything inside it.
But this expansion only happens here, in `BuildPlanExecutor` (the concurrent builder, used with `-T`). The sequential builder — `DefaultLifecycleExecutionPlanCalculator.calculateMojoExecutions()` — is the default for Maven 4 single-threaded builds (`builderId = "singlethreaded"` in `DefaultMavenExecutionRequest`). Its filter is exact-match:
```java
if (skippedPhases.contains(entry.getKey())) {
continue;
}
```
With that code, `--skip-phases=verify` skips only mojos bound to the literal phase `"verify"`. Surefire, bound to `"test"`, still runs — even though `test` is a child of `verify` in the lifecycle DAG.
The result: `--skip-phases=verify` behaves differently depending on whether the user passes `-T` or not. That is a silent correctness bug for the default (most common) build mode.
**Fix:** extract `expandSkippedPhases` to a shared utility (e.g. a static method in a helper class or on `LifecycleRegistry`) and call it from both `BuildPlanExecutor.BuildContext` and `DefaultLifecycleExecutionPlanCalculator.calculateMojoExecutions()`. Then add a test to `DefaultLifecycleExecutionPlanCalculatorTest` mirroring `skippingParentPhaseAlsoSkipsSubPhases`.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070920266
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -481,9 +481,9 @@ public Collection<Phase> phases() {
after(TEST_COMPILE),
after(TEST_RESOURCES),
dependencies(SCOPE_TEST, READY))),
- phase(INTEGRATION_TEST)),
- phase(INSTALL, after(PACKAGE)),
- phase(DEPLOY, after(PACKAGE)))));
+ phase(INTEGRATION_TEST, after(BUILD))),
+ phase(INSTALL, after(VERIFY)),
+ phase(DEPLOY, after(VERIFY)))));
Review Comment:
Fixed in ea92e8327b: added two ITs to `MavenITmng13230SkipPhasesTest`:
- `installPhaseRunsVerifyWithNewDag` — asserts that `mvn install` now runs surefire (via `verify`), confirming the DAG change.
- `installWithSkipVerifySkipsVerifyMojos` — asserts that `mvn install --skip-phases=verify` still succeeds but suppresses surefire, confirming users can opt out.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070748969
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenInvoker.java:
##########
@@ -236,6 +236,17 @@ protected void populateRequest(MavenContext context, Lookup lookup, MavenExecuti
request.setNoSnapshotUpdates(context.options().suppressSnapshotUpdates().orElse(false));
request.setGoals(context.options().goals().orElse(List.of()));
+ request.setSkippedPhases(context.options().skippedPhases().orElse(List.of()));
+ if (context.options().skipTests().orElse(false)) {
+ List<String> phases = new ArrayList<>(request.getSkippedPhases());
+ if (!phases.contains("test")) {
+ phases.add("test");
+ }
+ if (!phases.contains("integration-test")) {
+ phases.add("integration-test");
+ }
+ request.setSkippedPhases(phases);
Review Comment:
**Missing test for `--skip-tests` expansion logic.**
This block was not in the previously approved commit (`f784a25`). It adds new behavior: when `--skip-tests` is present, `test` and `integration-test` are appended to `skippedPhases` if not already present. There is no unit test that verifies:
1. `--skip-tests` alone produces `skippedPhases = ["test", "integration-test"]`
2. `--skip-tests` combined with `--skip-phases=verify` does not double-add or drop entries
3. `--skip-tests` with `--skip-phases=test` (already in the list) doesn't duplicate `test`
The idempotency check (`!phases.contains("test")`) is correct, but it's exercised by no test. A `CommonsCliMavenOptionsTest` or `MavenInvokerTest` covering this path should be added.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -481,9 +481,9 @@ public Collection<Phase> phases() {
after(TEST_COMPILE),
after(TEST_RESOURCES),
dependencies(SCOPE_TEST, READY))),
- phase(INTEGRATION_TEST)),
- phase(INSTALL, after(PACKAGE)),
- phase(DEPLOY, after(PACKAGE)))));
+ phase(INTEGRATION_TEST, after(BUILD))),
+ phase(INSTALL, after(VERIFY)),
+ phase(DEPLOY, after(VERIFY)))));
Review Comment:
**DAG change bundled in this PR while #13232 is still open — and no integration test.**
This squash includes the lifecycle DAG fix from #13232 (`install`/`deploy` now require `verify`; `integration-test` now requires `BUILD`). That is a **behavioral breaking change**: users running `mvn install` who previously skipped `verify` (because `install` only required `package`) will now always run `verify`.
Two issues:
1. **No integration test** covers this behavioral change. The only tests added are unit-level mocks in `BuildPlanExecutorTest` / `DefaultLifecycleExecutionPlanCalculatorTest` for skip-phases, not for the DAG topology change itself.
2. **13232 is still open.** If 13232 is meant to be a separate PR, having its content squashed here creates a confusing review history. If this is intentional (the two are inseparable), close 13232 and update this PR's description accordingly.
At minimum, an IT (e.g. `MavenITmng13230SkipPhasesTest`) that runs `mvn verify --skip-phases=test` and asserts the `test` phase mojos did not execute, plus a separate IT confirming `mvn install` now runs verify, should be added before merge.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4070954650
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng13230SkipPhasesTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration tests for the {@code --skip-phases} and {@code --skip-tests} CLI options
+ * introduced by <a href="https://github.com/apache/maven/issues/13230">GH-13230</a>,
+ * and the lifecycle DAG change that makes {@code install} and {@code deploy} depend on
+ * {@code verify} (so that {@code mvn install} always runs {@code verify}).
+ *
+ * @since 4.1.0
+ */
+class MavenITmng13230SkipPhasesTest extends AbstractMavenIntegrationTestCase {
+
+ /**
+ * Verify that {@code --skip-phases=test} suppresses mojo executions bound to
+ * the {@code test} phase (e.g. maven-surefire-plugin) while still running
+ * the phases leading up to and after it.
+ */
+ @Test
+ void skipPhasesSupressesMojosForSkippedPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-skip-test.txt");
+ verifier.addCliArguments("verify", "--skip-phases", "test");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // The test phase was skipped — surefire must not appear in the log
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code --skip-tests} (short: {@code -st}) expands to skipping
+ * both the {@code test} and {@code integration-test} phases, so surefire
+ * does not run.
+ */
+ @Test
+ void skipTestsOptionSuppressesTestPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-skip-tests.txt");
+ verifier.addCliArguments("verify", "--skip-tests");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // Both test and integration-test phases are skipped via --skip-tests
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that without any skip option the {@code test} phase executes normally
+ * and surefire appears in the build log.
+ */
+ @Test
+ void noSkipOptionRunsTestPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-no-skip.txt");
+ verifier.addCliArgument("verify");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // Surefire is bound to the test phase and must execute
+ verifier.verifyTextInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code --skip-phases=test} combined with existing phases still
+ * works when {@code --skip-phases} already contains {@code test} — idempotency
+ * of the phase list must not cause issues.
+ */
+ @Test
+ void skipTestsIsIdempotentWhenTestAlreadyInSkipPhases() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-idempotent.txt");
+ // --skip-phases already lists test; --skip-tests must not duplicate it
+ verifier.addCliArguments("verify", "--skip-phases", "test", "--skip-tests");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify the lifecycle DAG change: {@code install} now depends on {@code verify},
+ * so {@code mvn install} must run the {@code verify} phase (and thus surefire).
+ * Prior to this change, {@code install} only required {@code package}, so tests
+ * were never run by {@code mvn install}.
+ */
+ @Test
+ void installPhaseRunsVerifyWithNewDag() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-install-runs-verify.txt");
+ verifier.addCliArgument("install");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // install → after(verify) → verify includes test, so surefire must execute
+ verifier.verifyTextInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code mvn install --skip-phases=verify} skips the verify mojos
+ * (surefire does not run) while {@code install} itself still succeeds.
+ * This confirms the DAG change does not lock users out of skipping verify.
+ */
+ @Test
+ void installWithSkipVerifySkipsVerifyMojos() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-install-skip-verify.txt");
+ verifier.addCliArguments("install", "--skip-phases", "verify");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
Review Comment:
**Wrong assertion — `verify` skip does not suppress surefire.**
`--skip-phases=verify` suppresses mojos bound to the `verify` phase (e.g. maven-failsafe-plugin:verify, maven-checkstyle-plugin:check). It does **not** skip mojos bound to the `test` phase. Surefire (`maven-surefire-plugin:test`) is bound to `test`, which runs before `verify` in the lifecycle order and is not in the skipped list.
The comment even says "surefire (bound to test, inside verify)" — surefire is not inside verify. It is bound to `test`, a distinct phase that executes earlier. The assertion `verifyTextNotInLog("maven-surefire-plugin")` will fail at runtime.
Fix: the test should skip `test,verify` to actually suppress surefire, or it should assert that surefire DID run (it's not affected by `--skip-phases=verify`) and verify that a verify-phase mojo did not.
The simplest correct version that keeps the test's intent ("users can still skip test execution even with the new DAG"):
```suggestion
verifier.addCliArguments("install", "--skip-phases", "test,verify");
verifier.execute();
verifier.verifyErrorFreeLog();
// test and verify mojos are skipped — surefire must not run
verifier.verifyTextNotInLog("maven-surefire-plugin");
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13233:
URL: https://github.com/apache/maven/pull/13233#issuecomment-5772614394
Fixed in f784a25567: moved the skipped-phases check before `getMojoDescriptor()` in the concurrent `plan()` method. When a phase is explicitly declared on the `<execution>`, we can determine whether it is skipped without loading the plugin descriptor — this makes the `verify(pluginManager, never()).getMojoDescriptor(...)` assertion in `BuildPlanExecutorTest.skippedPhasesMojoIsNotAddedToPlan()` actually pass (the test was added in 2ce1f45e9f but the implementation hadn't been updated to match the stricter assertion).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases and --skip-tests CLI options; fix lifecycle DAG [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13233:
URL: https://github.com/apache/maven/pull/13233#issuecomment-5793297360
Closing as superseded by #13249, which covers the same feature set with a broader scope.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #13233:
URL: https://github.com/apache/maven/pull/13233#issuecomment-5771996894
**Missing tests (blocking)**
The feature touches two independent code paths — `DefaultLifecycleExecutionPlanCalculator` (sequential) and `BuildPlanExecutor` (concurrent) — and neither has a test for the new skip logic. `DefaultLifecycleExecutionPlanCalculatorTest` and `BuildPlanExecutorTest` were not modified.
At minimum needed:
1. A unit test in `DefaultLifecycleExecutionPlanCalculatorTest` verifying that mojos bound to a skipped phase are excluded from the returned list while mojos bound to other phases are not.
2. A unit test in `BuildPlanExecutorTest` (or a new `SkipPhasesTest`) verifying the concurrent path behaves identically.
3. An integration test (IT) under `its/core-it-suite/` covering an end-to-end scenario: `mvn verify --skip-phases=test` where a test plugin is bound to the `test` phase confirms it does not run, but `verify`-bound mojos do.
Without tests, the next rebase that changes the phase resolution order or the `plan()` inner class structure will silently break this feature.
_This review was generated by an AI agent, Hermès on behalf of @gnodet._
<!-- reviewer: gnodet-bot -->
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069018924
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -619,6 +620,7 @@ private Clock getClock(Object key) {
private void plan() {
lock.writeLock().lock();
try {
+ Set<String> skippedPhases = new HashSet<>(session.getRequest().getSkippedPhases());
Review Comment:
Fixed in a494d37554. Added the same null guard as the sequential path: `session.getRequest() != null ? new HashSet<>(...) : Set.of()`.
##########
impl/maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java:
##########
@@ -121,6 +121,11 @@ public interface MavenExecutionRequest {
List<String> getGoals();
+ // Skipped phases
+ MavenExecutionRequest setSkippedPhases(List<String> skippedPhases);
+
+ List<String> getSkippedPhases();
Review Comment:
Fixed in a494d37554. Full Javadoc with `@param`, `@return`, and `@since 4.1.0` added to both methods.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4069049959
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/CommonsCliMavenOptions.java:
##########
@@ -215,6 +215,14 @@ public Optional<String> atFile() {
return Optional.empty();
}
+ @Override
+ public Optional<List<String>> skippedPhases() {
+ if (commandLine.hasOption(CLIManager.SKIP_PHASES)) {
+ return Optional.of(Arrays.asList(commandLine.getOptionValues(CLIManager.SKIP_PHASES)));
Review Comment:
**Whitespace sensitivity bug — silent skip failure.**
`commandLine.getOptionValues()` returns the raw tokens as split by Commons CLI. With `valueSeparator(',')` + `hasArgs()`, the CLI splits on commas — but does NOT strip whitespace. A user who writes:
```
mvn verify --skip-phases test, integration-test
```
gets `["test", " integration-test"]` (note the leading space). The `skippedPhases.contains("integration-test")` check in both `DefaultLifecycleExecutionPlanCalculator` and `BuildPlanExecutor` then silently fails to skip `integration-test` — no error, no warning, mojos run anyway.
Fix: strip whitespace at the source:
```suggestion
return Optional.of(Arrays.stream(commandLine.getOptionValues(CLIManager.SKIP_PHASES))
.map(String::strip)
.collect(java.util.stream.Collectors.toList()));
```
Alternatively, add a warning in the option description (as `activatedProfiles` does: _"Don't use spaces between commas"_), but stripping is strictly safer since it handles the common user mistake silently and correctly.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4071621290
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng13230SkipPhasesTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration tests for the {@code --skip-phases} and {@code --skip-tests} CLI options
+ * introduced by <a href="https://github.com/apache/maven/issues/13230">GH-13230</a>,
+ * and the lifecycle DAG change that makes {@code install} and {@code deploy} depend on
+ * {@code verify} (so that {@code mvn install} always runs {@code verify}).
+ *
+ * @since 4.1.0
+ */
+class MavenITmng13230SkipPhasesTest extends AbstractMavenIntegrationTestCase {
+
+ /**
+ * Verify that {@code --skip-phases=test} suppresses mojo executions bound to
+ * the {@code test} phase (e.g. maven-surefire-plugin) while still running
+ * the phases leading up to and after it.
+ */
+ @Test
+ void skipPhasesSupressesMojosForSkippedPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-skip-test.txt");
+ verifier.addCliArguments("verify", "--skip-phases", "test");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // The test phase was skipped — surefire must not appear in the log
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code --skip-tests} (short: {@code -st}) expands to skipping
+ * both the {@code test} and {@code integration-test} phases, so surefire
+ * does not run.
+ */
+ @Test
+ void skipTestsOptionSuppressesTestPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-skip-tests.txt");
+ verifier.addCliArguments("verify", "--skip-tests");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // Both test and integration-test phases are skipped via --skip-tests
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that without any skip option the {@code test} phase executes normally
+ * and surefire appears in the build log.
+ */
+ @Test
+ void noSkipOptionRunsTestPhase() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-no-skip.txt");
+ verifier.addCliArgument("verify");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // Surefire is bound to the test phase and must execute
+ verifier.verifyTextInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code --skip-phases=test} combined with existing phases still
+ * works when {@code --skip-phases} already contains {@code test} — idempotency
+ * of the phase list must not cause issues.
+ */
+ @Test
+ void skipTestsIsIdempotentWhenTestAlreadyInSkipPhases() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-idempotent.txt");
+ // --skip-phases already lists test; --skip-tests must not duplicate it
+ verifier.addCliArguments("verify", "--skip-phases", "test", "--skip-tests");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ verifier.verifyTextNotInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify the lifecycle DAG change: {@code install} now depends on {@code verify},
+ * so {@code mvn install} must run the {@code verify} phase (and thus surefire).
+ * Prior to this change, {@code install} only required {@code package}, so tests
+ * were never run by {@code mvn install}.
+ */
+ @Test
+ void installPhaseRunsVerifyWithNewDag() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-install-runs-verify.txt");
+ verifier.addCliArgument("install");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ // install → after(verify) → verify includes test, so surefire must execute
+ verifier.verifyTextInLog("maven-surefire-plugin");
+ }
+
+ /**
+ * Verify that {@code mvn install --skip-phases=verify} skips the verify mojos
+ * (surefire does not run) while {@code install} itself still succeeds.
+ * This confirms the DAG change does not lock users out of skipping verify.
+ */
+ @Test
+ void installWithSkipVerifySkipsVerifyMojos() throws Exception {
+ Path basedir = extractResources("mng-13230");
+
+ Verifier verifier = newVerifier(basedir);
+ verifier.setLogFileName("log-install-skip-verify.txt");
+ verifier.addCliArguments("install", "--skip-phases", "verify");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
Review Comment:
Fixed in 46b7d894ef2bd3ae70bc2e2e413f56626e898928. Corrected to `--skip-phases=test,verify` — surefire is bound to `test`, not `verify`, so both phases need to be skipped to suppress it.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13230: add --skip-phases CLI option to skip mojo executions by phase [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13233:
URL: https://github.com/apache/maven/pull/13233#discussion_r4071797520
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +707,32 @@ private void plan() {
}
}
+ /**
+ * Expands the set of explicitly skipped phase names to include all their descendant
+ * (child/sub) phases in the lifecycle DAG.
+ *
+ * <p>When a user specifies {@code --skip-phases=verify}, all mojos bound to sub-phases
+ * of {@code verify} (e.g. {@code test}, {@code unit-test}, {@code integration-test}) must
+ * also be skipped, since those sub-phases are part of the skipped phase.</p>
+ *
+ * @param explicit the set of phase names explicitly listed in {@code --skip-phases}
+ * @return a new set containing the original phase names plus all their descendants
+ */
+ private Set<String> expandSkippedPhases(Set<String> explicit) {
Review Comment:
Fixed in 72b8d24c579a90885710aab3c4ba70c906211a1f: injected `LifecycleRegistry` into `DefaultLifecycleExecutionPlanCalculator` and added `expandSkippedPhases()` — same logic as `BuildPlanExecutor`. Both builders now expand `--skip-phases=verify` to include all descendant sub-phases. Unit test `skipParentPhaseAlsoFiltersSubPhases` added to pin the sequential path.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java:
##########
@@ -211,6 +211,9 @@ public List<MojoExecution> calculateMojoExecutions(MavenSession session, MavenPr
MojoNotFoundException, NoPluginFoundForPrefixException, InvalidPluginDescriptorException,
PluginVersionResolutionException, LifecyclePhaseNotFoundException {
final List<MojoExecution> mojoExecutions = new ArrayList<>();
+ final Set<String> skippedPhases = session.getRequest() != null
+ ? new HashSet<>(session.getRequest().getSkippedPhases())
+ : Set.of();
Review Comment:
Fixed in 72b8d24c579a90885710aab3c4ba70c906211a1f. `DefaultLifecycleExecutionPlanCalculator` now has its own `expandSkippedPhases()` backed by the injected `LifecycleRegistry`. The exact-match filter is gone — both builders use the same expansion semantics.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng13230SkipPhasesTest.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration tests for the {@code --skip-phases} and {@code --skip-tests} CLI options
+ * introduced by <a href="https://github.com/apache/maven/issues/13230">GH-13230</a>,
+ * and the lifecycle DAG change that makes {@code install} and {@code deploy} depend on
+ * {@code verify} (so that {@code mvn install} always runs {@code verify}).
+ *
+ * @since 4.1.0
+ */
+class MavenITmng13230SkipPhasesTest extends AbstractMavenIntegrationTestCase {
+
+ /**
+ * Verify that {@code --skip-phases=test} suppresses mojo executions bound to
+ * the {@code test} phase (e.g. maven-surefire-plugin) while still running
+ * the phases leading up to and after it.
+ */
+ @Test
+ void skipPhasesSupressesMojosForSkippedPhase() throws Exception {
Review Comment:
Fixed in 72b8d24c579a90885710aab3c4ba70c906211a1f: renamed to `skipPhasesSuppressesMojosForSkippedPhase` (double 'p').
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org