-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
modernized the TESTING architecture of the codebase { GSOC} #541
Conversation
@therewasaguy @kyle1james for now i have removed sinon.js ,from test/testDeps/sinon.js , i would like to hear some suggestions about its requirements for unit testing now and in future! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thus reducing the size of the codebase
Aside from a couple minor comments, the only real issue is that this isn't working on Firefox (79.0). Getting this error
Uncaught DOMException: AudioWorkletNode constructor: Unknown AudioWorklet name 'sound-file-processor'
I suspect it has something to do with the way we're importing the audio worklet modules inline with raw-loader here
p5.js-sound/src/audioWorklet/index.js
Lines 2 to 6 in 91d8b34
const moduleSources = [ | |
require('raw-loader!./recorderProcessor').default, | |
require('raw-loader!./soundFileProcessor').default, | |
require('raw-loader!./amplitudeProcessor').default, | |
]; |
It's possible that raw-loader doesn't work well with firefox <script ... type="module">
Hoping for a quick fix, I tried upgrading the raw-loader dependency to 4.0.1 but that doesn't seem to resolve the issue...
UPDATE
seems like raw-loader is working as expected, it's just loading the modules after we need them
p5.js-sound/src/audioWorklet/index.js
Lines 21 to 34 in 91d8b34
p5.prototype.registerMethod('init', function () { | |
if (initializedAudioWorklets) return; | |
// ensure that a preload function exists so that p5 will wait for preloads to finish | |
if (!this.preload && !window.preload) { | |
this.preload = function () {}; | |
} | |
// use p5's preload system to load necessary AudioWorklet modules before setup() | |
this._incrementPreload(); | |
const onWorkletModulesLoad = function () { | |
initializedAudioWorklets = true; | |
this._decrementPreload(); | |
}.bind(this); | |
loadAudioWorkletModules().then(onWorkletModulesLoad); |
alright, I think the issue with Firefox is actually an issue with how we've structured some of the tests. For example, I've commented out all of the We can fix this by creating the amplitude as part of the test lifecycle, wrapping this p5.js-sound/test/tests/p5.SoundFile.js Line 10 in 91d8b34
in a
|
…strict declaration
@therewasaguy added sinon.js for as it was used inside test/tests/p5.soundRecorder.js alos removed |
…e to ensure proper loading of AudioWorklet modules for * sounFile processor * Oscillator processor * Amplitude processor * test/index.js - then used dynamic import to import the tests after each of worklet processors have been loaded successfully
@therewasaguy i have solved the problem , actually, there was no way we could be able to know when all AudioWorkletProcessor will load finally so I have attached a new the tests are running fine in firefox too ! and issue seems to be resolved ! |
@therewasaguy hey! it been long seeing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @endurance21 all of this looks great, thanks!
i'm not familiar with the changes you are doing,
and i don't know how to test it, so i will let someone else merge it :)
Hey @endurance21, the intention of the At the time, there was no way to ask p5 to wait for an arbitrary promise before running Is it possible for you to just use the p5 Alternatively, if p5 has better support for promises these days, maybe you could replace the preloading code entirely? |
@oshoham thanks a ton for the review, I need to give it a shot by using preload hook and see if that helps. |
@oshoham it worked that way! thanks again! |
@therewasaguy i have tested on firefox and chrome too, tests are running smooth now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work @endurance21 !
<link rel="stylesheet" href="../node_modules/mocha/mocha.css" /> | ||
<script src="../node_modules/mocha/mocha.js"></script> | ||
<script src="../node_modules/chai/chai.js"></script> | ||
<script src="../node_modules/sinon/pkg/sinon.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're dependent on the file structure of node_modules here, just an observation, but I do think this is better than what we had before !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
earlier those files are stored with fixed version in codebase , now we use npm install and it downloads from npm for every codebase setup!
that way we just reduced file size of our codebase!
MORE CLEANER AND INTUITIVE TESTING ARCHITECTURE
getting rid of requirejs .finally !
used es6 imports
used npm to download "chai" and "mocha" and removed the cached version in test/testDeps/chai.js , test/testDeps/mocha.js
thus reducing the
size
of the codebase !can be further linked to karma.js for headless testing ! { future goals}
can be further used for integrating in CI environment !