Resolves all the errors with VolumeActions.test.js

[omsuneri]

@walterbender After changes in the VolumeActions.js there is failure of some of the tests in the test suite which are resolved and also add some more edge test cases to maximise the code coverage please review this.

[walterbender]

I am a bit confused here. Doesn't block_id need to be an int (an index into the blockList)? The blockList you are mocking has only 1 entry. And doesn't setMasterVolume call synth.setMasterVolume, which expects connections (with is a null list in your mock data.)

[omsuneri]

@walterbender right in noting that block_id should typically be an integer or an index into blockList, and that the setMasterVolume method involves calling synth.setMasterVolume, which expects connections. i m correcting this in next commit

[walterbender]

Also, I have been looking at the synthutils test, which has a few things wrong with it above and beyond Volume. Not sure how it ever worked properly.

[omsuneri]

Also, I have been looking at the synthutils test, which has a few things wrong with it above and beyond Volume. Not sure how it ever worked properly.

I m too running around many errors in the tests will look in to that too @walterbender thansk for pointing me out

[omsuneri]

@walterbender i had revised the changes you mention with blocklist and setmastervolume take a look over this
if you find some more changes need to be revised do let me know !!
also i m trying to refactor some of the test files which are causing failure of running test.

[omsuneri]

@walterbender do you need something else to be changed with this pr

[github-actions]

❌ 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

[walterbender]

        it("it should creates a PolySynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            __createSynth(turtle, "guitar", "guitar", {});
            expect(instruments[turtle]["electronic synth"]).toBeInstanceOf(Tone.PolySynth)
        });
        it("it should creates a PolySynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            __createSynth(turtle, "guitar", "sine", {});
            expect(instruments[turtle]["electronic synth"]).toBeInstanceOf(Tone.PolySynth)
        });
        it("it should creates a amsynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            const instrumentName = "guitar"
            __createSynth(turtle, instrumentName, "amsynth", {});
            expect(instruments[turtle][instrumentName]).toBeInstanceOf(Tone.AMSynth)
        });

This is wrong. A guitar is a Sampler, not AMSynth.
But I also don't understand the __createSynth calls.

[walterbender]

getVolume should be removed... it doesn't exist in synthUtils

[walterbender]

The calls to setMasterVolume needs first and last connection args.

[ac-mmi]

        it("it should creates a PolySynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            __createSynth(turtle, "guitar", "guitar", {});
            expect(instruments[turtle]["electronic synth"]).toBeInstanceOf(Tone.PolySynth)
        });
        it("it should creates a PolySynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            __createSynth(turtle, "guitar", "sine", {});
            expect(instruments[turtle]["electronic synth"]).toBeInstanceOf(Tone.PolySynth)
        });
        it("it should creates a amsynth based on the specified parameters, either using samples, built-in synths, or custom synths", () => {
            const instrumentName = "guitar"
            __createSynth(turtle, instrumentName, "amsynth", {});
            expect(instruments[turtle][instrumentName]).toBeInstanceOf(Tone.AMSynth)
        });

This is wrong. A guitar is a Sampler, not AMSynth. But I also don't understand the __createSynth calls.

@walterbender instruments[turtle][instrumentName] is likely holding state from a previous test.
so i think we should delete it then proceed to creating the appropriate synth instance (_createBuiltinSynth, _createCustomSynth, or _createSampleSynth)

I added this check before creating a new synth:

if (instruments[turtle] && instruments[turtle][instrumentName]) {
    delete instruments[turtle][instrumentName];
}

After making this change, all tests passed successfully.

[omsuneri]

@walterbender in this PR i had only resolved the errors regarding the volumeAction.test.js all the other errors you are mentioning are of synthutils i guess so is there any required changes i the volumeaction.test.js

[omsuneri]

also regarding the errors of synthUtils test i m resolving those too asap in another PR to keep reviewing easier @walterbender

[omsuneri]

The calls to setMasterVolume needs first and last connection args.

@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');
});

@ac-mmi I think you should comment this in your PR as this creates an unwanted confusion to me please !!

[walterbender]

OK. So just the VolumeActions here? I'll go ahead and merge.