Closed
Bug 1481676
Opened 6 years ago
Closed 6 years ago
Add AudioWorklet tests to WPT
Categories
(Core :: Web Audio, enhancement, P3)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: arnaud.bienner, Assigned: arnaud.bienner)
References
Details
Attachments
(1 file, 3 obsolete files)
Follow-up of bug 1472324. Some part of AudioWorklet can now be tested.
Assignee | ||
Updated•6 years ago
|
Blocks: audioworklet
Depends on: 1472324
Assignee | ||
Comment 1•6 years ago
|
||
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
Updated•6 years ago
|
Rank: 25
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Summary: Add AudioWorklet tests in WPT → Add AudioWorklet tests to WPT
Assignee | ||
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
(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)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → arnaud.bienner
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: GhULWTiyHRx
Comment 5•6 years ago
|
||
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+
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
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 10•6 years ago
|
||
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+
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #9003761 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9003762 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9003366 -
Attachment is obsolete: true
Comment 13•6 years ago
|
||
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.
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4e4248206816
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•