Add tests for LegoWidget core logic (#6344)#6503
Add tests for LegoWidget core logic (#6344)#6503021nirav-blip wants to merge 6 commits intosugarlabs:masterfrom
Conversation
|
✅ All Jest tests passed! This PR is ready to merge. |
github-actions
bot
added
tests
The js/widgets directory had almost no tests. Now it does. Tests cover: _calculateFallbackFrequency() — A4=440Hz, all pitches/octaves, edge cases addRowBlock() — insertion, duplicates, internal state doesn't break clearBlocks() — actually resets the array Integration between methods Had to add a module export to legobricks.js so tests could import it. That's it. No DOM, no randomness, just deterministic tests. Fixes sugarlabs#6344
021nirav-blip
force-pushed
the
Test-Coverage-for-LegoWidget
branch
from
0ce0983 to
cd2e988
Compare
|
✅ All Jest tests passed! This PR is ready to merge. |
| /** | ||
| * MusicBlocks v3.6.2 | ||
| * | ||
| * @author Test Implementation |
There was a problem hiding this comment.
author Test Implementation?
request a re-review after you have scoped the PR changes
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
vanshika2720
left a comment
vanshika2720
left a comment
There was a problem hiding this comment.
@021nirav-blip avoid testing internal properties (_rowBlocks, _rowMap, etc.) directly prefer testing behavior over implementation details.
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
|
@vanshika2720 Options: Which do you prefer? |
There was a problem hiding this comment.
@021nirav-blip (b). Don’t add getters just for tests avoid testing internals. Keep the pure function tests and, if possible, add behavior-level tests without exposing private state.
github-actions
bot
added
size/M
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@vanshika2720 - I dropped the internal property tests like you said, so only _calculateFallbackFrequency() has coverage at the moment. The problem is #6344 wants addRowBlock() and clearBlocks() covered too. Since there aren't any public APIs for them, we can't really see if they're working without peeking at the internal state. Should we just live with the lower coverage, or should I add some getters so the tests have something to hook into? Or maybe you had a different idea? |
vanshika2720
left a comment
vanshika2720
left a comment
There was a problem hiding this comment.
@021nirav-blip Keep only _calculateFallbackFrequency() tests. Don’t add getters just for testing.
If addRowBlock() / clearBlocks() have no public behavior, it’s fine to leave them uncovered.
3 participants
The js/widgets directory had almost no tests. Now it does.
Tests cover:
_calculateFallbackFrequency() — A4=440Hz, all pitches/octaves, edge cases
addRowBlock() — insertion, duplicates, internal state doesn't break
clearBlocks() — actually resets the array
Integration between methods
Had to add a module export to legobricks.js so tests could import it. That's it.
No DOM, no randomness, just deterministic tests.
Fixes #6344
PR Category