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

added unit test for p5.gain in es6 #462

Closed
wants to merge 2 commits into from

Conversation

endurance21
Copy link
Collaborator

@endurance21 endurance21 commented Apr 30, 2020

added unit tests for all the methods in p5.Gain
here are the test results and AUDIO GRAPH ( inspected with WEB AUDIO INSPECTOR) of this unit test
Selection_086

@endurance21
Copy link
Collaborator Author

@therewasaguy
please review this PR !
Also, i have been using this amazing WEB AUDIO INSPECTOR tool to see the AUDIO GRAPHS
, I find it very useful while working with web audio API, i am curious if we can use this as integrated part of our codebase, it will really help the developers to understand and write codes ,specially when they can see what they are creating!

@endurance21
Copy link
Collaborator Author

@therewasaguy we can add it too?

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

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

looks good just a couple questions

test/tests/p5.Gain.js Show resolved Hide resolved

mainGainNode.setInput(inputNode);
mainGainNode.connect(outputNode);
mainGainNode.disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

this is testing that we are able to call the disconnect method without that throwing any runtime errors, but how do we know if we successfully disconnected? (I'm not sure!)

Also, will this disconnect the inputNode or only the connection between the mainGaneNode and the outputNode? I think it's only that one. Again, how would we know from this test?

@endurance21 endurance21 requested a review from therewasaguy June 6, 2020 13:15
Base automatically changed from master to main March 5, 2021 02:53
Copy link
Contributor

@Abhijay007 Abhijay007 left a comment

Choose a reason for hiding this comment

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

Seems like the Test for p5.Gain.js is already updated and present in the repo, refer: https://github.com/processing/p5.js-sound/blob/main/test/tests/p5.Gain.js (last updated by @satyasaibhushan) ,

@davepagurek I think we can close this PR now.

@davepagurek
Copy link
Contributor

We may want to copy over just the can set the output level of gain Node test, since the current test file doesn't test changing the gain at all. But I think that's the only one that isn't covered now!

@Abhijay007
Copy link
Contributor

We may want to copy over just the can set the output level of gain Node test, since the current test file doesn't test changing the gain at all. But I think that's the only one that isn't covered now!

okay got it, so can I open a PR to add the can set the output level of gain Node in the existing gain test?

@davepagurek
Copy link
Contributor

yeah sounds good!

@Abhijay007
Copy link
Contributor

Abhijay007 commented Jan 3, 2023 via email

@davepagurek
Copy link
Contributor

Thanks @Abhijay007! I'll close this PR and carry over reviews on your new one.

@davepagurek davepagurek closed this Jan 4, 2023
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.

4 participants