Details
- Reviewers
jib - Commits
- Restricted Diffusion Commit
rMOZILLACENTRAL341a3c4a36fd: Bug 934425 - Web platform http test for setSinkId. r=jib - Bugzilla Bug ID
- 934425
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
Event Timeline
Duh, I got through almost the entire review before I realized this is basically a copy of setSinkId.https.html :)
In any case, since that file is in such a sorry state, I recommend applying this review to both files.
Also, I dislike that both tests are ambivalent about whether setSinkId succeeds or fails. IMHO the https one must assert success, and the http one must assert failure.
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 3–12 | If it were me, I'd remove all this boiler plate. WebRTC tests have a lot less. | |
| 6 | Wrong author. | |
| 18 | Mozilla uses ==, Chrome uses ===. I also prefer just inlining functions this small. Also, there's destructuring if we want: list.find(({kind}) => kind == "audiooutput") | |
| 19–21 | Would it be better to use a new new Audio() in each test? Avoids state dependencies between tests. | |
| 23 | nonexistent | |
| 25–26 | Please rewrite with async/await. Also, indent by 2, and word-wrap IMHO. Basically, I vote we ignore consistency with the other tests in this folder, and go with the code style of more recent tests like in the link above. | |
| 32 | forEach runs all setSink calls in parallel on the same audio which is nondeterministic. We also generally want to do things in sequence in tests. With async/await we can do: for (const {deviceId} of devices.filter(({kind}) => kind == "audiooutput")) {
await audio.setSinkId(deviceId);
} | |
| 33–34 | As discussed, we also want to test audio.sinkId before the promise resolves. E.g.: const p = audio.setSinkId(deviceId); assert_equals(audio.sinkId, null, "before it resolves, setSinkId is unchanged"); assert_equals(await p, undefined, "setSinkId resolves with undefined"); assert_equals(audio.sinkId, deviceId, "when it resolves, setSinkId updates sinkId to the requested deviceId"); | |
| 35–36 | Why can only one device be set without permission? I don't understand. If so, this strikes me as an odd way to write this test. Seems simpler to assert length == 1 upfront, test the first entry, and forgo the for-loop then. We generally want tests that test the same number of things regardless of system configuration. | |
| 37–38 | Uha, weird to see nested promise_tests. I think I'd prefer to replace these with asserts (make them part of the same test). | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 33–34 | s/null/""/ | |
I haven't updated the https test just yet. Once we have the final version of this one I will do it. Keep in your mind that, in theory, should be no difference in behavior between the http and https test. This is because we do not provide authorization to any of the devices in both tests so it's only the default device that can be set. In FF case this would make the https test fail if we had more than one device in automation. But we don't :)
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 19–21 | The first two tests are the default state and a very simple error case. I do not expect any false state after them. I think we can use the same. | |
| 35–36 | This is because the first device is assumed to be the default one. The following devices, if any, setSinkId will fail. I removed the initial for loop and I created a similar idea in a cleaner way (imo). However I could remove the loop totally. Have a look as is, and I can update it in next round. | |
| 37–38 | Remove almost all pf them, apart from the loop at the end. I could remove the loop totally. Have a look at it first. | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 18 | Nit: no need for this to be global. Maybe move it to where it's initialized? | |
| 20 | const | |
| 21–23 | Actually, since we're already filtering, we can simplify: assert_not_equals(outputDevicesList.length, 0,
"media device list includes at least one audio output device"); | |
| 24 | Hmm, actually that's not guaranteed by the spec. We might have to go through all of them to make sure one succeeds. | |
| 33–34 | I don't see any support in the spec for this. I would expect NotFoundError. | |
| 36 | redundant I think. | |
| 37–39 | What does this test? | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 24 | The problem is that we do not know when a device is default or not in order to expect failure or not. | |
| 33–34 | Would be better to create an issue against the spec to support it, since Chrome has been working like that for sometime? | |
| 37–39 | That setting sinkId on the rest, non default device, will fail. | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 24 | Right, but if we go through all of them then we can check that only one of them succeeded, and all the others failed. | |
| 33–34 | w3c/mediacapture-output#80 has been merged, so this is good. | |
| 37–38 | I don't think we can assume the first in the list is the default device. Assuming we go through and check each one, as I suggested above, then we can detect the default one by which one succeeds. | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 24 | We can do that but we cannot use the promise_test/promise_reject methods from test harness because we don't know in advance which device will be the default one. | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 18 | Please use promise_test here. There's certain infrastructure involved with a web-platform-test that we must use, otherwise the test doesn't count. It probably doesn't show up at all now. I'm not sure why you think we cannot use it. We should be able to loop over the available devices and test them all with asserts AFAICT within a single promise test, so ping me if you have specific problems. | |
| 20 | const again | |
| 26 | remove comment (and assumption) | |
| 32 | acceptedDevice (singular) looks like an un-referenced variable. Also, this assumption seems wrong. Remove. I suspect this test isn't running now (or at least isn't showing any output). | |
| 41 | "NotAllowedError". | |
| 43 | We're missing: assert_equals(acceptedDevices.length, 1, "Only one device succeeded (the default one)."); | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 18 | That works thanks. I thought that the promise_test method will report the rejected promises as failure. | |
| 32 | Opps, nice catch ... with JS I am missing compiler more and more ... | |
| 43 | Hhmm I don't get this one, acceptedDevices is a decimal. | |
| testing/web-platform/tests/audio-output/setSinkId.html | ||
|---|---|---|
| 43 | Sorry, yeah I meant: assert_equals(acceptedDevices, 1, "Only one device succeeded (the default one)."); after the for-loop has finished. I now see you test for this inside the for-loop instead. That works too. Keep one or both. | |