test(e2e): fix flaky model switchto test race condition#1628
Draft
Morabbin wants to merge 5 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Session-scoped RPC e2e test to avoid asserting a specific model ID value.
Changes:
- Replaced exact
modelIdassertions with truthiness checks in the session-scoped RPC test.
Show a summary per file
| File | Description |
|---|---|
| nodejs/test/e2e/rpc_session_state.e2e.test.ts | Loosens model ID assertions in the RPC session state e2e test. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Morabbin
force-pushed
the
morabbin/fix-rpc-model-flake
branch
from
1a6c603 to
0d4933d
Compare
The previous fix attempted to await the `session.model_change` event after `switchTo` resolved, which caused a timeout because the event was often already emitted during the `switchTo` execution. Additionally, hardcoding 'gpt-4.1' in the event listener caused timeouts on environments that gracefully fall back to default models when auth is missing. This commit starts the event listener concurrently with the `switchTo` call, and asserts that the resulting model IDs match what the server actually resolved.
Morabbin
force-pushed
the
morabbin/fix-rpc-model-flake
branch
from
0d4933d to
0ad49f7
Compare
Morabbin
changed the title
Wrap the multi-arg waitForEvent call and drop trailing whitespace so npm run format:check passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI environments without entitlement for the requested model (gpt-4.1) fall back to a different model, so switchTo can echo the requested id while getCurrent reports the fallback. Assert truthiness instead of equality to the requested/returned id, matching the getCurrent pattern. Use a proper session.model_change type guard so event.data.newModel is typed without an `as any` cast, clearing the no-explicit-any lint warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Register the model_change listener before calling switchTo so a fast event cannot fire before the listener is attached (the original race). Assert the switchTo result and the model_change event agree (same server operation) instead of hard-coding gpt-4.1, which fails in fallback/no-auth environments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous test attempted to await the
session.model_changeevent afterswitchToresolved. This caused a timeout because the event was often already emitted during theswitchToexecution (resulting in a race condition that failed frequently on Windows CI). Additionally, hardcoding 'gpt-4.1' in the event listener and assertions caused timeouts in environments that gracefully fall back to default models when auth is missing.This PR fixes the test by starting the event listener concurrently with the
switchTocall, and asserts that the resulting model IDs match what the server actually resolved.