Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816663002:
Ensure tests don't depend on fake devices being added synchronously (Closed)
|
Created:
3 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
3 years, 8 months ago Reviewers:
ortuno CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure tests don't depend on fake devices being added synchronously
As specified navigator.usb.test.addFakeDevice and removeFakeDevice do
not synchronously add and remove the fake device however the Mojo-based
polyfill does. This patch adds a delay and fixes the WebUSB tests so
that they do not depend on this behaviors.
BUG=705734
Review-Url: https://codereview.chromium.org/2816663002
Cr-Commit-Position: refs/heads/master@{#465329}
Committed: https://chromium.googlesource.com/chromium/src/+/da15150a621a853c809cfc0375ef78822bfd54b7
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address ortuno@ comments #
Total comments: 9
Patch Set 3 : Remove GUIDs from the test interface #
Total comments: 4
Patch Set 4 : Define FakeUSBDevice class only once #
Messages
Total messages: 28 (13 generated)
reillyg@chromium.org changed reviewers: + ortuno@chromium.org
PTAL, I think this resolves your comment on the WPT export review.
https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:29: function getConnectedDevice(deviceInit) { Comment what the function does. Optional: You might want to change the name of the function it's not really obvious what it does from reading a test. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/resources/webusb-test.js (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:535: let guid = g_nextGuid++ + ""; nit: let guid = (g_nextGuid++).toString(); https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:11: let promise = connectionEventPromise('connect'); How high do you think this event is in the priority list of someone implementing the API? We are blocking many tests on this feature and it would be unfortunate if other browsers can't re-use these tests until this specific maybe-not-that-important feature is implemented. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:14: return promise.then(device => { Why are you nesting the promises? Why not: return promise .then(() => navigator.usb.getDevices()) .then(devices => { assert_equals(devices.length, 1); // ... }); Sometimes it is necessary to nest but IMO code is much easier to read if promises are not nested. https://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:24: let promise = connectionEventPromise('connect'); Why not getConnectedDevice()? https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:27: return promise.then(device => { nit: You don't need `device`. You can replace it with `()` or `_`. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/usbDevice.html (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:54: }); Seems like a the test that should be split into two. There is no guarantee that both cases will run and seems like it could easily become a flaky test. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:100: let addPromise = connectionEventPromise('connect'); Why not getConnectedDevice()? https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:105: let removePromise = connectionEventPromise('disconnect'); Seems like something you could abstract as well.
Address ortuno@ comments
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:29: function getConnectedDevice(deviceInit) { On 2017/04/12 03:31:07, ortuno wrote: > Comment what the function does. > > Optional: You might want to change the name of the function it's not really > obvious what it does from reading a test. Done. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/resources/webusb-test.js (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:535: let guid = g_nextGuid++ + ""; On 2017/04/12 03:31:07, ortuno wrote: > nit: let guid = (g_nextGuid++).toString(); Done. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:11: let promise = connectionEventPromise('connect'); On 2017/04/12 03:31:07, ortuno wrote: > How high do you think this event is in the priority list of someone implementing > the API? We are blocking many tests on this feature and it would be unfortunate > if other browsers can't re-use these tests until this specific > maybe-not-that-important feature is implemented. As discussed offline I gave this a try but it requires guaranteeing that we can flush an event through the system and I think that's a more complex thing to implement correctly than the connection events. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:14: return promise.then(device => { On 2017/04/12 03:31:07, ortuno wrote: > Why are you nesting the promises? Why not: > > return promise > .then(() => navigator.usb.getDevices()) > .then(devices => { > assert_equals(devices.length, 1); > // ... > }); > > Sometimes it is necessary to nest but IMO code is much easier to read if > promises are not nested. > > https://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html I need |device| to available in the scope of the handler passed to the next then(). https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:24: let promise = connectionEventPromise('connect'); On 2017/04/12 03:31:07, ortuno wrote: > Why not getConnectedDevice()? Done. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usb.html:27: return promise.then(device => { On 2017/04/12 03:31:07, ortuno wrote: > nit: You don't need `device`. You can replace it with `()` or `_`. Done. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/usb/usbDevice.html (right): https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:54: }); On 2017/04/12 03:31:07, ortuno wrote: > Seems like a the test that should be split into two. There is no guarantee that > both cases will run and seems like it could easily become a flaky test. The test API doesn't guarantee a way to inject a disconnect between when the Open message is sent and the response is received. I've turned this into a test to explicitly test the first case. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:100: let addPromise = connectionEventPromise('connect'); On 2017/04/12 03:31:08, ortuno wrote: > Why not getConnectedDevice()? I've added getFakeDeviceAndGuid() which handles the problem of getting both the USBDevice and also the guid so that we can disconnect the device later. https://codereview.chromium.org/2816663002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/usb/usbDevice.html:105: let removePromise = connectionEventPromise('disconnect'); On 2017/04/12 03:31:08, ortuno wrote: > Seems like something you could abstract as well. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping.
lgtm https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:30: // USBDevice and its guid (for calling navigator.usb.test.removeFakeDevice(). optional: // Creates a fake device and returns a promise that resolves once the // 'connect' event is fired for the fake device. The promise is resolved with // an object containing a USBDevice that corresponds to the fake USB Device // and the device's guid. https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:35: return { device: device, guid: guid }; optional: Would it make sense/be worth it to return a FakeUSBDevice rather than the guid. Then tests could call disconnect() directly on that object instead of passing a guid around. https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/usb.html:56: return navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { out of topic: Out of curiosity, how do you test the chooser? For web bluetooth we have a test API that we use to check what happens in the chooser e.g. a new device is added, a device is removed, etc.
Remove GUIDs from the test interface
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look as the code I added to add a FakeUSBDevice class is non-trivial. https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:30: // USBDevice and its guid (for calling navigator.usb.test.removeFakeDevice(). On 2017/04/18 00:44:22, ortuno wrote: > optional: > > // Creates a fake device and returns a promise that resolves once the > // 'connect' event is fired for the fake device. The promise is resolved with > // an object containing a USBDevice that corresponds to the fake USB Device > // and the device's guid. Done. https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/usb-helpers.js:35: return { device: device, guid: guid }; On 2017/04/18 00:44:22, ortuno wrote: > optional: Would it make sense/be worth it to return a FakeUSBDevice rather than > the guid. Then tests could call disconnect() directly on that object instead of > passing a guid around. Done. https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/usb.html:56: return navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { On 2017/04/18 00:44:22, ortuno wrote: > out of topic: Out of curiosity, how do you test the chooser? For web bluetooth > we have a test API that we use to check what happens in the chooser e.g. a new > device is added, a device is removed, etc. https://cs.chromium.org/chromium/src/chrome/browser/usb/usb_chooser_controlle...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/usb.html:56: return navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { On 2017/04/18 at 01:27:52, Reilly Grant wrote: > On 2017/04/18 00:44:22, ortuno wrote: > > out of topic: Out of curiosity, how do you test the chooser? For web bluetooth > > we have a test API that we use to check what happens in the chooser e.g. a new > > device is added, a device is removed, etc. > > https://cs.chromium.org/chromium/src/chrome/browser/usb/usb_chooser_controlle... hmm so for Web Bluetooth we have tests to make sure the right devices appear on the chooser depending on the filters. Do WebUSB's layout tests have something similar? https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/resources/webusb-test.js (right): https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:379: throw new Error('Cannot remove unknown device "' + guid + '"'); |guid| doesn't exist anymore. https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:541: class FakeUSBDevice { nit: The fact that we are redefining the class every time addFakeDevice is called seems iffy. Could we declare it outside the function?
Define FakeUSBDevice class only once
https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/usb.html:56: return navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { On 2017/04/18 03:04:06, ortuno wrote: > On 2017/04/18 at 01:27:52, Reilly Grant wrote: > > On 2017/04/18 00:44:22, ortuno wrote: > > > out of topic: Out of curiosity, how do you test the chooser? For web > bluetooth > > > we have a test API that we use to check what happens in the chooser e.g. a > new > > > device is added, a device is removed, etc. > > > > > https://cs.chromium.org/chromium/src/chrome/browser/usb/usb_chooser_controlle... > > hmm so for Web Bluetooth we have tests to make sure the right devices appear on > the chooser depending on the filters. Do WebUSB's layout tests have something > similar? What devices appear in the chooser isn't visible to layout tests. The filtering logic is tested in, https://cs.chromium.org/chromium/src/device/usb/usb_device_filter_unittest.cc https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/resources/webusb-test.js (right): https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:379: throw new Error('Cannot remove unknown device "' + guid + '"'); On 2017/04/18 03:04:06, ortuno wrote: > |guid| doesn't exist anymore. Done. https://codereview.chromium.org/2816663002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/resources/webusb-test.js:541: class FakeUSBDevice { On 2017/04/18 03:04:06, ortuno wrote: > nit: The fact that we are redefining the class every time addFakeDevice is > called seems iffy. Could we declare it outside the function? Done.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2816663002/#ps60001 (title: "Define FakeUSBDevice class only once")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492539714263290,
"parent_rev": "493c47dd802534fa89c50c48fc9c6f72622b067d", "commit_rev":
"da15150a621a853c809cfc0375ef78822bfd54b7"}
Message was sent while issue was closed.
Description was changed from ========== Ensure tests don't depend on fake devices being added synchronously As specified navigator.usb.test.addFakeDevice and removeFakeDevice do not synchronously add and remove the fake device however the Mojo-based polyfill does. This patch adds a delay and fixes the WebUSB tests so that they do not depend on this behaviors. BUG=705734 ========== to ========== Ensure tests don't depend on fake devices being added synchronously As specified navigator.usb.test.addFakeDevice and removeFakeDevice do not synchronously add and remove the fake device however the Mojo-based polyfill does. This patch adds a delay and fixes the WebUSB tests so that they do not depend on this behaviors. BUG=705734 Review-Url: https://codereview.chromium.org/2816663002 Cr-Commit-Position: refs/heads/master@{#465329} Committed: https://chromium.googlesource.com/chromium/src/+/da15150a621a853c809cfc0375ef... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/da15150a621a853c809cfc0375ef...
Message was sent while issue was closed.
https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/usb/usb.html (right): https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/usb/usb.html:56: return navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { On 2017/04/18 at 18:21:38, Reilly Grant wrote: > On 2017/04/18 03:04:06, ortuno wrote: > > On 2017/04/18 at 01:27:52, Reilly Grant wrote: > > > On 2017/04/18 00:44:22, ortuno wrote: > > > > out of topic: Out of curiosity, how do you test the chooser? For web > > bluetooth > > > > we have a test API that we use to check what happens in the chooser e.g. a > > new > > > > device is added, a device is removed, etc. > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/usb/usb_chooser_controlle... > > > > hmm so for Web Bluetooth we have tests to make sure the right devices appear on > > the chooser depending on the filters. Do WebUSB's layout tests have something > > similar? > > What devices appear in the chooser isn't visible to layout tests. The filtering logic is tested in, > > https://cs.chromium.org/chromium/src/device/usb/usb_device_filter_unittest.cc No need to change anything, but why isn't this tested through layout tests? The algorithm to filter devices is spec'ed and without cross browser tests for the algorithm there is no way to know all implementations are correct.
Message was sent while issue was closed.
On 2017/04/18 21:27:16, ortuno wrote: > https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/usb/usb.html (right): > > https://codereview.chromium.org/2816663002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/usb/usb.html:56: return > navigator.usb.requestDevice({ filters: [] }).then(chosenDevice => { > On 2017/04/18 at 18:21:38, Reilly Grant wrote: > > On 2017/04/18 03:04:06, ortuno wrote: > > > On 2017/04/18 at 01:27:52, Reilly Grant wrote: > > > > On 2017/04/18 00:44:22, ortuno wrote: > > > > > out of topic: Out of curiosity, how do you test the chooser? For web > > > bluetooth > > > > > we have a test API that we use to check what happens in the chooser e.g. > a > > > new > > > > > device is added, a device is removed, etc. > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/usb/usb_chooser_controlle... > > > > > > hmm so for Web Bluetooth we have tests to make sure the right devices appear > on > > > the chooser depending on the filters. Do WebUSB's layout tests have > something > > > similar? > > > > What devices appear in the chooser isn't visible to layout tests. The > filtering logic is tested in, > > > > https://cs.chromium.org/chromium/src/device/usb/usb_device_filter_unittest.cc > > No need to change anything, but why isn't this tested through layout tests? The > algorithm to filter devices is spec'ed and without cross browser tests for the > algorithm there is no way to know all implementations are correct. With the current layering it is impossible to test with a layout test. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
