Skip to content

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)

@github-actions
Copy link
Contributor

❌ 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

@github-actions
Copy link
Contributor

❌ 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

@github-actions
Copy link
Contributor

❌ 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

@github-actions
Copy link
Contributor

❌ 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

@github-actions
Copy link
Contributor

❌ 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. !!

@github-actions
Copy link
Contributor

❌ 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
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

❌ 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

@github-actions
Copy link
Contributor

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

Failed Tests:

VolumeActions.test.js

@github-actions
Copy link
Contributor

❌ 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
@github-actions
Copy link
Contributor

❌ 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