Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563113002:
bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. (Closed)
Descriptionbluetooth: Add tests for Sidebar in bluetooth internals browser test suite.
Adds two tests for sidebar setup and open/close functionality.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6fd69ebdedf3d71f082fbbb8ac0ef9fbabea9052
Cr-Commit-Position: refs/heads/master@{#438428}
Patch Set 1 #Patch Set 2 : Merge upstream #
Total comments: 4
Patch Set 3 : Close sidebar in teardown, split sidebar open/close tests #
Total comments: 4
Patch Set 4 : Change to Array#some #
Depends on Patchset: Messages
Total messages: 26 (16 generated)
Description was changed from ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 ========== to ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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.
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Just two minor comments. https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:264: adapterFactory.reset(); I think you also want to reset the state of the sidebar. https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:443: test('SidebarOpenClose', function() { Would it make sense to split these? That way instead of having a big SidebarOpenClose test you have: Sidebar_DefaultState Sidebar_OpenClose Sidebar_OpenTwice Sidebar_CloseTwice
https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:264: adapterFactory.reset(); On 2016/12/12 23:24:48, ortuno wrote: > I think you also want to reset the state of the sidebar. Done. https://codereview.chromium.org/2563113002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:443: test('SidebarOpenClose', function() { On 2016/12/12 23:24:48, ortuno wrote: > Would it make sense to split these? That way instead of having a big > SidebarOpenClose test you have: > > Sidebar_DefaultState > Sidebar_OpenClose > Sidebar_OpenTwice > Sidebar_CloseTwice Done.
lgtm
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org
LGTM. https://codereview.chromium.org/2563113002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2563113002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: var sidebarObj = null; Nit: s/sidebarObj/sidebar Or is |sidebar| already taken? https://codereview.chromium.org/2563113002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2563113002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:436: var foundItem = sidebarItems.find(function(item) { Nit: You can use Array#some here, as follows expectTrue(sidebarItems.some(function(item) { return item.dataset.pageName === pageName; })); https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://codereview.chromium.org/2563113002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2563113002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: var sidebarObj = null; On 2016/12/14 00:32:05, dpapad wrote: > Nit: s/sidebarObj/sidebar > Or is |sidebar| already taken? |sidebar| is a module on the window object so I didn't want to confuse myself and others with the naming. https://codereview.chromium.org/2563113002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2563113002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:436: var foundItem = sidebarItems.find(function(item) { On 2016/12/14 00:32:05, dpapad wrote: > Nit: You can use Array#some here, as follows > > expectTrue(sidebarItems.some(function(item) { > return item.dataset.pageName === pageName; > })); > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... 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...
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 ortuno@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2563113002/#ps60001 (title: "Change to Array#some")
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": 1481688497603590,
"parent_rev": "54a420357f7485564c443829ff0c9ae2bd29928c", "commit_rev":
"b627807a5798cca3cd4af3d0e404d46e7c5f5f81"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2563113002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2563113002 ========== to ========== bluetooth: Add tests for Sidebar in bluetooth internals browser test suite. Adds two tests for sidebar setup and open/close functionality. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6fd69ebdedf3d71f082fbbb8ac0ef9fbabea9052 Cr-Commit-Position: refs/heads/master@{#438428} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6fd69ebdedf3d71f082fbbb8ac0ef9fbabea9052 Cr-Commit-Position: refs/heads/master@{#438428} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
