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

Fixed Tests for Turtle Singer and Synth Utils #4437

Merged
merged 21 commits into from
Feb 21, 2025

Conversation

ac-mmi
Copy link
Contributor

@ac-mmi ac-mmi commented Feb 20, 2025

This PR fixes issues with the tests for TurtleSinger, SynthUtils, and VolumeActions which i saw in my previous PR. There were some errors popping up due to undefined properties and some inconsistent mocks, so I cleaned that up.

In turtle-singer.test.js, I fixed TypeErrors by properly setting up blockList and connections. I also improved the mocking for Tone.Panner and updated test cases to cover more edge cases. In synthutils.test.js, I resolved issues with mocks causing errors and initialized activity.logo.synth correctly to avoid undefined errors.

In volumeactions.test.js, I fixed undefined connections by setting up blockList as follows:

blockList: {
       mockBlk: {
         connections: ['mockConnection1', 'mockConnection2']
         }
}

I ran the npm test command, and all the tests passed.

Screenshot (2585)

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@omsuneri
Copy link
Member

@ac-mmi try to look at the #4422 as this PR resolves all the errors of volumeaction also you are not supposed to make changes in the root file also try to folow our guide for implementing tests also i observed that you had changed multiple po files which need to be done in another PR

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 20, 2025

@omsuneri i have removed those po files. The changes that i have done in the root file in turtle-singer.js was this line
const lastConnection = activity.logo.blockList[blk].connections.slice(-1)[0] || null;
where earlier it used last() function but when i ran test case i got last() as undefined function.

Screenshot (2586)

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 20, 2025

#4422 (comment)
@walterbender I added the following to activityMock to ensure that setMasterVolume receives the first and last connection arguments correctly:

blockList: {
    mockBlk: {
        connections: ['mockConnection1', 'mockConnection2']
    }
}

I also updated the tests to ensure setMasterVolumeand setSynthVolume are called with the correct arguments.

test('should set master volume correctly', () => {
    Singer.setMasterVolume(logoMock, 50, 'mockBlk');
    expect(logoMock.synth.setMasterVolume).toHaveBeenCalledWith(50, 'mockConnection1', 'mockConnection2');
});

test('should set synth volume correctly', () => {
    Singer.setSynthVolume(logoMock, turtleMock, 'noise1', 80, 'mockBlk');
    expect(logoMock.synth.setVolume).toHaveBeenCalledWith(turtleMock, 'noise1', 80 / 25, 'mockBlk');
});

@omsuneri
Copy link
Member

omsuneri commented Feb 20, 2025

@ac-mmi try to mock the last as global.last = jest.fn((array) => array[array.length - 1]);
before the import of require

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@omsuneri
Copy link
Member

omsuneri commented Feb 20, 2025

@ac-mmi also try to remove the volumaction part as well as those errors are removed in the #4422 and also there i had added many new tests so kindly please do that tooo else it will create a conflict between that

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 20, 2025

@omsuneri Done

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@omsuneri
Copy link
Member

omsuneri commented Feb 20, 2025

@ac-mmi you dont need to merge the chnages of #4422 in your PR just simply remove the chnages you done for volumeaction.js and volumeaction.test.js dont copy those changes here
also try keep branch clean.
hope you understand my comment. !!

Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

1 similar comment
Copy link

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

Failed Tests:

synthutils.test.js
VolumeActions.test.js
turtle-singer.test.js

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 20, 2025

@omsuneri Alright i will take care of this in future

@omsuneri
Copy link
Member

@ac-mmi no issues now this looks good !!

@walterbender
Copy link
Member

please remove the unnecessary comments, such as // New additon

@ac-mmi ac-mmi requested a review from walterbender February 21, 2025 04:32
@walterbender
Copy link
Member

What are the .po.tmp files for?

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 21, 2025

@walterbender The .po.tmp files are simplified versions of the .po files with just the msgid and msgstr values, without any comments or extra details. I noticed this when working on PR #4433 , where I fixed an issue with "gong" word in quz.po. It had an empty msgstr, and the .tmp file stopped processing at that point. I think it’s meant to make processing easier but might get stuck on problematic entries.

@walterbender
Copy link
Member

Please make any changes to PO files in a separate PR. Also, the comments in the PO files are there to help the translators.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 21, 2025

@walterbender, I noticed the PR got closed, and I’m not sure why. Could you let me know the reason? I’d like to understand so I can learn from it and improve. If possible, I’d like to continue working on this. Thanks!

@walterbender walterbender reopened this Feb 21, 2025
@walterbender
Copy link
Member

It was closed by accident. Sorry about that. Fat fingers today.

Copy link

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

Failed Tests:

VolumeActions.test.js

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 21, 2025

@walterbender no problem

Copy link

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

Failed Tests:

VolumeActions.test.js

Copy link

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

Failed Tests:

VolumeActions.test.js

@ac-mmi ac-mmi requested a review from walterbender February 21, 2025 18:40
Copy link

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

Failed Tests:

VolumeActions.test.js

@walterbender walterbender merged commit 77c61f7 into sugarlabs:master Feb 21, 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