Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made musicutils.test.js #4124 #4140

Merged
merged 8 commits into from
Dec 14, 2024
Merged

Conversation

Commanderk3
Copy link
Contributor

@Commanderk3 Commanderk3 commented Dec 14, 2024

This PR brings the initialization of musicutils.test.js
It contains about 67 tests and covers 14 functions from musicutils.js

The rest of the portion will be done by @Aman1919 and @BeNikk as we are collaborating on this.

I had to paste the base64Encode function in musicutils.js (copied from base64Utils.js) . The base64Utils file uses a different export syntax than the rest (probably ES Module) therefor importing it caused some errors.

@walterbender Sir, kindy check this PR.

image

});
});
});
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this commented code is included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I made test functions for the "Temperament Management " block the results were not matched. Somehow the functions are not working.

IMG-20241214-WA0000.jpg

I did not want to edit musicutil.js so I left this job to @Aman1919

Copy link
Member

@walterbender walterbender Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please remove the commented code from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@walterbender this shows error while running the test
Screenshot 2024-12-14 at 10 12 19 PM

@walterbender walterbender merged commit c27aac7 into sugarlabs:master Dec 14, 2024
4 checks passed
@omsuneri
Copy link
Contributor

@Commanderk3 i think if you guyss are collaborting then make a draft PR as this might not confused the others to create a test and i had created a config and setup file for jest kindly use those two files to make sure the setup works fine with every test @walterbender can you review it again

@walterbender
Copy link
Member

I tested and saw no error. Maybe since your new files were added? I'll check again.

@omsuneri
Copy link
Contributor

It seems the same for me too @walterbender I ll make it correct if it happens with my files

@walterbender
Copy link
Member

@omsuneri your commit introduced the error on my system.

@omsuneri
Copy link
Contributor

Okay @walterbender I ll correct the test file
Give me some time to test

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