Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428773005:
bluetooth: Basic browser tests for chrome://bluetooth-internals. (Closed)
Descriptionbluetooth: Basic browser tests for chrome://bluetooth-internals.
UI tests of device management chrome://bluetooth-internals page
including device creation, device changes, and device removal.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/4a392852b8e9c6856f99fbcfcd8515246eca7cbb
Cr-Commit-Position: refs/heads/master@{#433365}
Patch Set 1 #Patch Set 2 : Fix type errors, merge upstream #Patch Set 3 : Add refreshDeviceList to deviceChanged #Patch Set 4 : Fix missing dependencies, enforce execution order of setup and test, add comments and helper functi… #Patch Set 5 : Clean up #
Total comments: 15
Patch Set 6 : Fix tests #
Total comments: 16
Patch Set 7 : Fix tests #Patch Set 8 : Fix formatting #
Total comments: 2
Patch Set 9 : Add rssi testing procedure, fix changeDevice #Patch Set 10 : Merge upstream #Patch Set 11 : Merge upstream #Patch Set 12 : Merge upstream #Patch Set 13 : Merge upstream #Patch Set 14 : Merge upstream #Patch Set 15 : Merge upstream #
Depends on Patchset: Messages
Total messages: 36 (19 generated)
Description was changed from ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. BUG=651282 ========== to ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org, ortuno@chromium.org, scheib@chromium.org
Description was changed from ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. UI tests of device management chrome://bluetooth-internals page including device creation, device changes, and device removal. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:49: */ How many copies of this function do we currently have? This is adding yet another one. Can we address this as part of this CL? Or in a separate CL that lands before this CL? https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:92: adapter: connection.bindStubDerivedImpl(this.adapter), Assuming this needs to be done every time getAdapter() is called, right? Otherwise let's just call bindStubDerivedImpl it once in the constructor and always return the same reference. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:104: var TestAdapter = function() { Can you merge TestAdapterProxy class into TestAdapter? Why does TestAdapter need to extend adapter.Adapter.stubClass? https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:106: }; \n https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:173: Nit: No need for blank line at the start of function. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:275: servicesColumn.textContent); Fits in previous line? https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:296: else expectFalse(removedRow.classList.contains('removed')); expectEquals(expectRemoved, !removedRow.classList.contains('removed')); Also why using expect() instead of assert(), throughout this file?
https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:49: */ On 2016/11/02 00:03:00, dpapad wrote: > How many copies of this function do we currently have? This is adding yet > another one. Can we address this as part of this CL? Or in a separate CL that > lands before this CL? Done. https://codereview.chromium.org/2428773005/ https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:92: adapter: connection.bindStubDerivedImpl(this.adapter), On 2016/11/02 00:03:00, dpapad wrote: > Assuming this needs to be done every time getAdapter() is called, right? > Otherwise let's just call bindStubDerivedImpl it once in the constructor and > always return the same reference. Done. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:104: var TestAdapter = function() { On 2016/11/02 00:03:00, dpapad wrote: > Can you merge TestAdapterProxy class into TestAdapter? Why does TestAdapter need > to extend adapter.Adapter.stubClass? connection.js documentation [1] says the stub passed into bindStubDerivedImpl must be a subclass of an auto-generated stub class. [1] https://cs.chromium.org/chromium/src/mojo/public/js/connection.js?rcl=0&l=172 https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:106: }; On 2016/11/02 00:03:00, dpapad wrote: > \n Done. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:173: On 2016/11/02 00:03:00, dpapad wrote: > Nit: No need for blank line at the start of function. Done. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:275: servicesColumn.textContent); On 2016/11/02 00:03:00, dpapad wrote: > Fits in previous line? Done. https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:296: else expectFalse(removedRow.classList.contains('removed')); On 2016/11/02 00:03:00, dpapad wrote: > expectEquals(expectRemoved, !removedRow.classList.contains('removed')); > > Also why using expect() instead of assert(), throughout this file? The WebUI browser_tests best practices [1] says to prefer expect* over assert* because it allows the rest of the test to run. So, I've just been making them all expect*. There are probably some places where assert* can be used instead though. [1] https://www.chromium.org/Home/domui-testing/webui-browser_tests#TOC-Best-prac...
https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:296: else expectFalse(removedRow.classList.contains('removed')); On 2016/11/02 at 22:47:24, mbrunson wrote: > On 2016/11/02 00:03:00, dpapad wrote: > > expectEquals(expectRemoved, !removedRow.classList.contains('removed')); > > > > Also why using expect() instead of assert(), throughout this file? > > The WebUI browser_tests best practices [1] says to prefer expect* over assert* because it allows the rest of the test to run. So, I've just been making them all expect*. There are probably some places where assert* can be used instead though. > > [1] https://www.chromium.org/Home/domui-testing/webui-browser_tests#TOC-Best-prac... Ok. The reality is that expect*() functions make WebUI tests tightly coupled with the fairly old test_api.js framework (see [1]), because expect functions have side-effects, where as assert functions are stateless, and I would like to remove them in the future. It would be nice if at some point we don't need to run nested test runner frameworks (mocha inside test_api.js), but I guess that is a battle to be fought in the future. [1] https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=169... https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:51: 'content/public/renderer/frame_interfaces', Nit: There is 2 more spaces of indentation than needed. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:58: console.log('Modules imported'); Can you remove this. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:129: return Promise.resolve({ info: this.adapterInfo_ }); No need for spaces {info: this.adapterInfo_} https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:130: }, \n between functions https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:138: adapter.AdapterClient); Indent is off. Also try to keep all arguments on the same line if possible (per styleguide). this.client = connection.bindHandleToProxy( client, adapter.AdapterClient); https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:153: handle, adapter.AdapterFactory); Indent off by 2. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:159: Nit: Remove \n https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:272: * @param {!string} address "!" is implied for number,boolean,string, so not necessary (here and elsewhere).
https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:129: return Promise.resolve({ info: this.adapterInfo_ }); On 2016/11/02 23:19:15, dpapad wrote: > No need for spaces {info: this.adapterInfo_} Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:130: }, On 2016/11/02 23:19:15, dpapad wrote: > \n between functions Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:138: adapter.AdapterClient); On 2016/11/02 23:19:15, dpapad wrote: > Indent is off. Also try to keep all arguments on the same line if possible (per > styleguide). > > this.client = connection.bindHandleToProxy( > client, adapter.AdapterClient); Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:153: handle, adapter.AdapterFactory); On 2016/11/02 23:19:15, dpapad wrote: > Indent off by 2. Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:159: On 2016/11/02 23:19:15, dpapad wrote: > Nit: Remove \n Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:272: * @param {!string} address On 2016/11/02 23:19:15, dpapad wrote: > "!" is implied for number,boolean,string, so not necessary (here and elsewhere). Done.
https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:51: 'content/public/renderer/frame_interfaces', On 2016/11/02 23:19:14, dpapad wrote: > Nit: There is 2 more spaces of indentation than needed. Done. https://codereview.chromium.org/2428773005/diff/100001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:58: console.log('Modules imported'); On 2016/11/02 23:19:15, dpapad wrote: > Can you remove this. Done.
The CQ bit was checked by mbrunson@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...
lgtm
https://codereview.chromium.org/2428773005/diff/130001/chrome/test/data/webui... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/130001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:330: Can you add a test for when an update doesn't contain the rssi value?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2428773005/diff/130001/chrome/test/data/webui... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2428773005/diff/130001/chrome/test/data/webui... chrome/test/data/webui/bluetooth_internals_browsertest.js:330: On 2016/11/03 22:20:48, ortuno wrote: > Can you add a test for when an update doesn't contain the rssi value? Done.
lgtm
The CQ bit was checked by mbrunson@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mbrunson@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mbrunson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2428773005/#ps270001 (title: "Merge upstream")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. UI tests of device management chrome://bluetooth-internals page including device creation, device changes, and device removal. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. UI tests of device management chrome://bluetooth-internals page including device creation, device changes, and device removal. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. UI tests of device management chrome://bluetooth-internals page including device creation, device changes, and device removal. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Basic browser tests for chrome://bluetooth-internals. UI tests of device management chrome://bluetooth-internals page including device creation, device changes, and device removal. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4a392852b8e9c6856f99fbcfcd8515246eca7cbb Cr-Commit-Position: refs/heads/master@{#433365} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/4a392852b8e9c6856f99fbcfcd8515246eca7cbb Cr-Commit-Position: refs/heads/master@{#433365}
Message was sent while issue was closed.
FYI: this CL seems to introduce a new flaky test as show by https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... One occurrence on Waterfall is https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/3830
Message was sent while issue was closed.
On 2016/11/21 19:43:11, stgao (slow on Monday) wrote: > FYI: this CL seems to introduce a new flaky test as show by > https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... > > One occurrence on Waterfall is > https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/3830 Looking into it.
Message was sent while issue was closed.
On 2016/11/21 at 21:23:44, mbrunson wrote: > On 2016/11/21 19:43:11, stgao (slow on Monday) wrote: > > FYI: this CL seems to introduce a new flaky test as show by > > https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... > > > > One occurrence on Waterfall is > > https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/3830 > > Looking into it. FWIW, the chrome://plugins tests (which is another WebUI page using Mojo), have been disabled for flakiness, see https://bugs.chromium.org/p/chromium/issues/detail?id=647305. Perhaps it's the same cause makes both tests flaky?
Message was sent while issue was closed.
On 2016/11/21 21:38:15, dpapad wrote: > On 2016/11/21 at 21:23:44, mbrunson wrote: > > On 2016/11/21 19:43:11, stgao (slow on Monday) wrote: > > > FYI: this CL seems to introduce a new flaky test as show by > > > > https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... > > > > > > One occurrence on Waterfall is > > > > https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/3830 > > > > Looking into it. > > FWIW, the chrome://plugins tests (which is another WebUI page using Mojo), have > been disabled for flakiness, see > https://bugs.chromium.org/p/chromium/issues/detail?id=647305. Perhaps it's the > same cause makes both tests flaky? Hmm. That could be the case since they're using the same structure. Although, this test is having timeout issues and not assertion errors. That would point to something not loading properly or quickly enough. More research required... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
