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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
