Fixes #13135: ensure completeness of the reactor summary, privilege f… - #13173
Conversation
…lege failures last Backport of the reactor summary improvements to the 3.10.x branch: every module is now always listed (including skipped ones) and failures are displayed last, logged at error level, so they are easier to spot.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The backport is functionally correct — all modules are now always shown in the reactor summary, failures appear last where the terminal cursor rests, and the new tests adequately cover the three grouping scenarios. Two items worth addressing before merge.
Metadata: category bug, target milestone 3.10.0.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| private final StringBuilder buffer; | ||
| private final boolean singleVersion; | ||
|
|
||
| private ReactorSummaryRequest(List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean singleVersion) { |
There was a problem hiding this comment.
🔧 ReactorSummaryRequest carries mutable state — allocate StringBuilder locally instead
ReactorSummaryRequest is a private data-holder class but it carries a StringBuilder that is mutated across all three logReactorSummaryGroup calls (append + setLength(0)). This is a correctness trap: any future refactor that calls logReactorSummaryGroup in a different order, twice for the same group, or in parallel will silently corrupt the buffer with no compiler warning.
The fix is trivial and has no checkstyle impact — allocate the StringBuilder locally at the top of logReactorSummaryGroup and drop it from the class:
| private ReactorSummaryRequest(List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean singleVersion) { | |
| private final List<ReactorSummaryEntry> entries; | |
| private final boolean singleVersion; |
Then update the constructor accordingly, remove getBuffer(), change the call site to:
ReactorSummaryRequest request = new ReactorSummaryRequest(entries, isSingleVersion);And in logReactorSummaryGroup, replace StringBuilder buffer = request.getBuffer(); with:
StringBuilder buffer = new StringBuilder(128);Note: the merged master implementation (#13167) already uses this pattern — the local StringBuilder approach. This 3.10.x backport should match.
| group = 2; | ||
| } else if (buildSummary == null) { | ||
| statusMessage = skippedMessage; | ||
| group = 0; |
There was a problem hiding this comment.
ℹ️ UNKNOWN buildSummary silently grouped with SKIPPED — consider group = 2 for safety
BuildSummary is currently abstract with only BuildSuccess and BuildFailure as concrete subclasses, so this branch is dead code in practice. But if a third subclass (e.g. BuildCancelled) is ever added, the result will land in group 0 alongside SKIPPED modules and be logged at info level, with no visual distinction from a module that was never scheduled to run.
Assigning group = 2 makes this branch safe by default at zero cost:
| group = 0; | |
| statusMessage = unknownMessage; | |
| group = 2; |
Same concern applies to the second else branch at line 191.
- Move StringBuilder from ReactorSummaryRequest into logReactorSummaryGroup as a local variable, eliminating the mutable state from the data-holder. - Assign group=2 to the UNKNOWN BuildSummary branch so any future BuildSummary subclass is treated like a failure rather than silently grouped with SKIPPED modules.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Both findings from the previous review are addressed in the follow-up commit:
StringBuildermoved to local variable —ReactorSummaryRequestis now a clean immutable data-holder;logReactorSummaryGroupallocates its ownStringBuilder, matching the master implementation in #13167.UNKNOWNbranch assignedgroup = 2— any futureBuildSummarysubclass will surface as a failure rather than silently blending with SKIPPED modules.
Logic is correct, tests cover the three grouping scenarios (all-success-with-skipped, failure-with-skipped, mixed), and CI is green on macOS and Ubuntu.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
… StringBuilder) (#13182) Port review findings from #13173 to master: - Move StringBuilder out of ReactorSummaryRequest record and allocate locally in logReactorSummaryGroup, eliminating mutable state from the data-holder. - Assign group=2 to the UNKNOWN BuildSummary branch so any future BuildSummary subclass is logged at error level last, not silently grouped with SKIPPED modules.
… StringBuilder) (#13181) Port review findings from #13173 to maven-4.0.x: - Move StringBuilder out of ReactorSummaryRequest record and allocate locally in logReactorSummaryGroup, eliminating mutable state from the data-holder. - Assign group=2 to the UNKNOWN BuildSummary branch so any future BuildSummary subclass is logged at error level last, not silently grouped with SKIPPED modules.
4 participants
…ailures last
Backport of the reactor summary improvements to the 3.10.x branch: every module is now always listed (including skipped ones) and failures are displayed last, logged at error level, so they are easier to spot.
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.