gnodet-bot commented on code in PR #13136:
URL: https://github.com/apache/maven/pull/13136#discussion_r4013498697
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -199,31 +200,48 @@ private void logReactorSummary(MavenSession session) {
List<MavenProject> projects = session.getProjects();
- StringBuilder buffer = new StringBuilder(128);
-
String skippedMessage = builder().warning("SKIPPED").build();
String successMessage = builder().success("SUCCESS").build();
String failureMessage = builder().failure("FAILURE").build();
String unknownMessage = builder().warning("UNKNOWN").build();
- boolean lastWasSkipped = false;
+ List<ReactorSummaryEntry> entries = new ArrayList<>(projects.size());
for (MavenProject project : projects) {
BuildSummary buildSummary = result.getBuildSummary(project);
String statusMessage;
- boolean shouldSkip = result.hasExceptions();
- if (buildSummary == null) {
- statusMessage = skippedMessage;
- } else if (buildSummary instanceof BuildSuccess) {
+ int group;
+ if (buildSummary instanceof BuildSuccess) {
statusMessage = successMessage;
+ group = 1;
} else if (buildSummary instanceof BuildFailure) {
statusMessage = failureMessage;
- shouldSkip = false;
+ group = 2;
+ } else if (buildSummary == null) {
+ statusMessage = skippedMessage;
+ group = 0;
} else {
statusMessage = unknownMessage;
+ group = 0;
+ }
+ entries.add(new ReactorSummaryEntry(project, buildSummary, group,
statusMessage));
+ }
+
+ ReactorSummaryRequest request = new ReactorSummaryRequest(entries, new
StringBuilder(128), isSingleVersion);
+
+ logReactorSummaryGroup(request, 0);
+ logReactorSummaryGroup(request, 1);
+ logReactorSummaryGroup(request, 2);
+ }
+
+ private void logReactorSummaryGroup(ReactorSummaryRequest request, int
group) {
+ boolean lastWasSkipped = false;
+ for (ReactorSummaryEntry entry : request.entries()) {
+ if (entry.group() != group) {
+ continue;
}
- if (shouldSkip) {
+ if (group == 0 && entry.buildSummary() == null) {
Review Comment:
⚠️ **Behavioral regression — SKIPPED modules silently dropped on successful
partial builds**
This condition unconditionally filters out all `null`-`buildSummary` entries
regardless of whether the build succeeded or failed. The old code gated this on
`result.hasExceptions()`:
```java
// Old
boolean shouldSkip = result.hasExceptions(); // only true when build failed
```
With the new code, running `mvn -pl moduleA install` (or any `--also-make` /
`--resume-from` subset) on a multi-module project produces a reactor summary
that **completely omits** the modules that were never built. Their
`buildSummary` is `null` and they silently vanish — no `SKIPPED` line, no `...`
prefix, nothing. This is a regression from Maven 3 / pre-#11977 behavior.
Fix: carry `result.hasExceptions()` into `ReactorSummaryRequest` (or pass it
as a parameter) and only suppress `null`-buildSummary entries when there are
exceptions:
```suggestion
if (group == 0 && entry.buildSummary() == null &&
request.hasExceptions()) {
```
…and add `boolean hasExceptions` to `ReactorSummaryRequest`, set from
`result.hasExceptions()` at call site.
##########
compat/maven-embedder/src/test/java/org/apache/maven/cli/event/ExecutionEventLoggerTest.java:
##########
@@ -380,8 +380,8 @@ void testSessionEndedFailureMultimodule() {
inOrder.verify(logger).info("Reactor Summary for Maven Project
artifact1 3.5.4-SNAPSHOT:");
inOrder.verify(logger).info("");
inOrder.verify(logger).info("...");
+ inOrder.verify(logger).info("Maven Project artifact1
............................ SUCCESS [ 1.000 s]");
Review Comment:
📋 **Missing test: successful partial build with some modules not built**
The only test scenarios covering `null`-buildSummary entries are failure
scenarios. There's no test for:
```
mvn -pl moduleA install // moduleB, moduleC → null buildSummary, no
exceptions
```
With the current implementation, those modules silently disappear from the
summary (see the regression comment on `logReactorSummaryGroup`). A test like
`testSessionEndedSuccessWithSkippedModules` would have caught it:
- `project1` → `BuildSuccess`
- `project2` → no build summary (null)
- `project3` → `BuildSuccess`
- No exception added
- Expected: `project1 SUCCESS`, `project2 SKIPPED`, `project3 SUCCESS` (in
build order)
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -232,11 +250,12 @@ private void logReactorSummary(MavenSession session) {
lastWasSkipped = false;
}
- buffer.append(project.getName());
+ StringBuilder buffer = request.buffer();
Review Comment:
🔧 **Minor: `request.buffer()` re-assigned on every iteration — misleading**
The `StringBuilder` is fetched from the record on every loop iteration,
producing a new local variable `buffer` each time that refers to the same
shared object. It reads like a fresh allocation each time but is not — a reader
might wonder why it's inside the loop rather than hoisted before it. Move it
before the loop:
```suggestion
StringBuilder buffer = request.buffer();
```
(i.e., hoist this line to just before the `for` loop, and remove it from
inside the loop body)
--
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]
