Test: expand musicutils core coverage by sapnilbiswas · Pull Request #6474 · sugarlabs/musicblocks

sapnilbiswas · 2026-04-02T22:47:06Z

Summary

Adds focused unit tests for musicutils.js core constants and mode utilities.

Changes

added new js/__tests__/utils/musicutils.test.js covering note names, solfege names, accidentals, and mode definitions
expanded js/utils/__tests__/musicutils.test.js to cover additional core lookup and mode-mapping branches
increased js/utils/musicutils.js statement coverage to 63.68%

Testing

npm test -- --coverage --runTestsByPath js/utils/__tests__/musicutils.test.js js/__tests__/utils/musicutils.test.js --collectCoverageFrom=js/utils/musicutils.js
npm test -- --json --outputFile=jest-results.json

PR category

Tests - Adds or updates test coverage

Fixes #6471

github-actions · 2026-04-02T22:48:35Z

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

github-actions · 2026-04-02T22:51:41Z

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

vanshika2720

@sapnilbiswas Some behaviors are being tested in multiple ways (VM + jest.requireActual), which adds redundancy and complexity. Also, globals aren’t cleaned up between tests.

Please simplify to one approach

github-actions · 2026-04-03T05:06:54Z

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

sapnilbiswas · 2026-04-03T05:10:03Z

Thanks, @vanshika2720, for the review. I have tried reducing the redundancy in the test

vanshika2720

@sapnilbiswas Thanks for the update and improved coverage! A few things still need attention:

Some duplicate test blocks — please clean those up
Mixed testing approaches — consider using a single consistent method
Globals aren’t cleaned between tests — add proper teardown
A few assertions are a bit brittle

github-actions · 2026-04-05T11:45:45Z

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

github-actions · 2026-04-05T11:48:14Z

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

Copilot

Pull request overview

This PR expands Jest unit test coverage for js/utils/musicutils.js, focusing on core music constants, scale/mode utilities, and additional lookup/mapping branches to reduce the risk of silent regressions in foundational music logic.

Changes:

Added a new test file to validate core constants (note names, solfege, accidentals, and mode definitions).
Expanded existing musicutils tests with additional cases for lookup fallbacks and mode/scale-mapping branches.
Increased coverage for js/utils/musicutils.js (per PR description).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`js/utils/__tests__/musicutils.test.js`	Adds helper-based global overrides and new test cases to hit additional fallback/mode-mapping branches.
`js/__tests__/utils/musicutils.test.js`	Introduces focused tests for core music constants and mode definitions by loading constants from `musicutils.js`.

Comments suppressed due to low confidence (1)

js/utils/tests/musicutils.test.js:1137

This withGlobalOverrides setup looks unused: getCustomNote references DOUBLEFLAT/DOUBLESHARP/FLAT/SHARP from the module scope in musicutils.js, not from global. Removing the override would reduce noise and avoid implying these symbols are configurable via globals.

    it("should handle array input by taking first element", () => {
        expect(getCustomNote(["C", "other"])).toBe("C");
        expect(getCustomNote(["D#", "other"])).toBe("D♯");
    });
    it("should convert double flat notation", () => {
        expect(getCustomNote("Cbb")).toBe("C𝄫");
        expect(getCustomNote("Dbb")).toBe("D𝄫");

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-04-05T12:21:07Z

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

vanshika2720

@sapnilbiswas Looks much cleaner, thanks!
but still above issue remains

github-actions · 2026-04-05T16:27:17Z

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

sapnilbiswas · 2026-04-05T16:28:19Z

Thank you for the follow-up review. I did one more small cleanup pass that only focused on making things consistent:

made the last global-heavy setup blocks in js/utils/__tests__/musicutils.test.js use the shared override helper.
kept the claims and coverage the same
stayed away from any changes to the production code or bigger refactors

Checked again locally after the update:

full Jest suite passes (125 suites and 4028 tests)
The targeted coverage for musicutils.js is still above the goal for the issue, which is 68.56% statements and 60.73% branches.

When you have a chance, please look again.

sapnilbiswas · 2026-04-05T16:29:01Z

Please let me know if something specifically needed

sapnilbiswas · 2026-04-06T16:11:52Z

@walterbender Sir can you please review my PR and let me know if there is anything i am missing at

vanshika2720

Hi @sapnilbiswas, this PR seems to be a duplicate of #6470. We should probably consolidate our efforts there to keep the PR history clean!

github-actions · 2026-04-18T08:45:58Z

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

…ated suites

github-actions · 2026-04-18T08:59:42Z

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

github-actions · 2026-04-18T09:08:29Z

Merge conflicts resolved. Ready for review.

github-actions · 2026-04-18T09:09:22Z

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

github-actions · 2026-04-18T09:15:25Z

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

sapnilbiswas · 2026-04-18T09:19:39Z

@Ashutoshx7 @vanshika2720 I have tried resolving all the scope creep and making it clean. Could you please help me with reviewing it

…e redundant file

github-actions · 2026-04-18T09:32:01Z

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

Ashutoshx7 · 2026-04-18T09:33:57Z

@Ashutoshx7 @vanshika2720 I have tried resolving all the scope creep and making it clean. Could you please help me with reviewing it

reviewing it right away

github-actions · 2026-04-18T09:39:51Z

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

Ashutoshx7 · 2026-04-18T09:52:08Z

are u stil working on this
i can see the recent commit

sapnilbiswas · 2026-04-18T10:02:35Z

are u stil working on this i can see the recent commit

I am done with it. You can go ahead with reviewing

Ashutoshx7 · 2026-04-18T12:53:36Z

This PR is extremely clean, addresses redundancy in the test suite as requested during review, and follows all repository patterns. I am 100% confident that this is ready for merge.
@walterbender
and we can close #6470

sapnilbiswas · 2026-04-18T12:54:46Z

This PR is extremely clean, addresses redundancy in the test suite as requested during review, and follows all repository patterns. I am 100% confident that this is ready for merge. @walterbender and we can close #6470

Thanks @Ashutoshx7 for reviewing this

github-actions Bot added tests Adds or updates test coverage size/XL Extra large: 500-999 lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Apr 2, 2026

sapnilbiswas force-pushed the musicutils-core-tests branch from e751d6c to ea0ca4b Compare April 2, 2026 22:50

vanshika2720 suggested changes Apr 3, 2026

View reviewed changes

github-actions Bot added size/L Large: 250-499 lines changed and removed size/XL Extra large: 500-999 lines changed labels Apr 3, 2026

sapnilbiswas requested a review from vanshika2720 April 3, 2026 20:36

vanshika2720 reviewed Apr 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings April 5, 2026 11:44

Copilot started reviewing on behalf of sapnilbiswas April 5, 2026 11:45 View session

github-actions Bot added size/XL Extra large: 500-999 lines changed and removed size/L Large: 250-499 lines changed labels Apr 5, 2026

Copilot AI reviewed Apr 5, 2026

View reviewed changes

Comment thread js/utils/__tests__/musicutils.test.js Outdated

Comment thread js/utils/__tests__/musicutils.test.js Outdated

Comment thread js/utils/__tests__/musicutils.test.js Outdated

Comment thread js/__tests__/utils/musicutils.test.js Outdated

sapnilbiswas requested a review from vanshika2720 April 5, 2026 12:26

vanshika2720 reviewed Apr 5, 2026

View reviewed changes

sapnilbiswas requested a review from vanshika2720 April 6, 2026 16:06

vanshika2720 reviewed Apr 9, 2026

View reviewed changes

sapnilbiswas marked this pull request as draft April 18, 2026 08:53

test: surgical expansion of musicutils coverage and cleanup of duplic…

5e86191

…ated suites

sapnilbiswas force-pushed the musicutils-core-tests branch from 70347d5 to 5e86191 Compare April 18, 2026 08:58

github-actions Bot added size/M Medium: 50-249 lines changed and removed size/XXL XXL: 1000+ lines changed labels Apr 18, 2026

chore: resolve merge conflict and adopt upstream tests

f58ab9d

github-actions Bot removed the needs-rebase label Apr 18, 2026

style: fix formatting in musicutils.test.js

4cca7bc

github-actions Bot added size/L Large: 250-499 lines changed and removed size/M Medium: 50-249 lines changed labels Apr 18, 2026

sapnilbiswas requested a review from vanshika2720 April 18, 2026 09:19

sapnilbiswas marked this pull request as ready for review April 18, 2026 09:19

test: merge core constants tests into main musicutils suite and delet…

7dd280f

…e redundant file

test: remove redundant test file js/__tests__/utils/musicutils.test.js

c0dba34

Ashutoshx7 approved these changes Apr 18, 2026

View reviewed changes

walterbender merged commit 5ad1c71 into sugarlabs:master Apr 18, 2026
10 checks passed

CodeLine9 mentioned this pull request Apr 19, 2026

[Bug]musicutils.js has zero Jest test coverage — regression risk for core music functions #6469

Closed

Uh oh!

Conversation

sapnilbiswas commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

PR category

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

vanshika2720 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 3, 2026

Uh oh!

sapnilbiswas commented Apr 3, 2026

Uh oh!

vanshika2720 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 5, 2026

Uh oh!

github-actions Bot commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 5, 2026

Uh oh!

vanshika2720 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 5, 2026

Uh oh!

sapnilbiswas commented Apr 5, 2026

Uh oh!

sapnilbiswas commented Apr 5, 2026

Uh oh!

sapnilbiswas commented Apr 6, 2026

Uh oh!

vanshika2720 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

sapnilbiswas commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

Ashutoshx7 commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

Ashutoshx7 commented Apr 18, 2026

Uh oh!

sapnilbiswas commented Apr 18, 2026

Uh oh!

Ashutoshx7 commented Apr 18, 2026

Uh oh!

sapnilbiswas commented Apr 18, 2026

Uh oh!

sapnilbiswas commented Apr 2, 2026 •

edited

Loading

vanshika2720 left a comment •

edited

Loading

vanshika2720 left a comment •

edited

Loading