Conversation
✅ Cross-SDK Consistency Review: APPROVEDI've reviewed PR #665 for cross-language SDK consistency, and I'm happy to report this PR demonstrates excellent consistency across all four SDK implementations. Summary of ChangesThis PR expands E2E test coverage for session resume and streaming functionality consistently across all SDKs (Node.js, Python, Go, and .NET). The changes include: 1. Enhanced Session Resume Tests
2. New Streaming Fidelity Test Suite
3. Test Organization Improvement
Cross-SDK Consistency Verification ✅I verified that all four SDKs have equivalent test implementations:
The test implementations are remarkably consistent:
No consistency issues found. This PR maintains excellent feature parity across all SDK implementations. 🎉
|
There was a problem hiding this comment.
Pull request overview
Expands cross-SDK E2E coverage around session resume and its interaction with streaming delta events, aiming to make the scenarios consistent across Node, Python, Go, and .NET using the shared replay-proxy snapshot mechanism.
Changes:
- Added/extended “continue conversation after resume” assertions across SDKs and updated the corresponding session snapshots.
- Introduced dedicated Streaming Fidelity E2E test suites (Python/Go/.NET) and a new snapshot for “deltas after resume”.
- Removed older session-level streaming tests/snapshots (with an apparent intent to consolidate streaming coverage under
streaming_fidelity).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/streaming_fidelity/should_produce_deltas_after_session_resume.yaml | Adds a shared snapshot conversation for streaming deltas after resuming a session. |
| test/snapshots/session/should_resume_a_session_using_the_same_client.yaml | Extends snapshot to include a follow-up turn after resume. |
| test/snapshots/session/should_resume_a_session_using_a_new_client.yaml | Extends snapshot to include a follow-up turn after resume. |
| test/snapshots/session/should_receive_streaming_delta_events_when_streaming_is_enabled.yaml | Removes a session-category snapshot previously used by Node session E2E tests. |
| test/snapshots/session/should_pass_streaming_option_to_session_creation.yaml | Removes a session-category snapshot previously used by Node session E2E tests. |
| python/e2e/test_streaming_fidelity.py | Adds new Python streaming fidelity E2E tests, including “deltas after resume”. |
| python/e2e/test_session.py | Adds assertions that resumed sessions remain stateful; removes older streaming-related tests. |
| nodejs/test/e2e/streaming_fidelity.test.ts | Adds Node test for “deltas after resume” (new client resume scenario). |
| nodejs/test/e2e/session.test.ts | Adds “continue conversation statefully” assertions for resume tests. |
| go/internal/e2e/streaming_fidelity_test.go | Adds Go streaming fidelity E2E tests including “deltas after resume”. |
| go/internal/e2e/session_test.go | Adds “continue conversation statefully” checks for resume tests. |
| dotnet/test/StreamingFidelityTests.cs | Adds .NET streaming fidelity E2E tests including “deltas after resume”. |
| dotnet/test/SessionTests.cs | Adds “continue conversation statefully” checks for resume tests; removes older streaming tests. |
test/snapshots/streaming_fidelity/should_produce_deltas_after_session_resume.yaml
Show resolved
Hide resolved
- Remove unused 'answer' variable in Python streaming fidelity test - Add onTestFinished cleanup for newClient in Node.js streaming resume test - Remove streaming tests from Node.js session.test.ts (were still referencing deleted session/ snapshots) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
b615abb to
aaac89c
Compare
Our E2E coverage for resume, and especially its interaction with streaming, was a bit inconsistent.
This expands coverage and makes it consistent across all languages.