Skip to content

Conversation

@Commanderk3
Copy link
Member

@Commanderk3 Commanderk3 commented Feb 26, 2025

@walterbender I have made tests for midi file. I brought getClosestStandardNoteValue to midi.js as it is solely used by it. This will improve maintenance and make it easier to write test cases.
Please review this PR.

image

@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.

@Commanderk3
Copy link
Member Author

Also in the previous PR for midi I forgot to correct the MAX_NOTEBLOCKS constant back to 100.

@walterbender
Copy link
Member

Hmm. I am seeing an error now with the midi section of the Save interface test.

FAIL js/tests/SaveInterface.test.js
● Test suite failed to run

Cannot find module '@tonejs/midi' from 'js/__tests__/SaveInterface.test.js'

  1 | const { Midi } = require('@tonejs/midi');
  2 | // Mocking the @tonejs/midi library
> 3 | jest.mock('@tonejs/midi', () => {
    |      ^
  4 |     return {
  5 |         Midi: jest.fn().mockImplementation(() => ({
  6 |             header: { ticksPerBeat: 480 },

  at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
  at Object.mock (js/__tests__/SaveInterface.test.js:3:6)

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 27, 2025

@walterbender Sir, you may have to install this module. You can do an npm i or npm i @tones/midi

image

That's why I think tests are passing in GitHub Actions

@github-actions
Copy link
Contributor

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

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 27, 2025

Importing the library itself is not a good idea. This should help.

image

Thank you for pointing it out.

@walterbender
Copy link
Member

OK. It should already be installed on the master branch.

@walterbender walterbender merged commit b9c16a0 into sugarlabs:master Feb 27, 2025
5 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.

2 participants