Skip to content

Conversation

@Commanderk3
Copy link
Member

This PR brings test suite for js/Saveinterface.js

@walterbender Please review this PR. A total of 30 tests are written covering all the functions.

image

@omsuneri
Copy link
Member

@walterbender @Commanderk3 I think this needd to be refactored once again before merging as there are many redundant mock activities

@Commanderk3
Copy link
Member Author

Okay I will change it. Thanks 👍

@Commanderk3
Copy link
Member Author

@walterbender Code is refactored. Please review the PR.

@Commanderk3
Copy link
Member Author

@omsuneri I followed your suggestion. Now there are less describe blocks

@omsuneri
Copy link
Member

@Commanderk3 seems good 👍

@omsuneri
Copy link
Member

@Commanderk3 its fine to have a multiple describe block in a test actually when i read a doc about test its always better to have a multiple describe as when a file is too large like block.js i m unable to set up all the functions' test in a single describe.

@walterbender
Copy link
Member

Is this expected?

Error: Not implemented: window.prompt

@Commanderk3
Copy link
Member Author

Is this expected?

Error: Not implemented: window.prompt

Yeah, but I will try to come up with a solution to avoid this error. The tests were passing so I kept this on low priority.

@walterbender
Copy link
Member

I think something like this will help:
Object.defineProperty(window, 'prompt', {
writable: true,
value: { assign: jest.fn() }
});

But then I see some other errors:
● download › should default filename to 'My Project.abc' if defaultfilename is null

@github-actions
Copy link
Contributor

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

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 22, 2025

I think something like this will help: Object.defineProperty(window, 'prompt', { writable: true, value: { assign: jest.fn() } });

But then I see some other errors: ● download › should default filename to 'My Project.abc' if the defaultfilename is null

@walterbender Thank you, it is working. The problem was with the prompt. It was breaking the code because Jest can't replicate the browser environment. Therefore when the prompt is triggered it sets the filename back to "undefined" from "My project.abc"

It should set the default file name as "My Project.abc" when no file name is passed.

@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

SaveInterface.test.js

@Commanderk3
Copy link
Member Author

I have to modify the test according to the new changes in master branch.

@Commanderk3 Commanderk3 marked this pull request as draft February 22, 2025 17:24
@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.

@walterbender
Copy link
Member

Anything else? It is still marked as DRAFT.

@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 22, 2025

Anything else? It is still marked as DRAFT.

Yeah, I am writing tests for Save MIDI.

@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 Commanderk3 marked this pull request as ready for review February 22, 2025 22:22
@Commanderk3
Copy link
Member Author

Commanderk3 commented Feb 22, 2025

@walterbender I am done with the tests. I had to mock the midi library, and it took some time to figure it out.

image

@walterbender walterbender merged commit 0903cef into sugarlabs:master Feb 23, 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.

3 participants