Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714853005:
Convert remaining AudioBufferSource tests to testharness (Closed)
|
Created:
3 years, 10 months ago by Raymond Toy Modified:
3 years, 9 months ago Reviewers:
hongchan CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert remaining AudioBufferSource tests to testharness
Manually converted this test to testharness and new Audit. Decided to
leave these tests alone keeping the reference output wav file. This
ensures the finishAudioTest continues to work so that we can generate
wav files if necessary for future and current tests.
BUG=684010
TEST=audiobuffersource.html, audiobuffersource-loop-points.html,
audiobuffersource-multi-channels.html
Review-Url: https://codereview.chromium.org/2714853005
Cr-Commit-Position: refs/heads/master@{#458838}
Committed: https://chromium.googlesource.com/chromium/src/+/6e8a1e9931da82222b1661e4da3a45bf6aee7ebd
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments. #
Total comments: 4
Patch Set 3 : Remove use of finishAudioTest #
Total comments: 3
Messages
Total messages: 24 (6 generated)
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:99: let semitone = i - numberOfNotes / 2; // start three octaves down This comment can be between l.98 and l.99. https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: context.oncomplete = (event) => { Can we use promise? https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-multi-channels.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-multi-channels.html:37: context.oncomplete = (event) => { promise? https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:35: bufferLoader = new BufferLoader( Can this be replaced with Audit.loadFileFromURL()? We also can simplify another function at the bottom - by putting the function content inside of then() resolver. https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:57: context.oncomplete = (event) => { Use promise?
https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: context.oncomplete = (event) => { On 2017/02/27 18:05:34, hongchan wrote: > Can we use promise? Yes, I think we can, but we need to keep some tests around that test the oncomplete event.
https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: context.oncomplete = (event) => { On 2017/02/27 18:13:23, Raymond Toy wrote: > On 2017/02/27 18:05:34, hongchan wrote: > > Can we use promise? > > Yes, I think we can, but we need to keep some tests around that test the > oncomplete event. If we want to test that, that should be in the OAC's test - not here. I believe OAC's tests are already doing things with oncomplete event.
On 2017/02/27 18:19:22, hongchan wrote: > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html > (right): > > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: > context.oncomplete = (event) => { > On 2017/02/27 18:13:23, Raymond Toy wrote: > > On 2017/02/27 18:05:34, hongchan wrote: > > > Can we use promise? > > > > Yes, I think we can, but we need to keep some tests around that test the > > oncomplete event. > > If we want to test that, that should be in the OAC's test - not here. I believe > OAC's tests are already doing things with oncomplete event. OK. As long as we also have tests somewhere that use finishAudioTest and setAudioData. (Which we still do.)
On 2017/02/27 18:13:23, Raymond Toy wrote: > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html > (right): > > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: > context.oncomplete = (event) => { > On 2017/02/27 18:05:34, hongchan wrote: > > Can we use promise? > > Yes, I think we can, but we need to keep some tests around that test the > oncomplete event. Oops. finishAudioTest requires an event argument, which we won't have if we use a promise. I don't want to modify finishAudioTest here (because it's used in other tests), and adding a new function to support this doesn't seem right for this CL. Let's keep this and update/modify/refactor finishAudioTest in a follow-up CL. WDYT?
On 2017/02/27 18:42:00, Raymond Toy wrote: > On 2017/02/27 18:13:23, Raymond Toy wrote: > > > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > > File > > > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html > > (right): > > > > > https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... > > > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:107: > > context.oncomplete = (event) => { > > On 2017/02/27 18:05:34, hongchan wrote: > > > Can we use promise? > > > > Yes, I think we can, but we need to keep some tests around that test the > > oncomplete event. > > Oops. finishAudioTest requires an event argument, which we won't have if we use > a promise. I don't want to modify finishAudioTest here (because it's used in > other tests), and adding a new function to support this doesn't seem right for > this CL. > > Let's keep this and update/modify/refactor finishAudioTest in a follow-up CL. > > WDYT? Sounds good to me!
https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html (right): https://codereview.chromium.org/2714853005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:35: bufferLoader = new BufferLoader( On 2017/02/27 18:05:35, hongchan wrote: > Can this be replaced with Audit.loadFileFromURL()? We also can simplify another > function at the bottom - by putting the function content inside of then() > resolver. Done.
https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html (right): https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:38: bufferSource.buffer = bufferList[0]; Does this work? I think this should be: bufferSource.buffer = buffer; https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:48: }) A missing semicolon. https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:54: Remove l.53~l.54.
https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html (right): https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:38: bufferSource.buffer = bufferList[0]; On 2017/02/27 19:12:54, hongchan wrote: > Does this work? I think this should be: > > bufferSource.buffer = buffer; Uhoh. This conversion isn't working, even if I change this to buffer instead of bufferList[0]. I changed the expected file to a different file and the test did not fail.
On 2017/02/27 19:28:49, Raymond Toy wrote: > https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html > (right): > > https://codereview.chromium.org/2714853005/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource.html:38: > bufferSource.buffer = bufferList[0]; > On 2017/02/27 19:12:54, hongchan wrote: > > Does this work? I think this should be: > > > > bufferSource.buffer = buffer; > > Uhoh. This conversion isn't working, even if I change this to buffer instead of > bufferList[0]. > > I changed the expected file to a different file and the test did not fail. I think finishAudioTest cannot be used with testharness. testharnessreport calls testRunner.dumpAsText() after finishAudioTest sets dumap as audio. So the audio file is never tested.
lgtm with nits https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html (right): https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:93: .notThrow(); I don't think we should assert on OAC here. This test is not about OAC. https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:96: Audit.loadFileFromUrl('audiobuffersource-loop-points-expected.wav') This is about setting up the test. We don't have to assert on this. If the promise resolver triggers task.don(), it should be good enough. The code readability gets really murky with the pattern like this. (e.g. loading and decoding within the argument of assertion)
https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html (right): https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:93: .notThrow(); On 2017/03/03 16:01:34, hongchan wrote: > I don't think we should assert on OAC here. This test is not about OAC. This and below is my cheating way to get some useful message printed. Without these the output looks something like: PASS > [iniitalize] PASS < [iniitalize] 0 assertions passed. As an output, that's pretty useless and also makes you wonder why there were no assertions in the task. We can remove this task, of course, and move the body into the other task.
On 2017/03/03 17:40:50, Raymond Toy wrote: > https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html > (right): > > https://codereview.chromium.org/2714853005/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/audiobuffersource-loop-points.html:93: > .notThrow(); > On 2017/03/03 16:01:34, hongchan wrote: > > I don't think we should assert on OAC here. This test is not about OAC. > > This and below is my cheating way to get some useful message printed. Without > these the output looks something like: > > PASS > [iniitalize] > PASS < [iniitalize] 0 assertions passed. > > As an output, that's pretty useless and also makes you wonder why there were no > assertions in the task. > > We can remove this task, of course, and move the body into the other task. Offline discussion says adding these assertions is ok, as a way of not having a task with 0 assertions.
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490204672574370,
"parent_rev": "c5b15bc3b88b73c1e50b6ce10564daada4648650", "commit_rev":
"6e8a1e9931da82222b1661e4da3a45bf6aee7ebd"}
Message was sent while issue was closed.
Description was changed from ========== Convert remaining AudioBufferSource tests to testharness Manually converted this test to testharness and new Audit. Decided to leave these tests alone keeping the reference output wav file. This ensures the finishAudioTest continues to work so that we can generate wav files if necessary for future and current tests. BUG=684010 TEST=audiobuffersource.html, audiobuffersource-loop-points.html, audiobuffersource-multi-channels.html ========== to ========== Convert remaining AudioBufferSource tests to testharness Manually converted this test to testharness and new Audit. Decided to leave these tests alone keeping the reference output wav file. This ensures the finishAudioTest continues to work so that we can generate wav files if necessary for future and current tests. BUG=684010 TEST=audiobuffersource.html, audiobuffersource-loop-points.html, audiobuffersource-multi-channels.html Review-Url: https://codereview.chromium.org/2714853005 Cr-Commit-Position: refs/heads/master@{#458838} Committed: https://chromium.googlesource.com/chromium/src/+/6e8a1e9931da82222b1661e4da3a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6e8a1e9931da82222b1661e4da3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
