Convert Oscillator sweep tests to testharness

Convert Oscillator sweep tests to testharness

Manually converted tests to use testharnees and new Audit.

BUG=688414
TEST=osc-*-sweep-snr.html

Review-Url: https://codereview.chromium.org/2675973003
Cr-Commit-Position: refs/heads/master@{#448782}
Committed: https://chromium.googlesource.com/chromium/src/+/9a3a1bc17dc95a2a6db144ef66919cc6e1e18693

[Raymond Toy, 2017-02-03 18:40:08]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2017-02-03 18:40:09]

PTAL

[hongchan, 2017-02-06 17:28:43]

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:25: // 
<script src="../resources/js-test.js"></script>
This comment might have to be updated.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
Not really sure about this pattern. Audit should be created the main scope, not
in the method. Generally I want these files to do not know each other.

However, if this is absolutely necessary, OscillatorTestingUtils can get the
instance of task runner via some method:
OscillatorTestingUtils.setTaskRunner(auditTaskRunner);

I agree with the idea of minimizing CL, but we should consider the better
abstraction of test helpers.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:114:
renderedData = renderedBuffer.getChannelData(0);
Why isn't this a local variable? Any reason being global?

[Raymond Toy, 2017-02-06 18:36:18]

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:25: // 
<script src="../resources/js-test.js"></script>
On 2017/02/06 17:28:43, hongchan wrote:
> This comment might have to be updated.

I'll have to retest. Other than the paths being wrong, I think this will still
work.  I suspect it won't work with testharness.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
On 2017/02/06 17:28:43, hongchan wrote:
> Not really sure about this pattern. Audit should be created the main scope,
not
> in the method. Generally I want these files to do not know each other.
> 
> However, if this is absolutely necessary, OscillatorTestingUtils can get the
> instance of task runner via some method:
> OscillatorTestingUtils.setTaskRunner(auditTaskRunner);
> 
> I agree with the idea of minimizing CL, but we should consider the better
> abstraction of test helpers.

Yeah, I debated on whether to bury everything here, or move it to the caller. 
I'll try that out.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:114:
renderedData = renderedBuffer.getChannelData(0);
On 2017/02/06 17:28:43, hongchan wrote:
> Why isn't this a local variable? Any reason being global?

Probably to make debugging easier so that it could be more easily examined in
the dev console.

[Raymond Toy, 2017-02-07 00:16:28]

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
On 2017/02/06 18:36:18, Raymond Toy wrote:
> On 2017/02/06 17:28:43, hongchan wrote:
> > Not really sure about this pattern. Audit should be created the main scope,
> not
> > in the method. Generally I want these files to do not know each other.
> > 
> > However, if this is absolutely necessary, OscillatorTestingUtils can get the
> > instance of task runner via some method:
> > OscillatorTestingUtils.setTaskRunner(auditTaskRunner);
> > 
> > I agree with the idea of minimizing CL, but we should consider the better
> > abstraction of test helpers.
> 
> Yeah, I debated on whether to bury everything here, or move it to the caller. 
> I'll try that out.

Done.

[hongchan, 2017-02-07 18:18:53]

I think it's probably my fault, but we have to clarify what we are trying to do
here. You are doing a bit of refactoring, but is that what we want? Perhaps we
should just focus on making it work and leave other parts as they are.

It's up to you, but the partial refactoring in this CL is not particularly
helpful. I think it takes a bit of work to completely detach audit from these
utility functions.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:25: // 
<script src="../resources/js-test.js"></script>
On 2017/02/06 18:36:18, Raymond Toy wrote:
> On 2017/02/06 17:28:43, hongchan wrote:
> > This comment might have to be updated.
> 
> I'll have to retest. Other than the paths being wrong, I think this will still
> work.  I suspect it won't work with testharness.

Does this work with 'js-test' at all? What I meant was to replace js-test with
the new audit and the testharness.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:114:
renderedData = renderedBuffer.getChannelData(0);
On 2017/02/06 18:36:18, Raymond Toy wrote:
> On 2017/02/06 17:28:43, hongchan wrote:
> > Why isn't this a local variable? Any reason being global?
> 
> Probably to make debugging easier so that it could be more easily examined in
> the dev console.

I think it's easy to put 'let' at the beginning. No?

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/Oscillator/osc-custom-sweep-snr.html
(right):

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/Oscillator/osc-custom-sweep-snr.html:16:
var tester = OscillatorTestingUtils;
var => let.

You changed this in other tests. So let's be consistent.

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
Not sure what's fixed here.

[Raymond Toy, 2017-02-07 18:57:58]

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:25: // 
<script src="../resources/js-test.js"></script>
On 2017/02/07 18:18:53, hongchan wrote:
> On 2017/02/06 18:36:18, Raymond Toy wrote:
> > On 2017/02/06 17:28:43, hongchan wrote:
> > > This comment might have to be updated.
> > 
> > I'll have to retest. Other than the paths being wrong, I think this will
still
> > work.  I suspect it won't work with testharness.
> 
> Does this work with 'js-test' at all? What I meant was to replace js-test with
> the new audit and the testharness.

You mean this bit of commented-out code? It used to work.  Haven't had to update
the expected results so I haven't tried it in a long time.

https://codereview.chromium.org/2675973003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:114:
renderedData = renderedBuffer.getChannelData(0);
On 2017/02/07 18:18:53, hongchan wrote:
> On 2017/02/06 18:36:18, Raymond Toy wrote:
> > On 2017/02/06 17:28:43, hongchan wrote:
> > > Why isn't this a local variable? Any reason being global?
> > 
> > Probably to make debugging easier so that it could be more easily examined
in
> > the dev console.
> 
> I think it's easy to put 'let' at the beginning. No?

You mean make renderedData truly local to checkResult?  Yes, it can be done.

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js
(right):

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
On 2017/02/07 18:18:53, hongchan wrote:
> Not sure what's fixed here.

Oops. Forgot to remove this.

https://codereview.chromium.org/2675973003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/oscillator-testing.js:38: var
audit = Audit.createTaskRunner();
On 2017/02/07 18:18:53, hongchan wrote:
> Not sure what's fixed here.

Forgot to remove it.

[hongchan, 2017-02-07 21:56:09]

lgtm

[Raymond Toy, 2017-02-07 22:06:12]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-02-07 22:07:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 23:25:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486505172923860,
"parent_rev": "b64a14bf64e5dc186e75515c1754839f731df15c", "commit_rev":
"9a3a1bc17dc95a2a6db144ef66919cc6e1e18693"}

[commit-bot: I haz the power, 2017-02-07 23:26:12]

Description was changed from

==========
Convert Oscillator sweep tests to testharness

Manually converted tests to use testharnees and new Audit.

BUG=688414
TEST=osc-*-sweep-snr.html
==========

to

==========
Convert Oscillator sweep tests to testharness

Manually converted tests to use testharnees and new Audit.

BUG=688414
TEST=osc-*-sweep-snr.html

Review-Url: https://codereview.chromium.org/2675973003
Cr-Commit-Position: refs/heads/master@{#448782}
Committed:
https://chromium.googlesource.com/chromium/src/+/9a3a1bc17dc95a2a6db144ef6691...
==========

[commit-bot: I haz the power, 2017-02-07 23:26:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9a3a1bc17dc95a2a6db144ef6691...