Skip to content

Visual improvements to worker lines with live test-progress suffix (mvnd-1.x) - #1666

Open
ammachado wants to merge 3 commits into
apache:mvnd-1.xfrom
ammachado:feature/worker-line-display-1x
Open

Visual improvements to worker lines with live test-progress suffix (mvnd-1.x)#1666
ammachado wants to merge 3 commits into
apache:mvnd-1.xfrom
ammachado:feature/worker-line-display-1x

Conversation

@ammachado

@ammachado ammachado commented Jul 2, 2026

Copy link
Copy Markdown

What

Presentation-only reshaping of mvnd's live terminal display, plus the client-side plumbing for a live per-test progress suffix, on mvnd-1.x.

Live now (driven by existing messages)

  • A drawn progress bar with percent prefixed to the status line.
  • Concise arrow-style worker lines: > :module goal (execution).
  • Dimmed > IDLE slots for free worker threads.

Wired but dormant (client-side only)

  • New PROJECT_TEST_PROGRESS wire message (Message.ProjectTestProgressEvent, round-trip serialization).
  • TerminalOutput.appendTestProgress(...) renders a styled suffix [Tests: N, Failures: .., Errors: .., Skipped: ..] Class#method, with failure/error counts in red.

Follow-up

The daemon-side feed that emits PROJECT_TEST_PROGRESS (the surefire ForkNodeFactory bridge) is not implemented on mvnd-1.x yet, so the test-progress suffix stays dormant until a follow-up commit lands it. This is a deliberate, spike-gated separation; a TODO on the PROJECT_TEST_PROGRESS constant records it.

Verification

  • common unit tests pass (message round-trip; bar geometry; suffix formatting and styles), RAT clean.
  • MultiModuleTest integration test passes (the reshaping did not break message-driven client assertions).
  • Built and verified with JDK 17.

🤖 Generated with Claude Code

@ammachado
ammachado marked this pull request as ready for review July 2, 2026 20:07
@ammachado
ammachado force-pushed the feature/worker-line-display-1x branch 2 times, most recently from d3eab36 to c81975d Compare July 3, 2026 04:45
@ammachado ammachado mentioned this pull request Jul 5, 2026
Open
@ammachado
ammachado force-pushed the feature/worker-line-display-1x branch from c81975d to 09d1362 Compare July 20, 2026 17:19
ammachado and others added 2 commits July 21, 2026 09:08
@ammachado @claude
test: prevent ambient environment from leaking into daemon builds on CI
882d615 
- Extract buildRequestEnvironment() in DefaultClient for subclass override
- Strip MAVEN_ARGS from JvmTestClient and NativeTestClient to block per-test flag overrides
- Write an isolated settings.xml in MvndTestExtension so tests never fall back to
  the runner's user settings (fixes InteractiveTest when actions/setup-java sets interactiveMode)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ammachado @claude
feat: visual improvements to worker lines with live test-progress suf…
d6ac6c0 
…fix on mvnd-1.x

Reshape the live terminal display: a drawn progress bar with percent on the
status line, concise arrow-style worker lines (> :module goal (execution)),
and dimmed > IDLE slots for free threads. At addProjectLine, append a styled
live test-progress suffix (tests/failures/errors/skipped counts plus the
current test class#method) driven by the new PROJECT_TEST_PROGRESS message.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ammachado
ammachado force-pushed the feature/worker-line-display-1x branch from 09d1362 to d6ac6c0 Compare July 21, 2026 13:09
gnodet
gnodet approved these changes Jul 28, 2026
View reviewed changes

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured PR adding client-side wire plumbing for future live test-progress display with tasteful visual improvements to worker lines.

Highlights:

  • New PROJECT_TEST_PROGRESS wire message (type 29) correctly marked dormant with a TODO for the daemon-side follow-up
  • buildRequestEnvironment() template-method pattern cleanly separates production and test paths
  • Isolated <settings/> file is a correct fix for the CI issue where actions/setup-java injects user settings
  • Solid test coverage: MessageTest covers serialization round-trips, TerminalOutputTest covers progress bar geometry and ANSI style assertions

Two minor observations (not blocking):

  • renderBar() doesn't clamp input — works correctly for edge cases but could be self-documenting
  • Plugin version removed from worker display is an intentional UX conciseness tradeoff per the PR description

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 added a commit to ammachado/maven-mvnd that referenced this pull request Jul 28, 2026
@ammachado @claude
fix: clamp renderBar percent to [0,100] per PR apache#1666 review fee…
3eedb48 
…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>
@ammachado

ammachado commented Jul 28, 2026

Copy link
Copy Markdown
Author

Thanks for the approval! Addressed the one non-blocking suggestion: renderBar() now clamps percent to [0, 100] explicitly with a comment on the invariant, plus a regression test for out-of-range input. Since #1670 is stacked on this branch, the fix landed there as 3eedb48 rather than as a new commit on this branch directly.

gnodet
gnodet reviewed Jul 30, 2026
View reviewed changes

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured PR with clean separation between visual improvements and dormant test-progress plumbing. The code is solid, test isolation improvements are valuable CI hygiene, and the new TerminalOutputTest coverage is thorough.

Two minor informational notes:

  1. Worker line format change (low): The concise worker line format intentionally drops the groupId: prefix and plugin version that were added in PR #748 (commit 724d1fe). The PR description acknowledges this trade-off for conciseness — just noting it as a deliberate reversal of a prior design decision.

  2. TransferEvent.toString() (low): The conditional exception field change is cosmetically unrelated to the PR's main scope. Consider splitting it into its own commit for a cleaner bisect history.

Overall this looks good — the test infrastructure fixes (isolated settings.xml, MAVEN_ARGS stripping) and the visual improvements are well-executed.

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 @claude
fix: port PR apache#1667 review feedback to mvnd-1.x
b574e61 
Applies the review-driven fixes from the sibling 2.x PR (apache#1667):
- PROJECT_TEST_PROGRESS now sorts after PROJECT_STARTED/MOJO_STARTED in the
  sendQueue, avoiding a race where the progress message could be dequeued
  before its project's PROJECT_STARTED event.
- ProjectTestProgressEvent reuses the existing readUTF/writeUTF null-sentinel
  convention instead of a redundant boolean-prefixed nullable encoding.
- Restores the plugin groupId/version in the worker line display, which was
  dropped by the visual redesign.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ammachado added a commit to ammachado/maven-mvnd that referenced this pull request Jul 31, 2026
@ammachado @claude
fix: clamp renderBar percent to [0,100] per PR apache#1666 review fee…
629790e 
…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>
@ammachado
ammachado changed the base branch from mvnd-1.x to master July 31, 2026 22:45
@ammachado
ammachado changed the base branch from master to mvnd-1.x July 31, 2026 22:49
gnodet
gnodet approved these changes Aug 1, 2026
View reviewed changes

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after commit b574e61 ("fix: port PR #1667 review feedback to mvnd-1.x"): all prior findings cleanly addressed.

  1. Message ordering — PROJECT_TEST_PROGRESS moved from class order 2 to 5, preventing the dequeue race.
  2. Serialization — Redundant writeNullableUTF/readNullableUTF removed; existing readUTF/writeUTF null-sentinel convention used correctly.
  3. Worker line display — pluginGroupId and pluginVersion restored.

All 6 CI checks pass. No new issues. ✅

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 added a commit to ammachado/maven-mvnd that referenced this pull request Aug 1, 2026
@ammachado @claude
fix: clamp renderBar percent to [0,100] per PR apache#1666 review fee…
f2bb155 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnodet gnodet gnodet approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ammachado @gnodet