test(e2e): add SSE streaming coverage - #350
Draft
duyhungtnn wants to merge 11 commits into
Draft
Conversation
Add end-to-end coverage for the SSE streaming transport: initial delivery, the polling-timeout-to-stream fallback, reconnect after updateUserAttributes, and a server-pushed patch landing on an open stream. The patch test mutates a live flag, so it needs a write-capable key and is skipped without one. The e2e workflow now runs under a concurrency guard so overlapping runs don't race on that same flag.
The streaming and streamingPatch e2e suites were left skipped after being added. Re-enable them now that they are verified working. Move the --browser.headless flag from the CI workflow into the test:e2e:browser script itself, and chain the browser and node e2e runs with && so a browser suite failure stops the run instead of being masked by the node suite's exit code.
Test 1 in streaming.spec.ts only checked that a stream_evaluations request had been sent, not that it succeeded, so it kept passing even with SSE completely broken (401, 404, aborted). It now waits for the stream's own response to resolve ok, which recordingFetch captures alongside the request URL. Every vi.waitFor in these suites also used the same timeout as its enclosing test, so the outer timeout always won first and hid the real assertion failure behind an opaque "test timed out" message. Inner timeouts are now clearly shorter. Also documents why the shared streaming test flag is never restored after a write: each write is a disposable, unique-per-run value that no test reads back as a baseline.
The streaming-patch test wrote the flag right after init resolved, but init only waits for the REST call, not for the SSE connection. A fast write could land before the stream opened, leaving the push undelivered until the test timed out with no useful message. Extract the open-stream check already used in the init test into a shared helper and reuse it as a barrier before the write. Also add comments to the /get_evaluations count checks explaining what a different count would mean, since they're easy to misread as arbitrary assertions.
A missing public API key previously surfaced as a 401 from the server or an undici TypeError about an undefined header, neither of which names the actual cause. Throw an explicit, named error as soon as the module loads, and mark the env type as optional so the missing case is representable.
The PATCH response body carries the reason a variation update was rejected, but it was discarded and never read, leaving the socket open under Node's undici. Read it once and include it in the thrown error so failures are debuggable.
Mutating the shared config object after defineBKTConfig() bypassed its validation and defaults, and relied on the returned object never being frozen. The comment justifying the override also no longer held, since neither test runner leaves cached values between tests. Build the timeout test's own config with its storage prefix set from the start instead, matching the pattern already used in streamingPatch.spec.ts.
There was a problem hiding this comment.
🟡 Changes recommended
The patch test uses the wrong variation accessor and missing optional credentials currently fail test collection instead of skipping the test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds end-to-end SSE coverage and CI safeguards for tests that mutate a live feature flag.
Changes:
- Tests stream initialization, fallback, reconnection, and pushed patches.
- Adds request recording and feature-flag mutation helpers.
- Adds E2E concurrency, secrets, headless execution, and type checks.
File summaries
| File | Description |
|---|---|
package.json |
Makes E2E execution sequential and headless. |
env.template |
Documents the write-capable API key. |
e2e/streamingPatch.spec.ts |
Tests server-pushed patches. |
e2e/streaming.spec.ts |
Tests SSE initialization, fallback, and reconnection. |
e2e/recordingFetch.ts |
Records requests and stream responses. |
e2e/globals.d.ts |
Types the optional API key. |
e2e/featureFlagApi.ts |
Adds live flag mutation utilities. |
e2e/constants.ts |
Adds the streaming feature ID. |
e2e/BKTClient.spec.ts |
Awaits attribute updates. |
.github/workflows/e2e.yml |
Adds concurrency and the API secret. |
.github/workflows/build.yml |
Adds library and test type checks. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Stream-open detection mishandles retries, and the promised workflow concurrency control is absent.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/e2e.yml:36
- The PR description says overlapping E2E runs are serialized, but this workflow has no
concurrencygroup. Because the new suite PATCHes a shared backend flag, pushes or dispatches can still overlap and race; add a workflow-level fixed concurrency group withcancel-in-progress: false.
e2e/recordingFetch.ts:20 responseForpermanently selects the first response for this path. If the stream's first attempt gets a recoverable 5xx and a retry succeeds,waitForStreamOpenstill times out; a bodyless 2xx also passes even thoughFetchEventSourcetreats it as terminal. Select a response that meets the transport's actual open conditions instead.
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
A response can resolve with ok: true but no usable readable body, which the SSE client treats as terminal rather than open, so check for a working body reader too, not just ok: true. Also read the latest matching response instead of the first, so a retry after a recoverable failure is reflected instead of being hidden behind the earlier failed attempt.
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.
This pull request introduces comprehensive improvements to the end-to-end (E2E) test suite, focusing on streaming (SSE) feature flag updates, test reliability, and CI workflow enhancements. Notably, it adds new E2E tests for streaming, utilities for recording fetch requests, and ensures environment variables are correctly set for public API access. It also improves type safety and concurrency handling in CI.
E2E Streaming Test Enhancements:
streaming.spec.tsto verify streaming (SSE) behavior, including stream connection establishment, fallback when REST is unavailable, and re-evaluation on user attribute updates.streamingPatch.spec.tsto test that server-pushed PATCH updates to feature flags are delivered over an open stream, using new helpers for dynamic test values and patching. [1] [2]Test Utilities and Helpers:
recordingFetch.tsutility to track and assert HTTP requests and responses made during tests, including a helper to wait for SSE stream connection.CI and Workflow Improvements:
Type Safety and Test Reliability:
updateUserAttributesto ensure correct sequencing.Test Command Consistency: