Test: Add e2e test for duplicate events after resume (#567)#630
Test: Add e2e test for duplicate events after resume (#567)#630SteveSandersonMS wants to merge 1 commit intomainfrom
Conversation
Regression test for #567. Verifies that resuming a session multiple times does not cause duplicate event delivery. The test creates a session, resumes it twice (letting old handles go ungracefully), then sends a message and asserts that assistant.message arrives exactly once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a Node.js E2E regression test (with a matching replay snapshot) to catch duplicate session events after calling resumeSession() multiple times, as reported in #567. This fits into the repo’s cross-SDK replay-based E2E suite to prevent regressions in session lifecycle behavior.
Changes:
- Added a new E2E test that resumes the same session twice and asserts
assistant.messageis delivered exactly once. - Added a new replay snapshot YAML covering the two-turn conversation used by the test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/snapshots/session/should_not_receive_duplicate_events_after_resume.yaml |
Adds replay snapshot for the new regression test scenario (two math prompts/responses). |
nodejs/test/e2e/session.test.ts |
Adds a regression test asserting no duplicate assistant.message events after multiple resumes. |
| it("should not receive duplicate events after resume", async () => { | ||
| // Regression test for https://github.com/github/copilot-sdk/issues/567 | ||
| // Each resumeSession call was adding an additional event listener without | ||
| // removing the previous one, causing events to be delivered N+1 times after N resumes. | ||
|
|
||
| // Create session and do a turn, then let the session handle go away ungracefully | ||
| const session1 = await client.createSession({ onPermissionRequest: approveAll }); | ||
| const sessionId = session1.sessionId; | ||
| await session1.sendAndWait({ prompt: "What is 1+1?" }); | ||
|
|
||
| // Resume twice (simulating two reconnects) without cleaning up previous handles. | ||
| // The old session handles just go away as they would in a real disconnect. | ||
| await client.resumeSession(sessionId, { onPermissionRequest: approveAll }); | ||
| const session3 = await client.resumeSession(sessionId, { onPermissionRequest: approveAll }); | ||
|
|
There was a problem hiding this comment.
This new regression test is expected (per PR description) to fail until the Copilot CLI/runtime fix lands, but this PR doesn’t bump the bundled CLI dependency or otherwise gate/skip the test. As-is, merging this PR is likely to make CI fail for the SDK repo. Consider temporarily marking this test as skipped with a TODO referencing the runtime PR (and unskip once the CLI package version used in tests includes the fix), or bump the CLI/runtime dependency in the same PR so the test is green immediately.
Cross-SDK Consistency ReviewThank you for adding this regression test for issue #567! The test verifies that duplicate events are not received after multiple 🔍 Consistency Gap IdentifiedThis PR adds the test only to the Node.js/TypeScript SDK, but the underlying issue is in the runtime's 📋 RecommendationConsider adding equivalent tests to the other SDK implementations:
The good news is that the test snapshot file (
This would ensure all SDKs have regression coverage for this issue. Note: This doesn't block the current PR if the plan is to add tests to other SDKs separately. Just flagging it for visibility.
|
Overview
Adds a regression test for #567 — duplicate event delivery after calling
resumeSession()multiple times.What the test does
assistant.messagearrives exactly once, not duplicatedWithout the corresponding runtime fix, this test fails with 3 copies of
assistant.message(1 original + 1 per leaked listener from each resume).Dependency