test: introduce structured test infrastructure and refactor palette tests for deterministic CI

[mahesh-09-12]

Summary

This PR introduces a structured test infrastructure and refactors palette tests to improve reliability, isolation, and determinism.

The goal is to reduce flaky behavior caused by implicit globals, shared state, and loosely mocked DOM interactions — which are known issues in the current codebase and CI.

---

What’s Changed

1. Test Infrastructure

• Introduced reusable test utilities:
  • DOM mocks (docById, palette DOM factory)
  • Activity factory
  • SVG and Image mocks
• Added centralized test setup (setupTests.js)
• Removed duplicated inline mocks across tests

2. Jest Configuration

• Enabled setupFilesAfterEnv for consistent test initialization
• Used:
  • restoreMocks
• Avoided resetMocks (as it breaks required mock implementations)

3. Palette Test Refactor

• Migrated palette.test.js to use shared utilities
• Replaced implicit global dependencies with controlled setup
• Improved DOM handling using consistent mock structure
• Stabilized macro and async-related tests

4. Fix: Implicit Mock Dependency

• Fixed trash.test.js which relied on a leaked global mock of window.addEventListener
• Replaced with explicit jest.spyOn to ensure compatibility with restored mocks

---

Why This Matters

Previously:

• Tests relied on global state and implicit mocks
• Fake DOM structures caused unintended execution paths
• CI failures could differ from local runs

Now:

• Tests are isolated and deterministic
• Dependencies are explicit
• Behavior is consistent across environments

This lays the foundation for:

• Reliable regression testing
• Safe dependency upgrades
• Stable CI pipelines

---

Coverage Note

There is a minor drop (~1%) in functional coverage.

This is expected because:

• Previous tests unintentionally executed additional code paths due to global mocks and loosely defined DOM
• Refactored tests now only execute intended logic paths

No test cases were removed, and overall test reliability has improved.

---

PR Category

• Bug Fix
• Feature
• Performance
• Tests
• Documentation

[github-actions]

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

[mahesh-09-12]

Hi @walterbender @omsuneri , I've opened a PR introducing test infrastructure improvements and palette test refactoring.
whenever you have time, I'd appreciate your feedback. Thanks!

[mahesh-09-12]

Hi @walterbender @omsuneri,
I wanted to check if the overall approach here makes sense, especially around removing implicit globals and centralizing test setup for deterministic behavior. My goal is to ensure CI results remain consistent and reliable going forward, so I'd really value any feedback on whether this direction aligns with the project's testing strategy.

[github-actions]

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

[github-actions]

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

[github-actions]

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

[mahesh-09-12]

Rebased this PR on latest master and resolved conflicts in palette.test.js.
Aligned tests with upstream by using real DOM (jsdom) and removing remaining fake DOM structures. The setup has been adjusted to integrate cleanly with current behavior and improve determinism.

@walterbender @omsuneri could you please take a look when you have time?

[walterbender]

I like the overall approach. It seems that it could make our test infrastructure more consistent and easier to maintain. I defer to @omsuneri to hash out the details, but one question I had is why are PaletteGlobals in the utils folder? It seems specific to the palette tests themselves.

[mahesh-09-12]

@walterbender thanks for the feedback!

You're right - PaletteGlobals is currently specific to the palette tests. I introduced it while refactoring palette.test.js to remove implicit globals and keep the setup more structured.

I placed it under test/utils anticipating similar patterns might be needed for other tests, but I agree it may be better scoped closer to the palette tests for now.
Happy to move or reorganize it based on your preference.

[github-actions]

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

[mahesh-09-12]

@omsuneri just a gentle ping on this when you have time. Would appreciate your thoughts, especially around the test setup structure.

[github-actions]

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

[github-actions]

Merge conflicts resolved. Ready for review.

[github-actions]

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

[mahesh-09-12]

@omsuneri just wanted to share that I've rebased this PR on the latest master and addressed the merge conflicts.
Happy to make any adjustments if needed -- no rush.

[github-actions]

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

[mahesh-09-12]

@vanshika2720 thanks for the review and the detailed feedback! I'll go through the suggestions, make the necessary changes, and push an update shortly

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.99%. Comparing base (3999048) to head (1cc9e61).
⚠️ Report is 117 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6368   +/-   ##
=======================================
  Coverage   51.99%   51.99%           
=======================================
  Files         170      170           
  Lines       56871    56871           
=======================================
  Hits        29571    29571           
  Misses      27300    27300           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mahesh-09-12]

Hi @vanshika2720

I rebased the branch onto the latest upstream first to verify everything against the current codebase before addressing your suggestions.

For the first point, I moved PaletteGlobals back into palette.test.js since it was only used there. While revisiting that test, I also updated the myblocks macro test to explicitly initialize its required state so it remains self-contained.

During the rebase, a few existing tests also needed small adjustments because the shared test setup now uses restoreMocks. In particular, toolbar-ui.test.js and pitchstaircase.test.js were updated to recreate their document spies within the appropriate beforeEach, keeping their behavior the same while making them compatible with the new setup.

For the second point, I double-checked the migration from the previous jest.setup.js. The shared canvas mock is still provided through test/setupTests.js, and ErrorHandler is now mocked locally in the tests that depend on it rather than globally. I reran the full test suite after these changes, and everything is passing.

Thanks again for the suggestions!

[walterbender]

@vanshika2720 what do you think now?

[vanshika2720]

@mahesh-09-12 Nice refactoring overall. One concern replacing jest.setup.js seems to drop parts of the existing global test setup (ErrorHandler and the full canvas mock). Could we preserve the current setup or migrate it completely to avoid future test regressions?

[mahesh-09-12]

Thanks @vanshika2720 for pointing this out!

I completed the migration so the previous shared setup is now preserved in test/setupTests.js instead of being partially replaced. I migrated the global ErrorHandler and the full canvas mock, removed the now-redundant local ErrorHandler mocks from the affected tests, and deleted the obsolete jest.setup.js.

I reran the full test suite after these changes, and all tests are passing

[mahesh-09-12]

Hi @vanshika2720, whenever you have a chance, could you please take another look at the latest changes? I addressed your feedback and updated the PR accordingly. Thanks!

[vanshika2720]

@mahesh-09-12 The execution has two real problems. The setupFilesAfterEnv swap is an incomplete migration that degrades the global test environment going forward future test authors will hit missing mock methods with no indication why. The module-level spy in trash.test.js is left in place despite being the root cause of the test being fixed, and it now behaves differently from before due to restoreMocks: true being added. The second problem could cause order-dependent test failures in trash.test.js that are hard to diagnose.

[mahesh-09-12]

@vanshika2720 thanks for taking another look. I'm trying to understand these concerns. From the current revision, jest.setup.js was migrated into test/setupTests.js rather than partially replaced, and I don't see a module-level jest.spyOn remaining in trash.test.js, the spy is now created inside the test. Could you point me to the specific mock methods you think are still missing, and the location of the module-level spy you're referring to? That will help me address the right issue.

[vanshika2720]

since clearMocks: true and restoreMocks: true are already configured in jest.config.js, the explicit afterEach in test/setupTests.js calling jest.clearAllMocks() and jest.restoreAllMocks() appears redundant. Unless there's a specific reason for keeping both, relying on the Jest configuration alone would simplify the test setup.

[mahesh-09-12]

@vanshika2720 good catch! since clearMocks and restoreMocks are already enabled in jest.config.js, the explicit afterEach was redundant. I've removed it and rerun the test suite.

[vanshika2720]

@mahesh-09-12 I noticed after the restoreMocks: true migration is that a couple of tests still seem to rely on patterns that can leak state. The module-level window.addEventListener spy in trash.test.js appears redundant now that the resize test creates its own local spy, and the direct assignments to document.addEventListener/removeEventListener in palette.test.js won't be restored automatically by Jest. Would it make sense to remove the obsolete module-level spy and use jest.spyOn(...).mockImplementation(...) for the document listeners so cleanup is handled consistently?

[mahesh-09-12]

@vanshika2720 thanks for your patience and the detailed feedback! I addressed both points by removing the redundant module-level window.addEventListener spy in trash.test.js and replacing the direct document.addEventListener/removeEventListener assignments in palette.test.js with jest.spyOn(...).mockImplementation(...) so they're handled consistently with restoreMocks.