Skip to content

test: improve Tempo widget test coverage and fix JSDOM mocks#5823

Merged
omsuneri merged 3 commits intosugarlabs:masterfrom
varruunnn:testtempo
Feb 21, 2026
Merged

test: improve Tempo widget test coverage and fix JSDOM mocks#5823
omsuneri merged 3 commits intosugarlabs:masterfrom
varruunnn:testtempo

Conversation

@varruunnn
Copy link
Contributor

Screenshot 2026-02-19 234334

Fixed Mocks: Refactored global.window and global.document mocks to directly attach to window.widgetWindows and HTMLCanvasElement.prototype to prevent wiping out the JSDOM environment.

UI Initialization: Added test blocks for init(activity) to verify DOM elements and event listeners (keyup) are created properly.

Canvas Rendering: Added tests for _draw() to verify the 2D context ellipse drawing and beat/synth triggers.

Tap Tempo: Added mock timer tests to verify the canvas onclick BPM calculation logic.
CLOSES #5822

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

Copy link
Contributor

@mahesh-09-12 mahesh-09-12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @varruunnn, nice improvement in coverage, and the JSDOM mock refactor is a good move. One thing to be careful about is isolation, since we’re attaching to HTMLCanvasElement.prototype and window.widgetWindows, we should ensure everything is reset between tests to avoid bleed-over. For init(activity), it would be stronger to trigger keyup and assert the actual side effect, not just listener presence. Similarly, _draw() and tap tempo tests would benefit from stricter behavioral assertions (call counts + edge timing cases) to better guard against regressions.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@varruunnn
Copy link
Contributor Author

Hey @mahesh-09-12, thanks for the great feedback! I’ve updated the PR to address all your points:
Test Isolation: Added jest.clearAllMocks() to the beforeEach block to prevent any state bleed over between tests.
Stronger init Test: Refactored the test to actually extract the listener, trigger the keyup event (keyCode 13), and assert that the _useBPM side effect occurs.
Strict Assertions: Tightened the _draw() and tap-tempo tests by adding explicit call counts (toHaveBeenCalledTimes(1)) to ensure strict behavioral checks and guard against visual regressions.
Let me know if there's anything else you'd like me to adjust!

Copy link
Contributor

@mahesh-09-12 mahesh-09-12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the feedback so thoroughly. The stronger behavioral assertions, proper mock isolation, and added boundary cases make this much safer from a regression standpoint. The init, draw, and tap-tempo paths are now covered in a meaningful way. Looks solid to me

@omsuneri omsuneri merged commit fc3ac97 into sugarlabs:master Feb 21, 2026
7 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.

Increase unit test coverage for the Tempo widget

3 participants