Stream live test-progress from forked test JVMs to the mvnd client (2.x) - #1713
Stream live test-progress from forked test JVMs to the mvnd client (2.x)#1713ammachado wants to merge 12 commits into
Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-structured PR with a thoughtful design — the bridge class pattern for cross-classloader communication via MvndTestProgress is the right approach for Maven's ClassWorlds model, the BannedSkipFilter state machine is clean, and the defensive error handling ensures the test-progress feature never breaks actual test execution. Good test coverage across unit tests and integration tests.
One minor omission noted below.
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
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Previous finding (missing mvnd.hideBannedProjectSkips from bash completion) addressed in commit 3b298b6 — property now correctly inserted in alphabetical order. No new issues introduced. 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
de24daf to
e3d5f94
Compare
|
CI investigation:
AI-assisted CI investigation. |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
New commits since last approval look good:
caae0239: bash-completion fix (addsmvnd.hideBannedProjectSkipsas previously requested)e3d5f945: routing fix — switches test-summary delivery from SLF4J toClientDispatcherso summaries actually reach the client. The test simplification for the new routing path is appropriate.
Both are clean, low-risk changes. 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
5689a3c to
6989a8f
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Branch rebased — diff delta is small (~34 added, 4 deleted, 3 new files vs. last approval), consistent with minor cleanup rather than material changes. Full diff re-reviewed: no new issues. All previously approved functionality intact — live test progress, flaky/failure reporting, banned-skip filtering, test summary routing, comprehensive test coverage. 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
6989a8f to
554f0cb
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Latest commits simplify TestSummaryExecutionListener by routing test summary directly through clientDispatcher.log() instead of SLF4J — net -22 lines, strictly simplifying. Bash completion entry correctly placed. Test assertion simplified to relative-order check. 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
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Great feature PR — the four-layer architecture (surefire-progress module → MvndTestProgress bridge → ClientDispatcher/Server → TerminalOutput) is clean and well-motivated. Thread safety, error handling, and test coverage are all solid.
One confirmed bug and one design observation after independent verification:
1. ANSI regex missing ESC character (medium)
The stripDecoration() regex Pattern.compile("\\[[0-9;]*m") matches [0-9;]*m but omits the ESC character (\u001B) that prefixes all real SGR sequences. When the daemon has color enabled, Maven produces sequences like \033[1m, and the regex matches [1m but leaves orphan \033 characters. This breaks the BANNED_MARKER equality check (stripped text contains invisible ESC chars that the constant doesn't). The unit test masks this by using bare [1m without ESC characters.
Fix: Pattern.compile("\u001B\\[[0-9;]*m") and update the test to use "\u001B[1m" sequences.
The feature degrades gracefully (banned messages are shown rather than hidden), so this is not a blocker.
2. Test summary severity discarded (low)
emitTestSummary() calls clientDispatcher.log(line.text) but discards the SummaryLevel computed by renderLines(). All summary lines arrive as plain BUILD_LOG_MESSAGE, losing the red/yellow/default visual distinction. This would require a protocol extension to fix, so more of a future enhancement.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! The ANSI regex issue from the previous review is fully resolved:
- ✅ ANSI regex fixed (commit 1b13e3d) — pattern is now
Pattern.compile("\u001b\\[[0-9;]*m")which properly anchors on the ESC byte - ✅ Unit tests updated — verify both colored text stripping and the critical colored
BANNED_MARKERcase, plus negative case (bare[1mwithout ESC is preserved)
The low-severity observation about emitTestSummary() discarding SummaryLevel remains, but that's a protocol-level limitation requiring a new message type — fine as a future enhancement.
Overall this is a well-architected feature with solid test coverage. Looks good to merge.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
The mvnd.testProgress flag (default on) gates the feature; MvndTestProgress is the shared bridge whose Class is exported to the Maven core realm so the daemon and the surefire plugin realm resolve one listener registry. Branch-identical with mvnd-1.x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
New module carrying the pure per-fork accumulator, the surefire ForkNodeFactory (MvndForkNodeFactory) that decorates the event handler to observe per-test events, and a jar locator. Kept in package org.mvndaemon.mvnd.forknode, OUT of the exported testprogress bridge prefix, so the surefire plugin realm loads it from its own class path (ClassWorlds exports are prefix-based). Branch-identical with mvnd-1.x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ClientDispatcher.testProgress enqueues a PROJECT_TEST_PROGRESS message; Server registers a MvndTestProgress listener that forwards to it around each build (guarded by mvnd.testProgress) and clears it in finally. Branch-identical with mvnd-1.x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Maven 4 wiring for the test-progress feed: - DaemonPlexusContainerCapsuleFactory: export the testprogress bridge package to the core realm so the surefire plugin realm shares one MvndTestProgress Class - MvndMojoExecutionConfigurator: overrides the default Maven 4 configurator to inject <forkNode implementation=MvndForkNodeFactory><projectId> into surefire test / failsafe integration-test executions (model mutation in afterProjectsRead is ignored under Maven 4's immutable model). Guarded on Surefire >= 3.0.0-M5. The Sisu wiring is load-bearing: @nAmed("default") + explicit implements MojoExecutionConfigurator + no-arg ctor + @priority(10) are all required for it to win the Map<String,MojoExecutionConfigurator> "default" key (see the class javadoc) - InvalidatingPluginRealmCache: addURL the mvnd-surefire-progress jar onto the surefire/failsafe plugin realm so it can load MvndForkNodeFactory - dist bundles the jar; bash completion adds -Dmvnd.testProgress - integration-tests: TestProgressTest (emission + opt-out) validates end-to-end against a real daemon Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extend the live test-progress feed beyond basic counts so a failing build
tells you which tests broke and why, directly in mvnd's output.
- Accumulate retrying/flaky state and capture FAILED/ERRORED test identities
plus a compact failure message (Surefire smartTrimmedStackTrace, sanitized),
streamed alongside flaky names in ProjectTestProgressEvent.
- Render a live "Flaky tests:" summary at build end, and inject a
"Failed tests:" / "Errored tests:" block ("<projectId> Class#method: message")
directly above Maven's BUILD FAILURE banner, with a build-finished fallback.
- Add mvnd.hideBannedProjectSkips (default true) to drop the per-project
"Skipping X / banned from the build" reactor blocks while preserving the
final reactor "... SKIPPED" rows; disable with =false.
- Cover with accumulator, message round-trip, and TerminalOutput unit tests
(ordering + banned-block filter), plus a new TestProgressFailureTest IT.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ging Move end-of-build test summary rendering from the client (string injection keyed on a "BUILD FAILURE" text match) to the daemon, where LoggingExecutionListener logs it through its own SLF4J logger just before the Reactor Summary. This routes the summary through the same MvndSimpleLogger pipeline as every other Maven console line, so ANSI coloring, [LEVEL] prefixes, and -q gating all come for free instead of being reimplemented client-side. ClientDispatcher now collects failed/errored/flaky test identities and per-fork numeric totals into a new TestBuildSummary as testProgress() events arrive; BuildEventListener exposes foldTestProgress()/ getTestSummary() so the Maven-realm listener can fold and render them. TerminalOutput drops the now-daemon-owned summary machinery, keeping only the live per-project progress line and the banned-skip filter. The -q client-side plumbing (MAVEN_QUIET, TerminalOutput's quiet field) is removed since the daemon's own logger level already handles it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix the supportsForkNode milestone check so it only guards the 3.0.0-Mx series instead of every 3.x version, always clear the JLine display at build end (including failures), make ProjectTestProgressEvent's list getters unmodifiable, dedupe the test-progress enabled check between Server and MvndTestProgressLifecycleParticipant, hoist the version regex to a static field, replace magic indices in TestBuildSummary's snapshot array with named constants, and document the BANNED_MARKER string-match as a maintenance risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dback renderBar() already produced a sane bar for out-of-range input, but the invariant wasn't self-documenting; clamp explicitly and add a regression test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Flush BannedSkipFilter on CANCEL_BUILD/BUILD_EXCEPTION (previously only BUILD_FINISHED did), replace the stale PROJECT_TEST_PROGRESS TODO now that the daemon-side feed is implemented, and give TestState.displayName() a defensive fallback so it can't return null into flakyDetail(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Guillaume Nodet <gnodet@gmail.com>
Send daemon-rendered test summaries through ClientDispatcher so clients receive the failure sections. Keep the integration test focused on the observable protocol ordering. Co-Authored-By: Codex <noreply@openai.com>
The SGR pattern in TerminalOutput and the strings in TerminalOutputTest embedded a raw ESC (0x1b) byte. That is invisible in diffs and review tooling, and can be silently dropped by anything that normalizes control characters. Replace the raw bytes with unicode escapes; the input matched at runtime is unchanged. Adds two assertions: a bare "[1m" without the ESC prefix is literal text and must survive stripping, and a color-wrapped banned marker must still strip down to BANNED_MARKER so BannedSkipFilter matches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1b13e3d to
605705e
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
AI Review — LGTM ✅
Well-architected, cleanly adapted for Maven 4 (MojoExecutionConfigurator instead of LifecycleParticipant), with comprehensive test coverage and properly defensive error handling. The previous review round's ANSI regex bug has been fixed, and no new blocking issues were found.
Key strengths:
-
2.x-specific hook: The adaptation from
LifecycleParticipant(1.x) toMojoExecutionConfigurator(2.x) is the right approach. Maven 4's immutable model prevents mutation inafterProjectsRead, so injecting the forkNode configuration at mojo-execution-configuration time is the correct hook point. The Javadoc onMvndMojoExecutionConfiguratorexplaining the Sisu wiring requirements (@Named("default"), explicit interface, no-arg constructor,@Priority) is excellent. -
Thread safety:
AtomicReferencefor the listener bridge,synchronizedonTestBuildSummary,TestProgressAccumulatorcorrectly single-threaded-per-fork per Surefire's fork-reader model. -
Error handling: Defensive throughout —
ProgressEventHandler.handleEvent()wrapsobserve()intry/catch(Throwable)so progress can never break a test run. Listener cleanup inServer.handle()'s outermostfinallyblock prevents leakage between sequential builds. -
BannedSkipFilter: Clean state machine that drops verbose "banned from the build" blocks while preserving reactor summary SKIPPED rows.
flush()calls atBUILD_FINISHED,CANCEL_BUILD, andBUILD_EXCEPTIONensure no buffered lines are lost on any exit path. -
Version check: Correctly handles the surefire version matrix (pre-3.0.0-M5, milestone guards, major > 3). Thorough test coverage.
-
Comprehensive tests: Unit tests for accumulator, summary rendering, terminal output, fork node factory, mojo configurator, message serialization. Integration tests for happy path and failure path.
Minor observations (informational only):
emitTestSummary()discards theSummaryLevel— all lines arrive as plainBUILD_LOG_MESSAGE. Acknowledged as a future protocol-level enhancement.TestSummaryExecutionListenermanually delegates all 15ExecutionListenermethods — fragile if the interface evolves, but fine for current stable Maven 4 API.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
2 participants
What this adds
The test-progress feed for mvnd 2.x (master, Maven 4): 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 #1667's worker-line suffix renders. It also adds a build-end summary of flaky/failed/errored tests built on top of that same feed.
Highlights
-Dmvnd.testProgressflag (Environment) to opt into the feature, wired into bash completion.mvnd-surefire-progressmodule providing a custom SurefireForkNodeFactory(MvndForkNodeFactory) plus aTestProgressAccumulatorthat tallies per-fork test events.MvndMojoExecutionConfiguratorinjects the fork-node factory into the Surefire mojo configuration (Maven 4's immutable model requires the configurator hook rather than 1.x's lifecycle participant), and the daemon dispatches accumulated progress to the client (ClientDispatcher,Server).InvalidatingPluginRealmCacheadds themvnd-surefire-progressjar onto the surefire/failsafe plugin realm soMvndForkNodeFactoryresolves at fork time. Separately,DaemonPlexusContainerCapsuleFactoryexports theorg.mvndaemon.mvnd.testprogressbridge package from the core realm.TestSummaryExecutionListener(daemon) andTestBuildSummary(logging) turn the accumulated per-fork data into a build-end summary of flaky, failed, and errored tests, wired in viaDaemonMavenInvoker.determineExecutionListener.TestProgressTest(feed end to end) andTestProgressFailureTest(failure/flake summary) each drive a sample project.masterare addressed by fix: feed real stdin to native mvnd process in NativeTestClient #1720.Files
36 files changed, ~2870 insertions across the common module, daemon, surefire-progress module, logging module, distribution, and integration tests.
Screenshots
Skipped here: the ones on PR #1670 are from mvnd 1.x's console output, which no longer matches 2.x — master already carries PR #1667's redesigned worker-line display (drawn progress bar, arrow-style
> :module goal (execution)lines, dimmed> IDLEslots), so those images wouldn't represent this PR's actual output. Happy to add fresh 2.x screenshots if useful.🤖 Generated with Claude Code