Skip to content

test: Fix Firefox CI hang in video spec (mock MediaStream)#255

Merged
HTMHell merged 1 commit into
masterfrom
fix/firefox-mediastream-spec
Jun 10, 2026
Merged

test: Fix Firefox CI hang in video spec (mock MediaStream)#255
HTMHell merged 1 commit into
masterfrom
fix/firefox-mediastream-spec

Conversation

@HTMHell

@HTMHell HTMHell commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Permanent fix for the Firefox CI failure that's blocking deploys.

src/app/core/modules/ngxs/store/video/video.spec.ts created a real new MediaStream() and then spyOn(...)-ed its native getVideoTracks (and a track's addEventListener). Firefox marks those native members non-configurable, so jasmine can't install the spy — it errors (getVideoTracks is non-configurable and can't be deleted) and hangs the browser ~93s until karma reports DISCONNECTED, failing the build job intermittently (Jasmine's randomized order surfaced the hang at different points).

Fix (matching rylo-translate): replace the real MediaStream/track with plain mock objects whose methods are jasmine.createSpy(...), so no native non-configurable property is patched. Verified video.spec.ts passes 10/10 on both Chrome and Firefox locally.

This keeps --browsers=ChromeHeadless,FirefoxHeadless in CI, so it supersedes #254 (the Chrome-only stopgap) — that PR can be closed.

Test plan

  • video.spec.ts passes on ChromeHeadless (10/10)
  • video.spec.ts passes on FirefoxHeadless locally (10/10, previously hung/disconnected)
  • CI build green on both browsers → deploy proceeds

Note

Low Risk
Test-only changes to mocking strategy; no runtime or security impact.

Overview
Fixes intermittent Firefox CI hangs in video.spec.ts by stopping use of spyOn on real MediaStream / track APIs.

StartCamera tests now use plain mock objects: getVideoTracks and track addEventListener are jasmine.createSpy instances on fake MediaStream / MediaStreamVideoTrack shapes (with getTracks stubbed where needed), instead of patching Firefox’s non-configurable native methods. Shared setup drops the global real MediaStream mock camera; one StopVideo case still seeds state with new MediaStream() where no spies are required. Minor test cleanup (inline error string expectations).

No production code changes—test-only CI stability so dual-browser Karma can stay enabled.

Reviewed by Cursor Bugbot for commit f4ce47e. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Tests
    • Improved unit test infrastructure for video state management with refined mock implementations and assertions.

Note: This release contains internal test improvements with no user-facing changes.

The video state spec created a real MediaStream and spied on its getVideoTracks
(and on a track's addEventListener). Firefox marks those native members
non-configurable, so jasmine could not install the spies — erroring and hanging
the browser until karma DISCONNECTed, intermittently failing CI. Replace the
real MediaStream/track with plain mock objects whose methods are spies, so no
native non-configurable property is patched (matches rylo-translate).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 36844b66-f342-4434-ad36-04812bcbf2bb

📥 Commits

Reviewing files that changed from the base of the PR and between 68a2741 and f4ce47e.

📒 Files selected for processing (1)
  • src/app/core/modules/ngxs/store/video/video.spec.ts

📝 Walkthrough

Walkthrough

This PR updates VideoState unit tests to standardize mock setup and assertion patterns. The changes remove a shared mockCamera variable, convert addEventListener to a Jasmine spy, refactor individual tests to build inline mocks, and align error message expectations across the test suite.

Changes

VideoState Test Standardization

Layer / File(s) Summary
Mock infrastructure and spy setup
src/app/core/modules/ngxs/store/video/video.spec.ts
MediaStreamVideoTrack mock now uses a Jasmine spy for addEventListener instead of a no-op function. Removed the shared mockCamera declaration and its beforeEach initialization to enforce inline mock construction within each test.
Error handling consistency
src/app/core/modules/ngxs/store/video/video.spec.ts
Error-path test now throws new Error('testError') with an exact message, and the error assertion checks state.video.error against that message directly.
StartCamera test refactoring
src/app/core/modules/ngxs/store/video/video.spec.ts
StartCamera tests now build inline mock cameras with getVideoTracks spies instead of modifying a pre-declared mock, and assert against the track's addEventListener spy directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • AmitMY

Poem

🐰 With spies in place and mocks inline,
No shared state to cross the line,
Each test now builds its own small stage,
Clean assertions grace the page!

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/firefox-mediastream-spec

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@HTMHell HTMHell merged commit d3e984c into master Jun 10, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant