test(ui/streamStoreHelpers): cover performConnect, watch/mic warnings, status sync#446
Conversation
…, status sync Adds 17 behavioral tests extending the existing test file: - performConnect: abort short-circuits, watch-only happy path, watch+publish happy path, deferred audio enable, requesting mic status, connection timeout sets error+disconnected, mid-connect abort doesn't overwrite state, watch setup errors clean up the connection. - schedulePostConnectWarnings (watch): warns after 10s when watchStatus stays non-live, stays silent when already live, bails out when the user disconnects before the threshold. - schedulePostConnectWarnings (mic): warns after 10s when mic source never resolves, stays silent when ready immediately, stays silent when mic becomes ready before the threshold. - setupConnectionStatusSync: propagates relay 'disconnected'/'connecting' status to the store with the expected error-message behavior. Behaviors are asserted via the mutated ConnectableState (status, errorMessage, mic/cameraStatus, isMicEnabled/isCameraEnabled), not via mock-call introspection. Tests reuse the existing @moq/* vi.mock blocks and createMockSignal/makeTrack helpers — no parallel test scaffolding. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 62.90% 63.23% +0.32%
==========================================
Files 215 215
Lines 55435 55435
Branches 1597 1597
==========================================
+ Hits 34874 35055 +181
+ Misses 20555 20374 -181
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| // --------------------------------------------------------------------------- | ||
| // performConnect | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
🟡 New section divider comment violates repository comment guidelines
The PR adds a performConnect section-divider banner even though AGENTS.md explicitly lists section dividers as comments not to write and says to use code structure instead. This is a repository-specific mandatory review rule violation in the changed test code.
| // --------------------------------------------------------------------------- | |
| // performConnect | |
| // --------------------------------------------------------------------------- |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| asMock(Hang.Moq.Connection.Reload).mockImplementation(function () { | ||
| return { established: connEstablished, status: connStatus, close: vi.fn() }; | ||
| }); | ||
| asMock(Effect).mockImplementation(function () { | ||
| return mockEffect; | ||
| }); | ||
| asMock(Watch.Broadcast).mockImplementation(function () { | ||
| return { status: watchStatusSig, close: vi.fn() }; | ||
| }); | ||
| asMock(Watch.Sync).mockImplementation(function () { | ||
| return { close: vi.fn() }; |
There was a problem hiding this comment.
📝 Info: performConnect tests intentionally use lightweight MoQ stand-ins
The new performConnect suite mocks MoQ constructors with minimal objects and does not configure Publish.Lite.Path.from / Watch.Lite.Path.from return values. I did not treat this as a bug because setupPublishPath and setupWatchPath only pass those values into mocked Publish.Broadcast / Watch.Broadcast constructors during these tests (ui/src/stores/streamStoreHelpers.ts:604-609, ui/src/stores/streamStoreHelpers.ts:411-416), and the assertions target store transitions, cleanup, status subscriptions, and timer warnings rather than path parsing behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| // Simulate the store transitioning out of `connected` before the timer fires | ||
| // — the warning callback should bail out without setting an error message. | ||
| state.status = 'disconnected'; | ||
| state.errorMessage = ''; |
There was a problem hiding this comment.
📝 Info: New warning tests rely on direct state mutation to simulate disconnects
The post-connect warning tests mutate the local state object directly before advancing fake timers. This is acceptable for this helper-level test because get returns the same mutable object and schedulePostConnectWarnings gates on get().status (ui/src/stores/streamStoreHelpers.ts:870-877), but it means the test is specifically validating the helper callback's status check rather than a full Zustand disconnect flow.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
Phase 2 coverage sprint —
ui/src/stores/streamStoreHelpers.tshad ~340 uncovered lines (the biggest single UI gap). This PR extends the existingstreamStoreHelpers.test.tswith 17 behavioral tests for the previously-uncovered orchestration entry point (performConnect) and the helpers it composes (schedulePostConnectWarnings,setupConnectionStatusSync,setupWatchPath's status sync). No production code is modified.All assertions are on observable behavior — the mutated
ConnectableState(status,errorMessage,micStatus,isMicEnabled/isCameraEnabled,watchStatus) and side-effects on the broadcast-config (audio.enabled.set). Mock-call introspection is not the sole assertion in any test.Behaviors covered
performConnect— abort-already-aborted short-circuit, watch-only happy path, watch+publish happy path (mic + camera enabled), deferred audio enable for combined audio+video publish, "requesting" mic status when source is initially unavailable, 12s relay-connection timeout setsdisconnected+errorMessage, mid-connect abort doesn't overwrite store state, watch-setup throw cleans up the connection and reports the error.schedulePostConnectWarnings(watch broadcast) — fires "not live yet" warning after 10s when watch status stays non-live, stays silent when already live, and bails out cleanly when the user disconnects before the threshold.schedulePostConnectWarnings(microphone) — fires "microphone is not available" warning after 10s when mic source never resolves, stays silent when mic source is immediately ready, and stays silent when the mic-source signal fires before the threshold.setupConnectionStatusSync— propagates relaydisconnectedstatus to a store-leveldisconnected+ descriptive error message, and propagates relayconnectingstatus toconnectingwithout setting an error.Files inspected
ui/src/stores/streamStoreHelpers.ts— production file under test (enumerated exports, tracedperformConnect→createConnectionAndHealth→setupWatchPath/setupPublishPath→schedulePostConnectWarnings).ui/src/stores/streamStoreHelpers.test.ts— existing test scaffolding (re-usedvi.mock('@moq/*')blocks,makeTrackfactory,createMockSignalhelper pattern, naming/sectioning style).Commands run
bun run test:run ui/src/stores/streamStoreHelpers.test.ts→ 81 passed (81) (64 pre-existing + 17 new)bun run lint→ exit 0, 6 pre-existing warnings, no new warnings or errorsFollow-ups / observations
defers audio publishing until the video catalog is readyassertsaudio.enabled.set(true)is called. The companion assertion (initialbroadcastConfig.audio.enabled === false) was not added to avoid asserting on the constructor-argument shape, which is a private contract betweensetupPublishPathandPublish.Broadcast. If the deferred-enable branch is intentionally broken in production, this test still fails (the.set(true)call never happens).applyWatchResult/applyPublishResult/applySecondaryPublishResult: These are not module exports and have no direct accessor — they're only reachable throughperformConnect. Their state mutations (attempt.watch / attempt.publish / attempt.secondaryPublish wiring) are covered indirectly via theperformConnecthappy-path tests that assert the resultingstate.isMicEnabled/state.isCameraEnabled/state.watchStatus. Adding direct tests would require exporting them or adding a__testInternals__hook in production code, which is out of scope per the "no production-code edits" rule.setupWatchPathteardown idempotency: Covered behaviorally by the connection-status-drop test (the disconnect path tears down both watch and publish viacleanupConnectAttempt, which the existing test suite already covers for idempotency). Adding a direct double-teardown test would also require exposingsetupWatchPath.Review & Testing Checklist for Human
ui/src/stores/streamStoreHelpers.test.tsfollow the same style as the existing test sections (vi.mock blocks at top, no parallel scaffolding, no.skip/.todo).streamStoreHelpers.ts(e.g. removingpublish.audio.enabled.set(true)from the deferred branch, or removing theset({ status: 'disconnected', errorMessage: ... })in the connection-drop handler) makes the corresponding test fail.bun run test:runlocally to verify the full UI suite still passes.Notes
Mocks for
@moq/hang,@moq/publish,@moq/watch,@moq/signalsare reused from the existing test file — no parallel mock setup was introduced. Constructor mocks usefunction () { return {...}; }rather than arrow functions becausenewinvocation requires[[Construct]]. Fake timers are scoped per-test viavi.useFakeTimers()/afterEach(vi.useRealTimers)for deterministic timing.Link to Devin session: https://staging.itsdev.in/sessions/9f38da412d794f6c83fe76b22153a19e
Requested by: @streamer45
Devin Review
ff366ff