Add AudioWorklet tests to WPT

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
Rank:
25
RESOLVED FIXED
Last year
5 months ago

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Follow-up of bug 1472324.
Some part of AudioWorklet can now be tested.
Blocks: 1062849
Depends on: 1472324
Actually, what I want to do is add tests to testing/web-platform/tests/worklets for AudioWorklet, to complete existing worklet tests, and activate some of them.
Summary: Activate some AudioWorklet tests in WPT → Add AudioWorklet tests in WPT
Rank: 25
Priority: -- → P3
Summary: Add AudioWorklet tests in WPT → Add AudioWorklet tests to WPT
I started writing a patch for this: putting the pref in unittest/user.js instead of all.js works, so that's good news.

Unfortunately, turning on the pref only for for non release and debug builds doesn't work.

While something like this works:
[test name]
  expected:
      if not release_or_beta: FAIL

This doesn't work:
  prefs:
    if not release_or_beta: [dom.audioworklet.enabled:true, dom.worklet.enabled:true]
returning an error from wptrunner:
"ValueError: dictionary update sequence element #0 has length 25; 2 is required"

Looks like wptrunner has a problem parsing conditionals dictionaries (or "prefs") values

Note that
prefs: [dom.audioworklet.enabled:true, dom.worklet.enabled:true]
works, but as noted in bug 1472324 comment 11, this will cause a crash on beta and release builds.

I think the nice way to fix it would be a open a bug (and pull request) on https://github.com/web-platform-tests/wpt/tree/master/tools/wptrunner to fix this, so we can use it here.

Otherwise we can wait for worklet to be ready for beta to push the patch, so we don't need to have release_or_beta conditionals checks.
I don't like this solution, because I believe it's better to test as much as we can now, to avoid regressions while we are working on adding worklet support.

Karl: thoughts?
Flags: needinfo?(karlt)
(In reply to Arnaud Bienner from comment #2)
> I think the nice way to fix it would be a open a bug (and pull request) on
> https://github.com/web-platform-tests/wpt/tree/master/tools/wptrunner to fix
> this, so we can use it here.

Yes, that's a good option.

> Otherwise we can wait for worklet to be ready for beta to push the patch, so
> we don't need to have release_or_beta conditionals checks.
> I don't like this solution, because I believe it's better to test as much as
> we can now, to avoid regressions while we are working on adding worklet
> support.

I agree that it would be better to test what we can now, so that we know if
we are regressing anything.

Tests can be conditionally disabled:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests#Disabling_tests

The disadvantage with that approach is we need to remember to enable them
again, but having a bug on file blocking 1062849 would be sufficient.

Alternatively, the test file can be annotated for an expected crash:

[filename.html]
    expected:
        if release_or_beta: CRASH

The disadvantage is that it may be somewhat difficult to guess the results
for release_or_beta.  I guess
https://hg.mozilla.org/mozilla-central/rev/f99b99cc076d06251bf16725647d52f65bf17d4d#l1.13
could be modified to crash even on m-c to determine the results, if you'd like
to do that.
Flags: needinfo?(karlt)
Depends on: 1482496
Assignee: nobody → arnaud.bienner
MozReview-Commit-ID: GhULWTiyHRx
Comment on attachment 9003366 [details]
Bug 1481676 - Add AudioWorklet tests to WPT. r=karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003366 - Flags: review+
Karl: sorry about creating lot of new requests.
The new tool is not behaving like moz review: not sure why.
moz-phab submit didn't updated the original review, but created new ones instead (even though I used hg commit --amend).
Not sure what I did wrong, but this is really counter-intuitive.
Maybe for now you can just approve the last one (https://phabricator.services.mozilla.com/D4205) and close the others?
Flags: needinfo?(karlt)
Comment on attachment 9003765 [details]
Bug 1481676 - Add AudioWorklet tests to WPT. r=karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003765 - Flags: review+
I would have guessed moz-phab is meant to add a "Differential Revision:" link
to the commit message and use that again to upload the next "diff" to the same
"revision", but this would not be the first counter-intuitive discovery with
the system.  We're all trying to work around its quirks while it is still
being developed.

The #phabricator IRC channel is a good place to share discoveries.

I've approved the last one, thank you, and resigned as reviewer on the others.
I don't know how to close them, but perhaps it doesn't matter if they are left
open.
Flags: needinfo?(karlt)
Pushed to try, and passed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4fa7b9fc6d1fa168cb0e6caf6cf31ee29317c0a
(the failing tests look intermittent and not related to my changes)
Keywords: checkin-needed
Attachment #9003761 - Attachment is obsolete: true
Attachment #9003762 - Attachment is obsolete: true
Attachment #9003366 - Attachment is obsolete: true
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e4248206816
Add AudioWorklet tests to WPT. r=karlt
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12730 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/4e4248206816
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
See Also: → 1528876
You need to log in before you can comment on or make changes to this bug.