Fix E2E tests after runtime update#919
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes the Node E2E “session start” assertion by making it resilient to additional non-session.start events being present in session.getMessages() immediately after session creation.
Changes:
- Collects all session history events and filters down to
session.startevents before asserting shape/content.
SteveSandersonMS
changed the title
Cross-SDK Consistency Issue: Test Assumptions About Message OrderThis PR fixes the Node.js E2E test to handle cases where Issue DetailsNode.js (✅ Fixed in this PR): // Before: Assumed messages[0] is session.start
expect(await session.getMessages()).toMatchObject([...])
// After: Filters for session.start events
const allEvents = await session.getMessages();
const sessionStartEvents = allEvents.filter((e) => e.type === "session.start");
expect(sessionStartEvents).toMatchObject([...])Python (❌ Needs similar fix): # python/e2e/test_session.py:24-28
messages = await session.get_messages()
assert len(messages) > 0
assert messages[0].type.value == "session.start" # ⚠️ Assumes messages[0] is session.start
assert messages[0].data.session_id == session.session_idGo (❌ Needs similar fix): // go/internal/e2e/session_test.go:33-40
messages, err := session.GetMessages(t.Context())
if len(messages) == 0 || messages[0].Type != "session.start" { // ⚠️ Assumes messages[0] is session.start
t.Fatalf("Expected first message to be session.start, got %v", messages)
}.NET (❌ Needs similar fix): // dotnet/test/SessionTests.cs:23-26
var messages = await session.GetMessagesAsync();
Assert.NotEmpty(messages);
var startEvent = Assert.IsType(SessionStartEvent)(messages[0]); // ⚠️ Assumes messages[0] is session.start
Assert.Equal(session.SessionId, startEvent.Data.SessionId);Recommended ActionTo maintain cross-SDK consistency, apply equivalent fixes to Python, Go, and .NET test suites:
This pattern appears in multiple test methods across all SDKs (e.g., in concurrent session tests at Python line 167-170, Go resumed session tests around line 469). Test Harness ChangeThe second change in this PR (returning 404 for non-chat-completion requests) is test infrastructure and doesn't affect API consistency. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #919
|
|
||
| expect(await session.getMessages()).toMatchObject([ | ||
| const allEvents = await session.getMessages(); | ||
| const sessionStartEvents = allEvents.filter((e) => e.type === "session.start"); |
There was a problem hiding this comment.
✅ Good fix! This change makes the test robust to event ordering. The same pattern should be applied to Python, Go, and .NET tests - they currently assume messages[0] is always session.start, which will fail with the runtime update.
| // Beyond this point, we're only going to be able to supply responses in CI if we have a snapshot, | ||
| // and we only store snapshots for chat completion. For anything else (e.g., custom-agents fetches), | ||
| // return 404 so the CLI treats them as unavailable instead of erroring. | ||
| if (options.requestOptions.path !== chatCompletionEndpoint) { |
There was a problem hiding this comment.
This change improves test harness behavior by returning 404 for non-chat-completion requests (e.g., custom-agents fetches) instead of erroring. This is test infrastructure and doesn't impact SDK API consistency. ✅
No description provided.