Skip to content

test(nodejs): fix connection failure and recovery E2E test - #790

Merged
sirtimid merged 1 commit into
mainfrom
sirtimid/fix-connection-recovery-e2e-test
Jan 29, 2026
Merged

test(nodejs): fix connection failure and recovery E2E test#790
sirtimid merged 1 commit into
mainfrom
sirtimid/fix-connection-recovery-e2e-test

Conversation

@sirtimid

@sirtimidsirtimid commented Jan 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Fixes the TODO E2E test that was timing out
  • Restructures the test to follow the pattern of successful reconnection tests

The key change is queueing the recovery message while the peer is down (which triggers reconnection logic) rather than after the peer restarts.

Test plan

  • E2E tests pass locally (yarn workspace @ocap/nodejs test:e2e:ci)
  • All 16 remote-comms tests pass, including the fixed test

Closes#663

🤖 Generated with Claude Code


Note

Low Risk
Test-only change that alters reconnection timing/ordering; low production risk but could affect E2E stability/flakiness if assumptions about recovery timing are wrong.

Overview
Enables the previously skipped handles connection failure and recovery E2E by restructuring it to queue the recovery message while the peer kernel is down, then restart the peer so the queued call drives reconnection.

Adds explicit assertions for initial connectivity, successful recovery (status: connected and expected pong), and a follow-up message to confirm the connection remains usable after restart.

Written by Cursor Bugbot for commit 51bd597. This will update automatically on new commits. Configure here.

Fixes the TODO test that was timing out by restructuring it to follow the
pattern of other successful reconnection tests. The key change is queueing
the recovery message WHILE the peer is down (which triggers reconnection
logic) rather than AFTER the peer restarts.
The test now:
1. Establishes initial connectivity
2. Stops the remote kernel to simulate failure
3. Queues a testConnection message (triggers reconnection)
4. Restarts the remote kernel
5. Verifies the recovery message completes successfully
6. Verifies ongoing connectivity with a follow-up message
Closes#663
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@sirtimid
sirtimid requested a review from a team as a code ownerJanuary 29, 2026 17:02
@sirtimid
sirtimid enabled auto-merge January 29, 2026 17:04
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines88.59%
🟰 ±0%
5778 / 6522
🔵Statements88.47%
🟰 ±0%
5870 / 6635
🔵Functions87.66%
🟰 ±0%
1507 / 1719
🔵Branches84.83%
🟰 ±0%
2086 / 2459
File CoverageNo changed files found.
Generated in workflow #3442 for commit 51bd597 by the Vitest Coverage Report Action

@rekmarksrekmarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sirtimid
sirtimid added this pull request to the merge queueJan 29, 2026
Merged via the queue into main with commit 1843d79Jan 29, 2026
32 checks passed
@sirtimid
sirtimid deleted the sirtimid/fix-connection-recovery-e2e-test branch January 29, 2026 17:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote comms: Fix remaining TODO tests

2 participants

@sirtimid@rekmarks