Closed Bug 1408456 Opened 7 years ago Closed 7 years ago

Move some Web Audio API tests from mochitest to web-platform-tests

Categories

(Core :: Web Audio, enhancement, P3)

Product:
Core
Component:
Web Audio
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: padenot, Assigned: padenot)

Details

Attachments

(18 files)

Bug 1408456 - Convert test_AnalyserNode.html to a web-platform-test.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - Convert test_AnalyserNode.html to a web-platform-test.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - Convert test_analyserNodeWithGain.html to a web-platform-test: test-analyser-gain.html.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - Convert test_analyserScale.html to a web-platform-test: test-analyser-scale.html.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - Convert test_analyserNodeMinimum.html to a web-platform-test: test-analyser-minimum.html.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - Convert test_analyserNodeOutput.html to a web-platform-test: test-analyser-output.html.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1408456 - convert mochitest test_analyserNodePassThrough.html and test_analyserNodePassThrough.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_AudioBuffer.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeEnded.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeLazyLoopParam.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeLoop.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeLoopStartEnd.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeLoopStartEndSame.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioBufferSourceNodeOffset.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_AudioContext.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioDestinationNode.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_AudioListener.html to a web-platform-test
59 bytes, text/x-review-board-request
Details
Bug 1408456 - convert mochitest test_audioParamChaining.html to a web-platform-test
59 bytes, text/x-review-board-request
Details 
As part of moving the Web Audio API spec to candidate recommendation, we need to add some Web Audio API test form Gecko to web platform tests.

This bug is about getting a sense of what it would take to have more tests ran by all implementations. I'll choose and convert some tests.

In practice, Raymond Toy from Google has been running all the tests from the chromium repository on Firefox, with somewhat good results.
Assignee: nobody → padenot
This was done using `mach web-platform-test-create path/to/the/test.html`, which opens an editor. Then I copy and past the js code from the mochitest to the new test's <script>.

Then I ran some regexp to change from simpletest.js to the wpt's functions, and then ran prettier on the file. Finally, I tweaked some whitespace things I was not happy about, removed the old test from the mochitest manifest, remove the test itself from the tree, added the new test, and commited.

Clearly it can be improved to be automated.
Comment on attachment 8918373 [details]
Bug 1408456 - Convert test_AnalyserNode.html to a web-platform-test.

https://reviewboard.mozilla.org/r/189192/#review194702

Would you be happy to push a changeset using hg mv or hg cp?
I've pushed that separately as it was easier for me to review, and may be fiddly to generate if using git.

::: commit-message-f3e93:1
(Diff revision 1)
> +Bug 1408456 - Convert test_AnalyserNode.html to a web-platform-test. r?karlt

Web platform tests are not run on Android or ASAN builds.

What do you think about leaving exact copies of the testharness.js tests in
dom/media/web-audio/test and running those copies only on these platforms (and
any others if appropriate), until web platform tests are run on these
platforms?

(hg cp could be used instead of hg mv.)

::: testing/web-platform/tests/webaudio/the-audio-api/the-analysernode-interface/test-analysernode.html:168
(Diff revision 1)
> +      assert_true(
> +        Math.abs(analyser.smoothingTimeConstant - 0.8) < 0.001,
> +        "Correct default value for smoothingTimeConstant"
> +      );

assert_approx_equals() is intended to do this with more helpful error messages, but getting the tests in as is is more important than these details.

http://web-platform-tests.org/writing-tests/testharness-api.html#assert_approx_equalsactual-expected-epsilon-description
Attachment #8918373 - Flags: review?(karlt) → review+
Comment on attachment 8918728 [details]
Bug 1408456 - Convert test_AnalyserNode.html to a web-platform-test.

https://reviewboard.mozilla.org/r/189554/#review194704
Attachment #8918728 - Flags: review?(karlt) → review+
A bit more work on this, with a few changes:
- `hg cp ...` is clearly better, thanks. 
- I've started writing some code to automate it: https://github.com/padenot/mochitest-to-wpt (it's a mess, of course, for now, but it helps)
- I've left the mochitest in place, unmodified, but have changed the manifest to only run the tests on ASAN or android
Comment on attachment 8918908 [details]
Bug 1408456 - Convert test_analyserScale.html to a web-platform-test: test-analyser-scale.html.

https://reviewboard.mozilla.org/r/189792/#review195204

::: testing/web-platform/tests/webaudio/the-audio-api/the-analysernode-interface/test-analyser-scale.html:4
(Diff revision 1)
> -  <title>Test AnalyserNode when the input is scaled </title>
> -  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> -  <script type="text/javascript" src="webaudio.js"></script>
> -  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> -</head>
> +  <meta charset="utf-8">
> +  <script src="/resources/testharness.js"></script>
> +  <script src="/resources/testharnessreport.js"></script>
> +  <script>
> +    var t = async_test("Analyser Scale");

The old name was specific about the input being scaled.  Please keep that.

::: testing/web-platform/tests/webaudio/the-audio-api/the-analysernode-interface/test-analyser-scale.html:22
(Diff revision 1)
> -  function getAnalyserData() {
> +    function getAnalyserData() {
>      gain.gain.setValueAtTime(currentGain, context.currentTime);

Missing alignment of the function body.

::: testing/web-platform/tests/webaudio/the-audio-api/the-analysernode-interface/test-analyser-scale.html:33
(Diff revision 1)
> -      if (array[i] > max) {
> +    if (array[i] > max) {
> -        max = Math.abs(array[i] - 128);
> +      max = Math.abs(array[i] - 128);
> -      }
> +    }
>      }
>      if (max <= currentGain * 128) {
> -      ok(true, "Analyser got scaled data for " + currentGain);
> +      assert_true(true, "Analyser got scaled data for " + currentGain);

http://web-platform-tests.org/writing-tests/testharness-api.html#asynchronous-tests
says that asserts in async_tests must be wrapped in t.step().

In the tests I've seen, usually as much code as practical is wrapped in
t.step().  I'm not sure why, but perhaps that captures any unexpected
exceptions better.

Here, the asserts could be wrapped by replacing the two
requestAnimationFrame(getAnalyserData) with
requestAnimationFrame(t.step_func(getAnalyserData))
or replacing getAnalyserData with

var getAnalyserData = t.step_func(function() {
 [...]
});

But probably best here is to use a "Single Page Test" by leaving the title as
is, removing async_test(), and using done() instead of t.done().

http://web-platform-tests.org/writing-tests/testharness-api.html#single-page-tests
Attachment #8918908 - Flags: review?(karlt) → review+
Comment on attachment 8918908 [details]
Bug 1408456 - Convert test_analyserScale.html to a web-platform-test: test-analyser-scale.html.

https://reviewboard.mozilla.org/r/189792/#review195222

::: testing/web-platform/meta/MANIFEST.json:631557
(Diff revision 1)
>    "webaudio/js/buffer-loader.js": [
>     "4d564eae0b3d7d1045626d1f144cd2638dba64e5",
>     "support"
>    ],
>    "webaudio/js/helpers.js": [
> -   "c5d44cf8101c50b59c366ed1971205193f32e1bf",
> +   "dff18a7e57adb3847b70fa7f1f3752b591b38d6e",

I wonder whether this change is in the wrong changeset?
Comment on attachment 8918907 [details]
Bug 1408456 - Convert test_analyserNodeWithGain.html to a web-platform-test: test-analyser-gain.html.

https://reviewboard.mozilla.org/r/189790/#review195246
Attachment #8918907 - Flags: review?(karlt) → review+
Comment on attachment 8918910 [details]
Bug 1408456 - Convert test_analyserNodeOutput.html to a web-platform-test: test-analyser-output.html.

https://reviewboard.mozilla.org/r/189796/#review195250

Same issue with unwrapped assert.  Would a single-page test approach work do you think?
Attachment #8918910 - Flags: review?(karlt)
Comment on attachment 8918909 [details]
Bug 1408456 - Convert test_analyserNodeMinimum.html to a web-platform-test: test-analyser-minimum.html.

https://reviewboard.mozilla.org/r/189794/#review195254

::: testing/web-platform/tests/webaudio/the-audio-api/the-analysernode-interface/test-analyser-minimum.html:8
(Diff revision 1)
>  <head>
> -  <title>Test AnalyserNode when the input is silent</title>
> -  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> -  <script type="text/javascript" src="webaudio.js"></script>
> -  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> -</head>
> +  <meta charset="utf-8">
> +  <script src="/resources/testharness.js"></script>
> +  <script src="/resources/testharnessreport.js"></script>
> +  <script>
> +    var t = async_test("Analyser minimum");

Either make this a single page test, or use t.step_function().
Attachment #8918909 - Flags: review?(karlt) → review+
Those new patches address all your comments, restoring the original title, using single page tests, and moving the utils.js hash change to the right commit.
Comment on attachment 8918910 [details]
Bug 1408456 - Convert test_analyserNodeOutput.html to a web-platform-test: test-analyser-output.html.

https://reviewboard.mozilla.org/r/189796/#review195724

::: testing/web-platform/tests/webaudio/js/helpers.js:146
(Diff revision 2)
> +        assert_equals(expectedFrames,
> +                     gTest.length, "Correct number of expected frames");

alignment
Attachment #8918910 - Flags: review?(karlt) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d3baf64827
Convert test_AnalyserNode.html to a web-platform-test. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a374884e626c
Convert test_analyserNodeWithGain.html to a web-platform-test: test-analyser-gain.html.  r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/794463d2292e
Convert test_analyserScale.html to a web-platform-test: test-analyser-scale.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9a7b008e91
Convert test_analyserNodeMinimum.html to a web-platform-test: test-analyser-minimum.html.  r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ddeed089a4
Convert test_analyserNodeOutput.html to a web-platform-test: test-analyser-output.html.  r=karlt
Comment on attachment 8933280 [details]
Bug 1408456 - convert mochitest test_analyserNodePassThrough.html and test_analyserNodePassThrough.html to a web-platform-test

https://reviewboard.mozilla.org/r/204216/#review212342

Can you open a new bug for this? Also, I think test_AudioListener.html is testing stuff that have been removed from the spec, have a look at the document: https://webaudio.github.io/web-audio-api/.

In general, when a bug is closed, it's better to open a new one.
Attachment #8933280 - Flags: review?(padenot)
Attachment #8940155 - Flags: review?(padenot)
Attachment #8940157 - Flags: review?(padenot)
Attachment #8940159 - Flags: review?(padenot)
Attachment #8940164 - Flags: review?(padenot)
Attachment #8940172 - Flags: review?(padenot)
Attachment #8940173 - Flags: review?(padenot)
Attachment #8940175 - Flags: review?(padenot)
Attachment #8940225 - Flags: review?(padenot)
Attachment #8940227 - Flags: review?(padenot)
Attachment #8940229 - Flags: review?(padenot)
Attachment #8940240 - Flags: review?(padenot)
Attachment #8933280 - Flags: review?(padenot)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: