Convert MediaStreamAudioSource test to testharness

Convert MediaStreamAudioSource test to testharness

Manually convert the test to use testharness and new Audit.

BUG=692724
TEST=mediastreamaudiosourcenode.html

Review-Url: https://codereview.chromium.org/2690343005
Cr-Commit-Position: refs/heads/master@{#455182}
Committed: https://chromium.googlesource.com/chromium/src/+/8f98597ba6a3add1e39d0f824fc8e96ee4f663b7

[Raymond Toy, 2017-02-24 17:34:22]

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

[Raymond Toy, 2017-02-24 17:34:23]

PTAL

[hongchan, 2017-02-24 23:28:09]

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html
(right):

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html:34:
.notThrow();
If clang-format did this, it is really bad. I would rewrite this like:

audit.define({
    label: 'test',
    description: 'Basic tests for MediaStreamAudioSourceNode API'
  }, (task, should) => {
  
    let successOrErrorCallback = funtion (callbackArg) {
      let isPassed;
      /* do something with callbackArg */
      should(isPassed, 'Stream generation')
          .message('succeded', 'failed');
      task.done();
    };

    should(() => { 
      navigator.webkitGetUserMedia(
          { audio: true },
          successOrErrorCallback,
          successOrErrorCallback);
    });
  }
);

I don't think we have to test if the getUserMedia() call throws. It is not our
feature to test.

[Raymond Toy, 2017-02-24 23:35:30]

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html
(right):

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html:34:
.notThrow();
On 2017/02/24 23:28:09, hongchan wrote:
> If clang-format did this, it is really bad. I would rewrite this like:

I think it's really ugly too, but we discussed this the yesterday and you said
it was ok.

However, we should either use clang-format and accept it as is, or not use
clang-format at all.

> 
> audit.define({
>     label: 'test',
>     description: 'Basic tests for MediaStreamAudioSourceNode API'
>   }, (task, should) => {
>   
>     let successOrErrorCallback = funtion (callbackArg) {
>       let isPassed;
>       /* do something with callbackArg */
>       should(isPassed, 'Stream generation')
>           .message('succeded', 'failed');
>       task.done();
>     };
> 
>     should(() => { 
>       navigator.webkitGetUserMedia(
>           { audio: true },
>           successOrErrorCallback,
>           successOrErrorCallback);
>     });
>   }
> );
> 
> I don't think we have to test if the getUserMedia() call throws. It is not our
> feature to test.

It was in the original so I didn't want to change the logic in this area.

[hongchan, 2017-02-24 23:48:18]

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html
(right):

https://codereview.chromium.org/2690343005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioSource/mediastreamaudiosourcenode.html:34:
.notThrow();
On 2017/02/24 23:35:30, Raymond Toy wrote:
> On 2017/02/24 23:28:09, hongchan wrote:
> > If clang-format did this, it is really bad. I would rewrite this like:
> 
> I think it's really ugly too, but we discussed this the yesterday and you said
> it was ok.
> 
> However, we should either use clang-format and accept it as is, or not use
> clang-format at all.
> 
> > 
> > audit.define({
> >     label: 'test',
> >     description: 'Basic tests for MediaStreamAudioSourceNode API'
> >   }, (task, should) => {
> >   
> >     let successOrErrorCallback = funtion (callbackArg) {
> >       let isPassed;
> >       /* do something with callbackArg */
> >       should(isPassed, 'Stream generation')
> >           .message('succeded', 'failed');
> >       task.done();
> >     };
> > 
> >     should(() => { 
> >       navigator.webkitGetUserMedia(
> >           { audio: true },
> >           successOrErrorCallback,
> >           successOrErrorCallback);
> >     });
> >   }
> > );
> > 
> > I don't think we have to test if the getUserMedia() call throws. It is not
our
> > feature to test.
> 
> It was in the original so I didn't want to change the logic in this area.

Yes, but this getUserMedia() part really threw me away. Perhaps we should keep
this change here, but please leave a TODO that says we need to fix this later.

Also skipping should() on getUserMedia() does not change the original logic, I
think.

[hongchan, 2017-03-07 17:58:01]

lgtm

I will leave this to you. However, handling of getUserMedia callback really
could use some clean up.

[Raymond Toy, 2017-03-07 18:08:39]

On 2017/03/07 17:58:01, hongchan wrote:
> lgtm
> 
> I will leave this to you. However, handling of getUserMedia callback really
> could use some clean up.

Yeah, I didn't want to think too hard during the conversion about what this is
really testing. I just mimicked what the original seemed to be testing.

After the conversion of these tests, we'll have to go and revisit every test
anyway to see if it makes sense as a WPT.

[Raymond Toy, 2017-03-07 18:09:58]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:10:49]

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-03-07 19:53:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488910198122820,
"parent_rev": "2fc04cd1fea6d3c1fc5d8e1bd5fa2528b2a462d8", "commit_rev":
"8f98597ba6a3add1e39d0f824fc8e96ee4f663b7"}

[commit-bot: I haz the power, 2017-03-07 19:53:59]

Description was changed from

==========
Convert MediaStreamAudioSource test to testharness

Manually convert the test to use testharness and new Audit.

BUG=692724
TEST=mediastreamaudiosourcenode.html
==========

to

==========
Convert MediaStreamAudioSource test to testharness

Manually convert the test to use testharness and new Audit.

BUG=692724
TEST=mediastreamaudiosourcenode.html

Review-Url: https://codereview.chromium.org/2690343005
Cr-Commit-Position: refs/heads/master@{#455182}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f98597ba6a3add1e39d0f824fc8...
==========

[commit-bot: I haz the power, 2017-03-07 19:54:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8f98597ba6a3add1e39d0f824fc8...