Fix active Node and replay E2E flakes - #2186
Conversation
4e5f6d6 to
ba7bf47
Compare
This comment has been minimized.
This comment has been minimized.
ba7bf47 to
daa55a7
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Reduces CI flakiness across Node/.NET/Rust E2E coverage by adjusting timeouts, isolating Windows .NET target frameworks into separate jobs, and aligning a Rust E2E snapshot with its actual prompt/expectations.
Changes:
- Adjust Node Vitest integration timeouts to allow longer execution on Windows while keeping other OS budgets unchanged.
- Split .NET
net8.0andnet472runs into distinct Windows matrix jobs via aframeworkdimension and pass--frameworktodotnet test. - Add a dedicated Rust snapshot and rename the Rust E2E test to match the new snapshot/intent.
Show a summary per file
| File | Description |
|---|---|
nodejs/vitest.config.ts |
Introduces OS-specific test/hook timeouts to reduce Windows E2E flakiness. |
.github/workflows/dotnet-sdk-tests.yml |
Adds a framework matrix dimension and runs dotnet test with explicit framework selection to avoid Windows runner contention. |
rust/tests/e2e/rpc_session_state.rs |
Renames the Rust E2E test to match the new processing/context metadata behavior/snapshot. |
test/snapshots/rpc_session_state/should_report_processing_and_context_metadata.yaml |
Adds a dedicated snapshot aligned with the Rust context metadata test prompt/expected output. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Ready to approve
The focused changes consistently address the three documented CI flakes without introducing unresolved issues.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Use a 60-second Vitest test and hook budget on Windows runners while preserving the existing 30-second budget elsewhere. This covers both default and in-process transports without weakening individual assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51fbb14d-022f-4acb-bdce-1753cba236f9
Run net8.0 and net472 in independent Windows matrix jobs instead of concurrently in one testhost job. This isolates each CLI and replay-proxy fixture graph without increasing the workflow's critical-path test duration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51fbb14d-022f-4acb-bdce-1753cba236f9
Rename the repurposed metadata test and route it to a snapshot that matches its RUST_CONTEXT_INFO prompt. Preserve the existing compaction snapshot used by Python instead of sharing an incompatible conversation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51fbb14d-022f-4acb-bdce-1753cba236f9
daa55a7 to
824defe
Compare
SDK Consistency Review ✅This PR contains only CI/test infrastructure fixes and does not modify any public SDK API surface. No cross-SDK consistency issues were found. Changes reviewed:
No changes to SDK client APIs; no action needed in other language implementations.
|
There was a problem hiding this comment.
🟡 Not ready to approve
The Rust test still relies on observing a transient processing state that the immediate replay response can complete before polling begins.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
rust/tests/e2e/rpc_session_state.rs:1105
- The dedicated fixture now completes a one-token response with no replay delay, but this test polls for a transient
processing == truestate only aftersend()returns. The replay proxy emits all chunks synchronously whenslowStreamingis false, whilewait_for_conditionpolls every 100 ms, so the turn can already be idle before the first poll and the test remains timing-dependent. Please make the in-flight state deterministic (for example, hold the turn open with a controlled tool/delayed stream) or avoid asserting that the transient state must be observed.
"should_report_processing_and_context_metadata",
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
July CI runs exposed three hermetic test failures: arbitrary Node E2E tests exhausted the fixed Windows timeout budget, the two .NET target-framework suites competed on the same Windows runner, and a repurposed Rust metadata test still selected an unrelated compaction snapshot.
This change:
net8.0andnet472coverage in separate parallel Windows matrix jobs, isolating each CLI/replay-proxy fixture graph without extending the workflow critical path;RUST_CONTEXT_INFOprompt, while preserving the shared compaction fixture used by Python.Validation included workflow YAML and shell syntax checks, Rust nightly formatting, Rust test-target compilation, snapshot parsing and preservation checks, and commit whitespace checks. Full local E2E execution was blocked because the configured package-feed proxy returned 404 for
@github/copilot@1.0.76-5.