Stream live test-progress from forked test JVMs to the mvnd client (mvnd-1.x)

[ammachado]

Important

This PR is stacked on top of #1666 (feature/worker-line-display-1x). It should be reviewed and merged after #1666. Until #1666 merges, the diff here includes its commits as ancestors; the base will effectively rebase onto mvnd-1.x once #1666 lands.

What this adds

The test-progress feed for mvnd 1.x: a bridge that streams live JUnit/Surefire test progress from forked test JVMs back to the daemon and out to the client. This is the data source that PR #1666's worker-line suffix renders.

Highlights

• -Dmvnd.testProgress flag (Environment) to opt into the feature, wired into bash completion.
• mvnd-surefire-progress module providing a custom Surefire ForkNodeFactory (MvndForkNodeFactory) plus a TestProgressAccumulator that tallies per-fork test events.
• Daemon-side wiring: MvndTestProgressLifecycleParticipant injects the fork-node factory into the Surefire mojo configuration, and the daemon dispatches accumulated progress to the client (ClientDispatcher, Server).
• Plugin-realm fix: InvalidatingPluginRealmCache loads MvndForkNodeFactory in the Surefire plugin realm so the class resolves at fork time.
• Integration test (TestProgressTest) driving a sample project end to end.

Files

21 files changed, ~1000 insertions across the common module, daemon, surefire-progress module, distribution, and integration tests.

🤖 Generated with Claude Code

[gnodet]

Review: Test Progress Reporting Feature

Well-designed feature with sound architecture. The maker/checker review process confirmed several items worth the author's attention.

Medium Severity

1. supportsForkNode version check logic gap
The version check in supportsForkNode applies the M5 milestone threshold to ALL 3.x versions, not just 3.0.0. For example, "3.1.0-M2" produces group(4)="2" and returns false, even though the milestone guard should only apply to the 3.0.0-Mx series. Practical impact is negligible since Surefire never published milestone versions beyond 3.0.0-Mx (3.1.0+, 3.2.x, 3.5.x all shipped as GA), but the fix (apply milestone check only when minor==0 && patch==0) is trivial and improves defensive correctness.

2. clearDisplay() no longer called on build failure
The base branch BUILD_FINISHED handler calls clearDisplay() unconditionally. This PR wraps it in if (failures.isEmpty()), so on failure the JLine display's last render (project status lines) remains painted in the terminal. The shouldShowProjectDetails() method and its comment suggest this is intentional (keep failure info visible), but this is a behavior change from prior code — please confirm this is the desired UX.

Low Severity

3. Mutable ArrayList references in getter methods
ProjectTestProgressEvent.getFlakyTests(), getFailedTests(), and getErroredTests() return the mutable internal lists directly. The constructor makes defensive copies, but the getters expose the internal state. Currently no callers mutate the returned lists, but wrapping in Collections.unmodifiableList() would be a safe defensive measure.

4. BANNED_MARKER fragile string matching
The string "This project has been banned from the build due to previous failures." is compared via equals() after stripping ANSI/level prefixes. If Maven changes this message text, the filter silently breaks. This is inherent to text-based output matching (no programmatic API alternative), but worth noting as a maintenance risk.

5. Pattern.compile on each invocation of supportsForkNode
The regex is compiled inline each call. Called once per surefire/failsafe plugin per project, so allocation count is small, but the pattern could be a static final field at no cost.

6. Duplicated isEnabled logic
Both Server.handle() and MvndTestProgressLifecycleParticipant.isEnabled() contain identical logic: Environment.MVND_TEST_PROGRESS.asOptional().map(Boolean::parseBoolean).orElse(Boolean.TRUE). Could be extracted to a shared static method.

7. Magic indices in int[] snapshot array
The TestBuildSummary snapshot array is packed as {completed, failures, errors, skipped, retrying, flaky} and unpacked with raw indices 0–5 (notably skipping index 4/retrying intentionally). A record or named-constant approach would make the skip of "retrying" explicit and prevent index-drift bugs.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[ammachado]

Thanks for the thorough review! Addressed all 7 points in acbb147:

Medium

1. supportsForkNode version gap — now only applies the milestone guard when minor == 0 && patch == 0, so 3.1.0-M2/3.2.5-M1 etc. are correctly treated as GA. Added a regression test for this.
2. clearDisplay() on failure — reverted to unconditional clearDisplay() at BUILD_FINISHED; the guarded version wasn't intentional.

Low
3. Mutable list getters — getFlakyTests()/getFailedTests()/getErroredTests() now return Collections.unmodifiableList(...).
4. BANNED_MARKER fragile match — agreed there's no programmatic alternative; added a comment documenting it as a maintenance risk so it's not silently missed if Maven changes the message.
5. Pattern.compile per call — hoisted to a static final Pattern VERSION_PATTERN.
6. Duplicated isEnabled logic — consolidated into MvndTestProgressLifecycleParticipant.isTestProgressEnabled(), called from both Server.handle() and the lifecycle participant.
7. Magic indices in the int[] snapshot — replaced with named IDX_* constants (records aren't available at this module's Java 8 target).

[gnodet]

Re-review: All Previous Findings Resolved

The fix commit (acbb147) correctly addresses all 7 findings from the previous review:

1. supportsForkNode version check — milestone guard now only applies to the 3.0.0-Mx series (checks minor/patch before applying). New test milestoneGuardOnlyAppliesToThe300MilestoneSeries covers "3.1.0-M2" and "3.2.5-M1".
2. clearDisplay() — again called unconditionally in the BUILD_FINISHED handler.
3. Mutable list getters — getFlakyTests/getFailedTests/getErroredTests now return Collections.unmodifiableList wrappers.
4. BANNED_MARKER — field now has a maintenance-risk comment documenting the fragility.
5. Pattern.compile — VERSION_PATTERN is now a static final field.
6. Duplicated isEnabled — Server.handle() delegates to MvndTestProgressLifecycleParticipant.isTestProgressEnabled() instead of duplicating the logic.
7. Magic array indices — IDX_COMPLETED/IDX_FAILURES/IDX_ERRORS/IDX_SKIPPED/IDX_FLAKY constants replace magic indices, with IDX_RETRYING=4 explicitly documented as not folded.

The fix commit is well-scoped — touches only the files relevant to the review feedback with no unrelated changes. Nice work!

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Re-review: New Commit (3eedb48)

The new commit adds a defensive clamp to renderBar(int percent) — Math.max(0, Math.min(100, percent)) — ensuring out-of-range inputs can't under/overfill the progress bar. A corresponding test (renderBarClampsOutOfRangeInput) covers both negative and >100 values.

Trivially correct, well-scoped change. LGTM.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Well-engineered feature adding live test-progress streaming from Surefire/Failsafe forks to the mvnd client. The classloading bridge design, thread safety (AtomicReference for cross-thread listener bridge, per-fork-channel accumulators), and Surefire version guarding are all sound. Test coverage is comprehensive.

Two items worth addressing before merge:

1. BannedSkipFilter not flushed on CANCEL_BUILD / BUILD_EXCEPTION (medium)

The BUILD_FINISHED handler (line 319) correctly calls bannedSkipFilter.flush(log::accept) before closing the log. However, the CANCEL_BUILD handler (line 258) and BUILD_EXCEPTION handler (line 272) skip this flush. Any reactor-level lines buffered in the filter (blank lines, separators, "Skipping X" preambles) will be silently dropped during cancelled or failed builds.

Suggested fix — add the flush before projects.values().stream()... in both handlers:

if (hideBannedProjectSkips) {
    bannedSkipFilter.flush(log::accept);
}

2. Stale TODO comment on PROJECT_TEST_PROGRESS (medium)

The comment at Message.java line 69-73 says "the daemon-side feed that emits this message is not implemented on mvnd-1.x yet", but this PR fully implements it across 12 commits (dispatch, bridge listener, forkNode injection, etc.). The TODO should be replaced with documentation describing the implemented feature.

Minor note

• TestState.displayName() null safety (low): Returns null when both testClass and testMethod are null, which would cause NullPointerException in flakyDetail() via new StringBuilder(displayName()). Practically unreachable since Surefire always provides source names, but a defensive "(unknown)" fallback would be more robust.

Note: This PR is stacked on PR #1666 — some findings (like BannedSkipFilter) may have been introduced in the base PR.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Re-review after commit b574e61 — the ported fixes (message ordering, serialization cleanup, plugin version restore) are correct. However, 3 findings from the previous review remain open:

1. BannedSkipFilter not flushed on CANCEL_BUILD / BUILD_EXCEPTION (medium)
TerminalOutput.java — BUILD_FINISHED (line 318) correctly calls bannedSkipFilter.flush(log::accept), but CANCEL_BUILD (line 256) and BUILD_EXCEPTION (line 269) close the log without flushing the filter first. Buffered cosmetic lines (blank lines, separators, "Skipping X" preambles) would be silently dropped during cancelled or failed builds.

2. Stale TODO on PROJECT_TEST_PROGRESS (medium)
Message.java line 71 says "TODO: the daemon-side feed … is not implemented on mvnd-1.x yet" but this PR fully implements it. Should be replaced with documentation describing the feature.

3. TestState.displayName() null safety (low)
TestProgressAccumulator.java line 289 — when both testClass and testMethod are null, displayName() returns null → flakyDetail() calls new StringBuilder(null) → NPE. Practically unreachable but a defensive "(unknown)" fallback would be more robust.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @gnodet

[ammachado]

Thanks for the re-review! Addressed all 3 remaining findings in 9822b4c:

1. BannedSkipFilter now flushes on CANCEL_BUILD and BUILD_EXCEPTION, matching the existing BUILD_FINISHED behavior — no more silently dropped cosmetic reactor lines on cancelled/failed builds.
2. Replaced the stale PROJECT_TEST_PROGRESS TODO in Message.java with a description of the implemented feature.
3. TestState.displayName() now falls back to "(unknown)" instead of returning null when both testClass and testMethod are absent, so flakyDetail() can't NPE.

[gnodet]

Re-review after commit 9822b4c: all 3 remaining findings are now addressed. ✅

1. BannedSkipFilter flush — CANCEL_BUILD and BUILD_EXCEPTION handlers now call bannedSkipFilter.flush(log::accept) guarded by hideBannedProjectSkips, matching the existing BUILD_FINISHED pattern. No more silently dropped cosmetic lines on cancelled/failed builds.

2. Stale TODO replaced — The Message.java TODO now accurately describes the implemented data flow (forked JVM → daemon → client display).

3. displayName() null safety — Returns "(unknown)" when both testClass and testMethod are null, preventing NPE in flakyDetail().

All changes are minimal and consistent with existing patterns. No new issues introduced.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @gnodet