Closed Bug 1481676 Opened 3 years ago Closed 3 years ago
Worklet tests to WPT
46 bytes, text/x-phabricator-request
|Details | Review|
Follow-up of bug 1472324. Some part of AudioWorklet can now be tested.
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
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?
(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.
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?
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.
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)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/4e4248206816 Add AudioWorklet tests to WPT. r=karlt
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.
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.