Skip to content

test(tui): cover WSTransport write paths, peer label, and handle_ws lifecycle (#36617) - #39393

Open
rodboev wants to merge 4 commits into
NousResearch:mainfrom
rodboev:pr/test-slash-worker
Open

test(tui): cover WSTransport write paths, peer label, and handle_ws lifecycle (#36617)#39393
rodboev wants to merge 4 commits into
NousResearch:mainfrom
rodboev:pr/test-slash-worker

Conversation

@rodboev

@rodboev rodboev commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #36617

Summary

Current main already covers MCP startup, disconnect teardown, and stalled-loop recovery for tui_gateway/ws.py, so the older branch description was no longer accurate. The remaining gap was the coalesced transport path: buffered streaming frames, _safe_send_many, batch-send failure, and close-time timer cancellation. The branch also patched only server, even though handle_ws directly imports and calls start_background_mcp_discovery.

This refresh rebuilds the PR on current main, consolidates the surviving coverage into tests/test_tui_gateway_ws.py, applies one shared MCP-startup patch across every handle_ws case, and adds assertions that distinguish the buffered _safe_send_many path from the old direct-send transport.

Changes

  • tests/test_tui_gateway_ws.py (+248/-41): add shared MCP-startup patching, prove buffered _safe_send_many batching, cover batch-send failure and timer cancellation, and keep the current-main handle_ws lifecycle cases in one module.
  • tests/tui_gateway/test_ws.py (-225): remove the duplicate older module after its surviving cases were consolidated into the current-main test file.

Validation

Scenario Before After
Two streaming frames followed by a control frame only direct-send ordering was covered test proves buffered tokens flush through one _safe_send_many batch before the control frame
_safe_send_many failure mid-batch untested covered, transport latches closed and later frames are not sent
close() with an armed token flush timer untested covered, timer is cancelled and the handle is cleared
handle_ws startup side effect in test cases only server was patched in the added cases every handle_ws case runs under the shared start_background_mcp_discovery patch

Test plan

  • pytest tests/test_tui_gateway_ws.py -v — 18 passed
  • ruff check tests/test_tui_gateway_ws.py — passed

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Jun 5, 2026
@rodboev
rodboev force-pushed the pr/test-slash-worker branch from 5567bbf to a72d89e Compare June 28, 2026 17:56
@rodboev
rodboev force-pushed the pr/test-slash-worker branch from a72d89e to d29b3c4 Compare July 7, 2026 04:30

@teknium1 teknium1 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.

Thanks for adding focused WebSocket coverage. This needs a current-main refresh before it can be salvaged.

Problems

  • tests/tui_gateway/test_ws.py:116 patches only tui_gateway.ws.server, while current handle_ws directly imports and invokes start_background_mcp_discovery at tui_gateway/ws.py:312-317. These tests would mutate the shared MCP-startup guard (hermes_cli/mcp_startup.py:31-50) and can start real background discovery.
  • Current WSTransport now coalesces streaming frames and sends via _safe_send_many (tui_gateway/ws.py:97-104, 125-159, 193-207). The proposed tests cover the earlier simple write paths but not batching, ordering, batch-send failure, or flush cancellation.

Suggested changes

  • Add a shared patch for hermes_cli.mcp_startup.start_background_mcp_discovery, modeled on tests/test_tui_gateway_ws.py:10-42.
  • Extend coverage for the current coalescing and teardown contracts; existing lifecycle coverage is in tests/test_tui_gateway_ws.py:90-164.

Automated hermes-sweeper review.

Comment thread tests/tui_gateway/test_ws.py Outdated
Comment thread tests/tui_gateway/test_ws.py Outdated
@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve test coverage: tui_gateway/ws.py

3 participants