Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694173006:
Convert MediaStreamAudioDestination 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 MediaStreamAudioDestination tests to testharness
Manually converted the file to use testharness and updated to use new
Audit instead of old.
BUG=692724
TEST=mediastreamaudiodestinationnode.html
Review-Url: https://codereview.chromium.org/2694173006
Cr-Commit-Position: refs/heads/master@{#455212}
Committed: https://chromium.googlesource.com/chromium/src/+/d342a948f3809c5f65941cf454c068771718cf0a
Patch Set 1 #Patch Set 2 : Remove task.describe #
Total comments: 11
Patch Set 3 : Address review comments #
Messages
Total messages: 14 (4 generated)
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, Is this what clang-format does? I actually prefer: should(() => { /* ... */ }, 'something is done') .throw(); https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:52: // addStream(mediaStreamDestination.stream). Do we still need this commented out section?
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, On 2017/02/24 23:18:04, hongchan wrote: > Is this what clang-format does? > > I actually prefer: > > should(() => { /* ... */ }, > 'something is done') > .throw(); Yes. Maybe adding { } will change the layout? https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:52: // addStream(mediaStreamDestination.stream). On 2017/02/24 23:18:04, hongchan wrote: > Do we still need this commented out section? This addStream part? We should probably test that.
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, On 2017/02/24 23:24:30, Raymond Toy wrote: > On 2017/02/24 23:18:04, hongchan wrote: > > Is this what clang-format does? > > > > I actually prefer: > > > > should(() => { /* ... */ }, > > 'something is done') > > .throw(); > > Yes. Maybe adding { } will change the layout? Nope. We get should( () => {mediaStreamDestination.channelCount = 9}, 'Setting the channel count beyond 8') .throw('NotSupportedError');
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, On 2017/02/24 23:28:42, Raymond Toy wrote: > On 2017/02/24 23:24:30, Raymond Toy wrote: > > On 2017/02/24 23:18:04, hongchan wrote: > > > Is this what clang-format does? > > > > > > I actually prefer: > > > > > > should(() => { /* ... */ }, > > > 'something is done') > > > .throw(); > > > > Yes. Maybe adding { } will change the layout? > > Nope. We get > > should( > () => {mediaStreamDestination.channelCount = 9}, > 'Setting the channel count beyond 8') > .throw('NotSupportedError'); Then at least we should add a space after/before curly braces. https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:52: // addStream(mediaStreamDestination.stream). On 2017/02/24 23:24:30, Raymond Toy wrote: > On 2017/02/24 23:18:04, hongchan wrote: > > Do we still need this commented out section? > > This addStream part? We should probably test that. I meant between the line 43 ~ 52.
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, On 2017/02/24 23:34:26, hongchan wrote: > On 2017/02/24 23:28:42, Raymond Toy wrote: > > On 2017/02/24 23:24:30, Raymond Toy wrote: > > > On 2017/02/24 23:18:04, hongchan wrote: > > > > Is this what clang-format does? > > > > > > > > I actually prefer: > > > > > > > > should(() => { /* ... */ }, > > > > 'something is done') > > > > .throw(); > > > > > > Yes. Maybe adding { } will change the layout? > > > > Nope. We get > > > > should( > > () => {mediaStreamDestination.channelCount = 9}, > > 'Setting the channel count beyond 8') > > .throw('NotSupportedError'); > > Then at least we should add a space after/before curly braces. I originally started with () => { media = 9 }, clang-format folded everything to one line, with no spaces. https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:52: // addStream(mediaStreamDestination.stream). On 2017/02/24 23:34:26, hongchan wrote: > On 2017/02/24 23:24:30, Raymond Toy wrote: > > On 2017/02/24 23:18:04, hongchan wrote: > > > Do we still need this commented out section? > > > > This addStream part? We should probably test that. > > I meant between the line 43 ~ 52. I think we should just make it test that we have one output. If this ever gets changed, we'll see the test fail.
https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html (right): https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:33: () => mediaStreamDestination.channelCount = 9, On 2017/02/24 23:48:02, Raymond Toy wrote: > On 2017/02/24 23:34:26, hongchan wrote: > > On 2017/02/24 23:28:42, Raymond Toy wrote: > > > On 2017/02/24 23:24:30, Raymond Toy wrote: > > > > On 2017/02/24 23:18:04, hongchan wrote: > > > > > Is this what clang-format does? > > > > > > > > > > I actually prefer: > > > > > > > > > > should(() => { /* ... */ }, > > > > > 'something is done') > > > > > .throw(); > > > > > > > > Yes. Maybe adding { } will change the layout? > > > > > > Nope. We get > > > > > > should( > > > () => {mediaStreamDestination.channelCount = 9}, > > > 'Setting the channel count beyond 8') > > > .throw('NotSupportedError'); > > > > Then at least we should add a space after/before curly braces. > > I originally started with > > () => { > media = 9 > }, > > clang-format folded everything to one line, with no spaces. > Acknowledged. https://codereview.chromium.org/2694173006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/MediaStreamAudioDestination/mediastreamaudiodestinationnode.html:52: // addStream(mediaStreamDestination.stream). On 2017/02/24 23:48:02, Raymond Toy wrote: > On 2017/02/24 23:34:26, hongchan wrote: > > On 2017/02/24 23:24:30, Raymond Toy wrote: > > > On 2017/02/24 23:18:04, hongchan wrote: > > > > Do we still need this commented out section? > > > > > > This addStream part? We should probably test that. > > > > I meant between the line 43 ~ 52. > > I think we should just make it test that we have one output. If this ever gets > changed, we'll see the test fail. Acknowledged.
lgtm
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": 1488909953342880,
"parent_rev": "f2ef57e81267138c5f8fabc2ea7044b59600238a", "commit_rev":
"d342a948f3809c5f65941cf454c068771718cf0a"}
Message was sent while issue was closed.
Description was changed from ========== Convert MediaStreamAudioDestination tests to testharness Manually converted the file to use testharness and updated to use new Audit instead of old. BUG=692724 TEST=mediastreamaudiodestinationnode.html ========== to ========== Convert MediaStreamAudioDestination tests to testharness Manually converted the file to use testharness and updated to use new Audit instead of old. BUG=692724 TEST=mediastreamaudiodestinationnode.html Review-Url: https://codereview.chromium.org/2694173006 Cr-Commit-Position: refs/heads/master@{#455212} Committed: https://chromium.googlesource.com/chromium/src/+/d342a948f3809c5f65941cf454c0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d342a948f3809c5f65941cf454c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
