fix(tests): stabilize tests by mocking core and controlling timers #2
Conversation
Resolved two main issues causing test failures: 1. Race condition from `setTimeout(..., 0)` during player init. 2. Tight coupling to internal refs not exposed by the component. Fixes: - Mocked `@interactive-video-labs/core` to isolate component behavior. - Used `vi.useFakeTimers()` to control async timing. - Exposed `playerRef` via setup to improve test accessibility.
WalkthroughA new Vue 3 component, Changes
Sequence Diagram(s)sequenceDiagram
participant App as Vue App
participant IV as InteractiveVideo (component)
participant Player as IVLabsPlayer
App->>IV: Mounts with props (videoUrl, cues, translations, etc.)
IV->>Player: Initializes IVLabsPlayer with config
Player-->>IV: Emits analytics event
IV-->>App: Calls onAnalyticsEvent callback (if provided)
IV->>Player: Loads cues and translations (if provided)
App->>IV: Updates cues or translations props
IV->>Player: Updates cues/translations
App->>IV: Unmounts
IV->>Player: Destroys player instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/InteractiveVideo.test.ts (1)
69-76: Verify the event callback extraction logic.The test assumes the
PLAYER_LOADEDevent listener is the first matching call, but this could be fragile if event registration order changes.Consider making the event callback extraction more robust:
-// Simulate an event being fired by the player -const eventCallback = (mockPlayerInstance.on as any).mock.calls.find(call => call[0] === 'PLAYER_LOADED')[1]; -eventCallback({ some: 'payload' }); +// Simulate an event being fired by the player +const playerLoadedCall = (mockPlayerInstance.on as any).mock.calls.find(call => call[0] === 'PLAYER_LOADED'); +expect(playerLoadedCall).toBeDefined(); +const eventCallback = playerLoadedCall[1]; +eventCallback({ some: 'payload' });src/index.ts (3)
42-66: Consider eliminating the setTimeout to fully resolve race conditions.While the tests now control this with fake timers, the
setTimeout(..., 0)pattern was identified as causing race conditions in the PR objectives. Consider usingnextTick()or checking DOM readiness more directly.Replace the setTimeout pattern with Vue's nextTick:
- // Use a timeout to ensure the element is in the DOM - setTimeout(() => { - if (containerRef.value) { - const player = new IVLabsPlayer(`#${uniqueId}`, playerConfig); - // ... rest of initialization - } - }, 0); + // Use nextTick to ensure the element is in the DOM + await nextTick(); + if (containerRef.value) { + const player = new IVLabsPlayer(`#${uniqueId}`, playerConfig); + // ... rest of initialization + }Note: You'll need to make the onMounted callback async and import
nextTickfrom Vue.
48-56: Reduce repetitive event listener code.The event listener setup is repetitive and could be simplified for better maintainability.
Consider using a more concise approach:
- if (props.onAnalyticsEvent) { - player.on('PLAYER_LOADED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('PLAYER_LOADED', payload)); - player.on('VIDEO_STARTED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('VIDEO_STARTED', payload)); - player.on('VIDEO_PAUSED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('VIDEO_PAUSED', payload)); - player.on('VIDEO_ENDED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('VIDEO_ENDED', payload)); - player.on('CUE_TRIGGERED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('CUE_TRIGGERED', payload)); - player.on('INTERACTION_COMPLETED', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('INTERACTION_COMPLETED', payload)); - player.on('ERROR', (payload?: AnalyticsPayload) => (props.onAnalyticsEvent as Function)('ERROR', payload)); - } + if (props.onAnalyticsEvent) { + const events: AnalyticsEvent[] = [ + 'PLAYER_LOADED', 'VIDEO_STARTED', 'VIDEO_PAUSED', 'VIDEO_ENDED', + 'CUE_TRIGGERED', 'INTERACTION_COMPLETED', 'ERROR' + ]; + events.forEach(event => { + player.on(event, (payload?: AnalyticsPayload) => + props.onAnalyticsEvent!(event, payload) + ); + }); + }
28-28: Improve unique ID generation robustness.The current ID generation using
Math.random()could theoretically produce collisions.Consider using a more robust approach:
- const uniqueId = `ivlabs-player-${Math.random().toString(36).substr(2, 9)}`; + const uniqueId = `ivlabs-player-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.json(2 hunks)src/index.ts(1 hunks)test/InteractiveVideo.test.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
🔇 Additional comments (7)
tsconfig.json (1)
5-5: LGTM! Module resolution change aligns with ES module approach.The change from "Node" to "bundler" module resolution properly supports the project's ES module configuration and modern bundling practices.
vitest.config.ts (2)
3-3: Good addition of Vue plugin support.The Vue plugin integration enables proper Single File Component compilation during testing.
8-9: Appropriate test environment configuration.The combination of
globals: trueandjsdomenvironment properly supports Vue component testing by providing global test APIs and DOM simulation.package.json (1)
4-4: Good addition of ES module support.Setting
"type": "module"properly configures the package for ES module semantics, aligning with the TypeScript configuration changes.test/InteractiveVideo.test.ts (2)
6-18: Excellent mocking strategy for test isolation.The comprehensive mock of
@interactive-video-labs/coreproperly isolates the component during testing, addressing the tight coupling issues mentioned in the PR objectives.
21-28: Proper timer control implementation.The use of fake timers in setup/teardown directly addresses the race condition issues caused by
setTimeout(..., 0)mentioned in the PR objectives.src/index.ts (1)
25-27: Good exposure of playerRef for testing.Exposing
playerRefvia the setup function addresses the tight coupling issues mentioned in the PR objectives, making tests less fragile.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Resolved two main issues causing test failures:
setTimeout(..., 0)during player init.Fixes:
@interactive-video-labs/coreto isolate component behavior.vi.useFakeTimers()to control async timing.playerRefvia setup to improve test accessibility.Summary by CodeRabbit
New Features
Tests
Chores