Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1466433002:
Add Settings bluetooth page test (Closed)
|
Created:
5 years, 1 month ago by stevenjb Modified:
4 years, 11 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Settings bluetooth page test
This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder).
BUG=543299
For simple change to BUILD.gn:
TBR=sky@chromium.org
Committed: https://crrev.com/dc099e3095d5152131e84991125db6311cfb1568
Cr-Commit-Position: refs/heads/master@{#367018}
Patch Set 1 #Patch Set 2 : . #
Total comments: 18
Patch Set 3 : More Feedback #
Total comments: 7
Patch Set 4 : Add callback arg #
Total comments: 6
Patch Set 5 : More feedback #
Total comments: 6
Patch Set 6 : Fix calls to setTimeout #Patch Set 7 : Disable test on Debug builds #Patch Set 8 : Rebase #Patch Set 9 : Rebase + Use FakeBluetooth + add device list tests #
Total comments: 36
Patch Set 10 : Feedback #Patch Set 11 : Move 'self = this' comment to mocha_adapter.js #Patch Set 12 : Add @types #Patch Set 13 : Rebase #
Total comments: 6
Patch Set 14 : . #Patch Set 15 : Rebase #Patch Set 16 : Make Chrome OS only #
Total comments: 9
Patch Set 17 : Chrome OS only Take 2 #Patch Set 18 : Chrome OS only Take 2 #Messages
Total messages: 77 (31 generated)
Description was changed from ========== Add Settings bluetooth page test BUG=543299 ========== to ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org, dschuyler@chromium.org, michaelpg@chromium.org
Note: not everyone needs to review this, but as the first subpage example using SettingsPageBrowserTest I figured I would solicit broad input.
https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:26: self.enabled_ = false; Nit: How about turning enabled into a simple local var as follows? var enabled = false; Then in ...setAdapterState = function() { enabled = state.powered; ... } and similarly in the assertions later.
https://codereview.chromium.org/1466433002/diff/20001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:994: 'test/data/webui/settings/bluetooth_page_browsertest.js', alphabetize https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:7: // Polymer BrowserTest fixture. comment https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:12: * @extends {PolymerTest} extends SPBT https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:28: chrome.bluetooth.getAdapterState = function() { shouldn't this take a callback? if this is passing, that means setAdapterState is being called before getAdapterState's callback is called.... https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:49: bluetoothSection.querySelector('settings-bluetooth-page'); $$ because getSection returns a SettingsSection. also change getSection to @return {!SettingsSection} bonus points for changing getPage to @return {!PolymerElement} https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:51: var enable = bluetooth.$$('#enableBluetooth'); bluetooth.$.enableBluetooth https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:57: var sections = page.shadowRoot.querySelectorAll('settings-section'); $$
https://codereview.chromium.org/1466433002/diff/20001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:994: 'test/data/webui/settings/bluetooth_page_browsertest.js', On 2015/11/20 15:08:17, michaelpg wrote: > alphabetize Done. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:7: // Polymer BrowserTest fixture. On 2015/11/20 15:08:17, michaelpg wrote: > comment removed https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:12: * @extends {PolymerTest} On 2015/11/20 15:08:17, michaelpg wrote: > extends SPBT Done. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:26: self.enabled_ = false; On 2015/11/19 23:12:32, dpapad wrote: > Nit: How about turning enabled into a simple local var as follows? > > var enabled = false; > > Then in > ...setAdapterState = function() { > enabled = state.powered; > ... > } > and similarly in the assertions later. Done. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:28: chrome.bluetooth.getAdapterState = function() { On 2015/11/20 15:08:17, michaelpg wrote: > shouldn't this take a callback? > > if this is passing, that means setAdapterState is being called before > getAdapterState's callback is called.... Yes it should. Done. The test is passing because SettingsBluetoothPage.bluetoothEnabled is a boolean that defaults to 'false' so the result of getAdapterState doesn't matter. In the non-fake implementation, bluetoothEnabled will also be updated by the onAdapterStateChanged event, but this doesn't test that yet. I actually need to do some minor re-factoring to make testing (and the code in general) a bit more robust, but I would rather do that separately. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:49: bluetoothSection.querySelector('settings-bluetooth-page'); On 2015/11/20 15:08:17, michaelpg wrote: > $$ because getSection returns a SettingsSection. > > also change getSection to @return {!SettingsSection} > > bonus points for changing getPage to @return {!PolymerElement} I changed the return types, but $$ doesn't work because <settings-bluetooth-page> is the contents of <settings-section>, and not a child of the shadow root. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:51: var enable = bluetooth.$$('#enableBluetooth'); On 2015/11/20 15:08:17, michaelpg wrote: > bluetooth.$.enableBluetooth Done. https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:57: var sections = page.shadowRoot.querySelectorAll('settings-section'); On 2015/11/20 15:08:18, michaelpg wrote: > $$ "Convenience method to run querySelector on this local DOM scope" (i.e. not querySelectorAll).
https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:28: callback({ Where is 'callback' defined? Should be passed as a parameter? https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:24: browsePreload: 'chrome://md-settings/', FYI, the test I added a few days ago, which loads the same URL has been disabled by a sheriff, see https://code.google.com/p/chromium/issues/detail?id=558434. I am still trying to figure out why it was sometimes timing out. My guess is that this test might have the same problem.
https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:28: callback({ On 2015/11/20 18:05:22, dpapad wrote: > Where is 'callback' defined? Should be passed as a parameter? Gah, yes. Done. Need to closure working for tests (but unfortunately that looks to be non trivial). https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:24: browsePreload: 'chrome://md-settings/', On 2015/11/20 18:05:22, dpapad wrote: > FYI, the test I added a few days ago, which loads the same URL has been disabled > by a sheriff, see https://code.google.com/p/chromium/issues/detail?id=558434. I > am still trying to figure out why it was sometimes timing out. My guess is that > this test might have the same problem. Hmm. I'll keep an eye on that (we already landed main_page_browsertest.js). I believe that SettingsMainPageBrowserTest should cover what Settings[Advanced|Basic]BrowserTest do, so if it turns out to be a framework issue (i.e. these tests are less flaky), we could just abandon that one.
https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:49: bluetoothSection.querySelector('settings-bluetooth-page'); On 2015/11/20 17:47:40, stevenjb wrote: > On 2015/11/20 15:08:17, michaelpg wrote: > > $$ because getSection returns a SettingsSection. > > > > also change getSection to @return {!SettingsSection} > > > > bonus points for changing getPage to @return {!PolymerElement} > > I changed the return types, but $$ doesn't work because > <settings-bluetooth-page> is the contents of <settings-section>, and not a child > of the shadow root. Acknowledged. https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:24: browsePreload: 'chrome://md-settings/', On 2015/11/20 18:28:16, stevenjb wrote: > On 2015/11/20 18:05:22, dpapad wrote: > > FYI, the test I added a few days ago, which loads the same URL has been > disabled > > by a sheriff, see https://code.google.com/p/chromium/issues/detail?id=558434. > I > > am still trying to figure out why it was sometimes timing out. My guess is > that > > this test might have the same problem. > > Hmm. I'll keep an eye on that (we already landed main_page_browsertest.js). > > I believe that SettingsMainPageBrowserTest should cover what > Settings[Advanced|Basic]BrowserTest do, so if it turns out to be a framework > issue (i.e. these tests are less flaky), we could just abandon that one. I agree with dpapad that if his test times out, this one probably would too because it also loads the whole page. My guess is the page is just too darn slow.
lgtm https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:25: var enabled_ = false; Nit: Local vars don't need underscore at the end (usually used to indicate a private member var). https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:55: expectTrue(enable.checked); What is the difference of expectTrue and assertTrue?
https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:24: browsePreload: 'chrome://md-settings/', On 2015/11/20 18:46:37, michaelpg wrote: > On 2015/11/20 18:28:16, stevenjb wrote: > > On 2015/11/20 18:05:22, dpapad wrote: > > > FYI, the test I added a few days ago, which loads the same URL has been > > disabled > > > by a sheriff, see > https://code.google.com/p/chromium/issues/detail?id=558434. > > I > > > am still trying to figure out why it was sometimes timing out. My guess is > > that > > > this test might have the same problem. > > > > Hmm. I'll keep an eye on that (we already landed main_page_browsertest.js). > > > > I believe that SettingsMainPageBrowserTest should cover what > > Settings[Advanced|Basic]BrowserTest do, so if it turns out to be a framework > > issue (i.e. these tests are less flaky), we could just abandon that one. > > I agree with dpapad that if his test times out, this one probably would too > because it also loads the whole page. My guess is the page is just too darn > slow. I'll keep an eye on SettingsMainPageBrowserTest before committing this. I'll also do some local benchmarking on that and Settings[Advanced|Basic]BrowserTest to see if there is a noticeable difference. https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:55: expectTrue(enable.checked); On 2015/11/20 18:49:43, dpapad wrote: > What is the difference of expectTrue and assertTrue? asset will fail and stop the test immediately. It is useful for things that should never fail or would invalidate the rest of the test. Here, if enable.checked is false, enabled_ might still be true which means that the 'changed' event and fake API is working but the toggle itself is somehow not (or is getting toggled back).
https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:27: chrome.bluetooth.getAdapterState = function(callback) { Is it useful to add a short timeout/delay to simulate the async call? (I've found that useful in some C++ tests, but maybe JS is different enough that a simulated delay wouldn't be necessary)?
lgtm
https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:25: var enabled_ = false; On 2015/11/20 18:49:43, dpapad wrote: > Nit: Local vars don't need underscore at the end (usually used to indicate a > private member var). Done. https://codereview.chromium.org/1466433002/diff/10006/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:27: chrome.bluetooth.getAdapterState = function(callback) { On 2015/11/20 22:22:36, dschuyler wrote: > Is it useful to add a short timeout/delay to simulate the async call? > (I've found that useful in some C++ tests, but maybe JS is different enough that > a simulated delay wouldn't be necessary)? We don't need an actual delay (and should generally avoid them if possible in tests), but deferring this with setTimeut(callback(...)) does make sense. Done.
https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:57: var sections = page.shadowRoot.querySelectorAll('settings-section'); On 2015/11/20 17:47:41, stevenjb wrote: > On 2015/11/20 15:08:18, michaelpg wrote: > > $$ > > "Convenience method to run querySelector on this local DOM scope" (i.e. not > querySelectorAll). Sorry, got a bit overzealous in reducing wordiness! https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:24: browsePreload: 'chrome://md-settings/', On 2015/11/20 18:55:03, stevenjb wrote: > On 2015/11/20 18:46:37, michaelpg wrote: > > On 2015/11/20 18:28:16, stevenjb wrote: > > > On 2015/11/20 18:05:22, dpapad wrote: > > > > FYI, the test I added a few days ago, which loads the same URL has been > > > disabled > > > > by a sheriff, see > > https://code.google.com/p/chromium/issues/detail?id=558434. > > > I > > > > am still trying to figure out why it was sometimes timing out. My guess is > > > that > > > > this test might have the same problem. > > > > > > Hmm. I'll keep an eye on that (we already landed main_page_browsertest.js). > > > > > > I believe that SettingsMainPageBrowserTest should cover what > > > Settings[Advanced|Basic]BrowserTest do, so if it turns out to be a framework > > > issue (i.e. these tests are less flaky), we could just abandon that one. > > > > I agree with dpapad that if his test times out, this one probably would too > > because it also loads the whole page. My guess is the page is just too darn > > slow. > > I'll keep an eye on SettingsMainPageBrowserTest before committing this. I'll > also do some local benchmarking on that and Settings[Advanced|Basic]BrowserTest > to see if there is a noticeable difference. Looks like SettingsMainPageBrowserTest.Main is also timing out repeatedly: http://chromium-build-logs.appspot.com/gtest_query?gtest_query=SettingsMainPa... No idea why it's only happening on "Linux Tests (dbg)(1)" though. https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:39: setTimeout(callback()); Did you mean this without the parens, so "callback" itself is deferred? FWIW I'm surprised setTimeout(undefined) doesn't error. I suppose it's using setTimeout(code) instead of setTimeout(function) and just creates an empty script... https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:34: * @return {!PolymerElement} The PolymerElement for the page. Cool, I didn't even know this extern existed. https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:52: * @param {!Node} page The DOM node for the page containing |section|. !PolymerElement
PTAL https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:39: setTimeout(callback()); On 2015/11/24 15:55:49, michaelpg wrote: > Did you mean this without the parens, so "callback" itself is deferred? > > FWIW I'm surprised setTimeout(undefined) doesn't error. I suppose it's using > setTimeout(code) instead of setTimeout(function) and just creates an empty > script... Doh. Yes. I think the other one is wrong too. Isn't JS fun? :) Fixed. https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:34: * @return {!PolymerElement} The PolymerElement for the page. On 2015/11/24 15:55:49, michaelpg wrote: > Cool, I didn't even know this extern existed. Acknowledged. https://codereview.chromium.org/1466433002/diff/70001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:52: * @param {!Node} page The DOM node for the page containing |section|. On 2015/11/24 15:55:49, michaelpg wrote: > !PolymerElement Done.
Doing a rough scan of the output, SettingsMainPageBrowserTest is taking around 18s on Linux Tests (dbg)(1), so yeah, we should disable them in DEBUG for now. I will do that separately, before I commit this, and update this test also. https://chromium-swarm.appspot.com/user/task/2b531ee37c2ed610
lgtm
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, dpapad@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps110001 (title: "Disable test on Debug builds")
The CQ bit was unchecked by stevenjb@chromium.org
Patchset #9 (id:150001) has been deleted
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps130001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:170001) has been deleted
Patchset #9 (id:190001) has been deleted
Patchset #9 (id:210001) has been deleted
This has been re-factored to use the FakeBluetooth and FakeBluetoothPrivate APIs, with more tests added. PTAL
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:40: bluetoothApiForTest = this.bluetoothApi_; Where is bluetoothApiForTest defined, and where is it being used? Could not find it. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:91: bluetoothSection.querySelector('settings-bluetooth-page'); Nit: use $$() shorthand. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:6: * @fileoverview Fake implementations of chrome.bluetooth and for testing. s/and// https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:15: this.enabled = false; Should we add type annotations for all members? I think it is helpful even if this file is not being compiled. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:18: this.setDevicesForTest = function(devices) { Can we move this on the prototype? Methods have no good reason to be defined in the constructor. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:32: getAdapterState: function(callback) { If these methods are overrides, why do we need to replicate type annotations? /** @override */ should be enough. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth_private.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:11: * @param {Bluetooth} bluetoothApi !Bluetooth https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:16: this.bluetoothApi_ = bluetoothApi; /** @private {!Bluetooth} */ https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:22: * @param {function():void=} callback The convention is that optional parameters are named opt_fooBar, in this case opt_callback, see https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:26: setTimeout(callback); The 2nd param to setTimeout is required according to the docs. Not sure why compiler is not catching this (or most likely this file is not being type-checked by the compiler). Let's change this to setTimeout(callback, 0); Also a comment explaining on why do we do this instead of calling the callback directly would be useful. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:67: onPairing: new FakeChromeEvent(), Shouldn't this be placed in the constructor instead? See convention (methods on prototype, attributes in constructor) at https://google.github.io/styleguide/javascriptguide.xml?showone=Method_and_pr....
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:59: // and test() will be a Mocha 'Suite' or 'Test' instance. I'm having trouble parsing this comment. I'm wondering if s/and test/because in test() this/ would read as intended? https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:98: // bluetooth.setAdapterState with pwered = false. is pwered a mistype of powered? https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:48: getDevice: assertNotReached, If this were to fire (in error), how would the person debugging this error know it was an invalid call to getDevice? Would this be nicer as getDevice: function(deviceAddress, callback) { assertNotReached(); },
Patchset #10 (id:250001) has been deleted
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:40: bluetoothApiForTest = this.bluetoothApi_; On 2015/12/16 22:07:48, dpapad wrote: > Where is bluetoothApiForTest defined, and where is it being used? Could not find > it. It, along with bluetoothPrivateApiForTest are defined in bluetooth_page.js. I will rename them bluetoothPage.bluetoothApiForTest, etc. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:59: // and test() will be a Mocha 'Suite' or 'Test' instance. On 2015/12/16 22:15:01, dschuyler wrote: > I'm having trouble parsing this comment. I'm wondering if s/and test/because in > test() this/ would read as intended? That would be: // Assign |self| to |this| instead of binding since 'this' in suite() // because in test() this will be a Mocha 'Suite' or 'Test' instance. Which doesn't parse either. I will try to create a better description in mocha_adapter.js and reference that here. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:91: bluetoothSection.querySelector('settings-bluetooth-page'); On 2015/12/16 22:07:48, dpapad wrote: > Nit: use $$() shorthand. Not actually the same thing. $$ queries the shadow-root, we need to query the contents of the section here. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:98: // bluetooth.setAdapterState with pwered = false. On 2015/12/16 22:15:01, dschuyler wrote: > is pwered a mistype of powered? Done. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:6: * @fileoverview Fake implementations of chrome.bluetooth and for testing. On 2015/12/16 22:07:49, dpapad wrote: > s/and// Done. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:15: this.enabled = false; On 2015/12/16 22:07:49, dpapad wrote: > Should we add type annotations for all members? I think it is helpful even if > this file is not being compiled. Personally I think that typing everything quickly becomes tedious and loses some of the benefits of Javascript's flexibility. For exposed (public) members and functions I agree that it is extremely helpful, but for a local member like this which is clearly a boolean, I don't see the advantage. If the consensus differs, I can be more diligent, but I do feel that it would slow me down. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:18: this.setDevicesForTest = function(devices) { On 2015/12/16 22:07:49, dpapad wrote: > Can we move this on the prototype? Methods have no good reason to be defined in > the constructor. Sure. It seemed better to put just the API implementation in the prototype, but I'll admit I'm still unclear on the advantages of using a prototype at all for a singleton instance like this. (I started with the FakeSettingsPrivate implementation). https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:32: getAdapterState: function(callback) { On 2015/12/16 22:07:49, dpapad wrote: > If these methods are overrides, why do we need to replicate type annotations? > /** @override */ should be enough. I didn't realize that. Done. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:48: getDevice: assertNotReached, On 2015/12/16 22:15:01, dschuyler wrote: > If this were to fire (in error), how would the person debugging this error know > it was an invalid call to getDevice? Would this be nicer as > getDevice: function(deviceAddress, callback) { > assertNotReached(); > }, I don't think that would help in a test where we have to rely on console output which is just a call stack. We would have to do something like: assertNotReached('Unexpected call, deviceAddress = ' + deviceAddress); Which would involve a fair bit of extra work for something we don't expect to happen. Also, but the time this section is fully implemented and reasonably well tested, most of this API will be implemented. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth_private.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:11: * @param {Bluetooth} bluetoothApi On 2015/12/16 22:07:49, dpapad wrote: > !Bluetooth Done. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:16: this.bluetoothApi_ = bluetoothApi; On 2015/12/16 22:07:49, dpapad wrote: > /** @private {!Bluetooth} */ Done. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:22: * @param {function():void=} callback On 2015/12/16 22:07:49, dpapad wrote: > The convention is that optional parameters are named opt_fooBar, in this case > opt_callback, see > https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming. Removed in favor of @override. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:26: setTimeout(callback); On 2015/12/16 22:07:49, dpapad wrote: > The 2nd param to setTimeout is required according to the docs. Not sure why > compiler is not catching this (or most likely this file is not being > type-checked by the compiler). Let's change this to > > setTimeout(callback, 0); > > Also a comment explaining on why do we do this instead of calling the callback > directly would be useful. Hmm, not according to https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout, We use this pattern (with no delay argument) pretty often to simulate asynchronous behavior. I'd like to think that is well enough understood not to comment that every time we use this pattern. (We can't currently compile the tests without a lot of effort to get testing.Test to compile, I tried). https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:67: onPairing: new FakeChromeEvent(), On 2015/12/16 22:07:49, dpapad wrote: > Shouldn't this be placed in the constructor instead? See convention (methods on > prototype, attributes in constructor) at > https://google.github.io/styleguide/javascriptguide.xml?showone=Method_and_pr.... Let's discuss this here: https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... While I kind of sorta understand the intent of the guideline, it's pretty convenient to be able to fake out the entire interface in one place, and this is a singleton object, used only in tests.
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:59: // and test() will be a Mocha 'Suite' or 'Test' instance. On 2015/12/17 at 00:13:34, stevenjb wrote: > On 2015/12/16 22:15:01, dschuyler wrote: > > I'm having trouble parsing this comment. I'm wondering if s/and test/because in > > test() this/ would read as intended? > > That would be: > > // Assign |self| to |this| instead of binding since 'this' in suite() > // because in test() this will be a Mocha 'Suite' or 'Test' instance. > > Which doesn't parse either. I will try to create a better description in mocha_adapter.js and reference that here. FWIW (I know we talked about this before), I find the var self = this trick unnecessary when we can simply bind to this, and use this within the test. I remember from previous discussion that if the anonymous function is not explicitly bound, mocha binds it to some control object, allowing to affect the test's lifetime, but since we are not using this control object, we could eliminate the need for this possibly confusing comment by simply binding the anonymous function to this. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:15: this.enabled = false; On 2015/12/17 at 00:13:34, stevenjb wrote: > On 2015/12/16 22:07:49, dpapad wrote: > > Should we add type annotations for all members? I think it is helpful even if > > this file is not being compiled. > > Personally I think that typing everything quickly becomes tedious and loses some of the benefits of Javascript's flexibility. For exposed (public) members and functions I agree that it is extremely helpful, but for a local member like this which is clearly a boolean, I don't see the advantage. > > If the consensus differs, I can be more diligent, but I do feel that it would slow me down. If you don't add @private and/or rename it to this.enabled_, it is not clear at all that this is a private member (see https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming for trailing underscore guideline). The reader has to search the file everytime to determine whether this is a private field. At the very least I would suggest adding the underscore at the end, and we can discuss whether we need to add types next time we meet (meaning when everyone is back from vacation). https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth_private.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth_private.js:26: setTimeout(callback); On 2015/12/17 at 00:13:35, stevenjb wrote: > On 2015/12/16 22:07:49, dpapad wrote: > > The 2nd param to setTimeout is required according to the docs. Not sure why > > compiler is not catching this (or most likely this file is not being > > type-checked by the compiler). Let's change this to > > > > setTimeout(callback, 0); > > > > Also a comment explaining on why do we do this instead of calling the callback > > directly would be useful. > Hmm, not according to https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout, > > We use this pattern (with no delay argument) pretty often to simulate asynchronous behavior. I'd like to think that is well enough understood not to comment that every time we use this pattern. > > (We can't currently compile the tests without a lot of effort to get testing.Test to compile, I tried). Ah, I see that is fine then. There is a mismatch in documentation, I looked it up at http://www.w3schools.com/jsref/met_win_settimeout.asp where it says "required" go figure. Also I can only imagine how hard it would be to compile test_api.js
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:59: // and test() will be a Mocha 'Suite' or 'Test' instance. On 2015/12/17 01:12:54, dpapad wrote: > On 2015/12/17 at 00:13:34, stevenjb wrote: > > On 2015/12/16 22:15:01, dschuyler wrote: > > > I'm having trouble parsing this comment. I'm wondering if s/and > test/because in > > > test() this/ would read as intended? > > > > That would be: > > > > // Assign |self| to |this| instead of binding since 'this' in suite() > > // because in test() this will be a Mocha 'Suite' or 'Test' instance. > > > > Which doesn't parse either. I will try to create a better description in > mocha_adapter.js and reference that here. > > FWIW (I know we talked about this before), I find the var self = this trick > unnecessary when we can simply bind to this, and use this within the test. I > remember from previous discussion > that if the anonymous function is not explicitly bound, mocha binds it to some > control object, allowing to affect the test's lifetime, but since we are not > using this control object, > we could eliminate the need for this possibly confusing comment by simply > binding the anonymous function to this. If we ever need to reference the Mocha Suite or Test object it would be extremely confusing. I think this is better / more consistent, although I could be convinced to name it something like 'test' instead of 'self'. If you can convince Michael otherwise, we can change these in a follow-up. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/fake_bluetooth.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/fake_bluetooth.js:15: this.enabled = false; On 2015/12/17 01:12:54, dpapad wrote: > On 2015/12/17 at 00:13:34, stevenjb wrote: > > On 2015/12/16 22:07:49, dpapad wrote: > > > Should we add type annotations for all members? I think it is helpful even > if > > > this file is not being compiled. > > > > Personally I think that typing everything quickly becomes tedious and loses > some of the benefits of Javascript's flexibility. For exposed (public) members > and functions I agree that it is extremely helpful, but for a local member like > this which is clearly a boolean, I don't see the advantage. > > > > If the consensus differs, I can be more diligent, but I do feel that it would > slow me down. > > If you don't add @private and/or rename it to this.enabled_, it is not clear at > all that this is a private member (see > https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming > for trailing underscore guideline). The reader has to search the file everytime > to determine whether this is a private field. At the very least I would suggest > adding the underscore at the end, and we can discuss whether we need to add > types next time we meet (meaning when everyone is back from vacation). So, this isn't actually private, we do use it elsewhere, so my own argument suggests that we should document these. Done.
lgtm
lgtm LGTM with suggestion. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:40: bluetoothApiForTest = this.bluetoothApi_; On 2015/12/17 at 00:13:34, stevenjb wrote: > On 2015/12/16 22:07:48, dpapad wrote: > > Where is bluetoothApiForTest defined, and where is it being used? Could not find > > it. > > It, along with bluetoothPrivateApiForTest are defined in bluetooth_page.js. > I will rename them bluetoothPage.bluetoothApiForTest, etc. Ok. I am guessing that those two objects are being used during loading of the page, right? Otherwise you could simply expose two method setBluetoothApi,setPrivateBlueTootApi on the <settings-bluetooth-page> element itself and call them here before any of the tests below execute. This would avoid the usage of global variables. https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:59: // and test() will be a Mocha 'Suite' or 'Test' instance. It's fine to leave as is and address in a follow up. My view is that we are making the main use case (where we don't use the object passed by mocha) more cryptic, for the possibility that we might use it one day. If that happens we could apply the trick on that particular test method only, instead of applying the trick in every method preemptively.
https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest.js (right): https://codereview.chromium.org/1466433002/diff/230001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest.js:40: bluetoothApiForTest = this.bluetoothApi_; On 2015/12/17 02:06:58, dpapad wrote: > On 2015/12/17 at 00:13:34, stevenjb wrote: > > On 2015/12/16 22:07:48, dpapad wrote: > > > Where is bluetoothApiForTest defined, and where is it being used? Could not > find > > > it. > > > > It, along with bluetoothPrivateApiForTest are defined in bluetooth_page.js. > > I will rename them bluetoothPage.bluetoothApiForTest, etc. > > Ok. I am guessing that those two objects are being used during loading of the > page, right? Otherwise you > could simply expose two method setBluetoothApi,setPrivateBlueTootApi on the > <settings-bluetooth-page> element > itself and call them here before any of the tests below execute. This would > avoid the usage of global > variables. Correct. Once a DOM element is stamped, we have no hooks that allow us to affect the associated Polymer object before ready() and attached() get called, so unfortunately we need to use a global variable. (The page calls into the bluetooth APIs in ready() to request paired devices).
https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:21: var bluetoothPage = bluetoothPage || { use cr.define? https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:65: bluetooth: { does this need to be a Polymer property vs. a vanilla property on the prototype? https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:74: bluetoothPrivate: { same
https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:21: var bluetoothPage = bluetoothPage || { On 2015/12/17 22:07:16, michaelpg wrote: > use cr.define? Hm, interesting question. I'm a bit reluctant to "pollute" the cr namespace with test specific objects. I'd be curious to see what Dan has to say, but if you're pretty certain that would be preferable I can change this now, otherwise we can re-visit it later. https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:65: bluetooth: { On 2015/12/17 22:07:17, michaelpg wrote: > does this need to be a Polymer property vs. a vanilla property on the prototype? It will likely get passed to subpages, so probably. I could make it vanilla for now if you would prefer.
P.S. Please make sure to take a look at the CL that this one depends on: https://codereview.chromium.org/1532503003/ On Thu, Dec 17, 2015 at 2:13 PM, <stevenjb@chromium.org> wrote: > > > https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... > File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js > (right): > > > https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... > chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:21: > var bluetoothPage = bluetoothPage || { > On 2015/12/17 22:07:16, michaelpg wrote: > >> use cr.define? >> > > Hm, interesting question. I'm a bit reluctant to "pollute" the cr > namespace with test specific objects. I'd be curious to see what Dan has > to say, but if you're pretty certain that would be preferable I can > change this now, otherwise we can re-visit it later. > > > https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... > chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:65: > bluetooth: { > On 2015/12/17 22:07:17, michaelpg wrote: > >> does this need to be a Polymer property vs. a vanilla property on the >> > prototype? > > It will likely get passed to subpages, so probably. I could make it > vanilla for now if you would prefer. > > https://codereview.chromium.org/1466433002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
SLGTM
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, dschuyler@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps350001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps370001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/370001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps390001 (title: "Make Chrome OS only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
You already have 3 reviewers, so I know you don't need more, but... lgtm I think this CL is very elegant and provides lots of great precedence, I am still worried about timeouts, though. https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:13: // the Mocha Suite or Test instance. this is probably more a general understanding of |this| in JS, though I suppose it doesn't hurt. are the test cases actually being .call()'d or .apply()'d with different objects? https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:90: assertFalse(self.bluetoothApi_.enabled); yeah, this just a case of |this| changing when introducing a new scope https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:115: test('device list', function() { I think we still might be better off doing the minimal amount per browser_test, due to timeouts. This might be accomplished by moving the setup stuff from the top of SettingsBluetoothPageBrowserTest.BlueTooth to the fixture, i.e. SettingsBluetoothPageBrowserTest.prototype = { ... setUp: function() { ... getPage('advanced') ... ... set this.* stuff to be used in test cases ... }, }; And then doing TEST_F('test 1', ... suite(), test() ...), TEST_F('test 2', suite(), test() ...) I'm still really worried about timeouts (and the fact that you have to add this test with a MAYBE_ is kind of an indicator we all should be as well).
https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:13: // the Mocha Suite or Test instance. On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > this is probably more a general understanding of |this| in JS, though I suppose > it doesn't hurt. are the test cases actually being .call()'d or .apply()'d with > different objects? Yes, the test cases are call'd or apply'd on a particular object, the Context object. This can be used to change timeouts, e.g.: test('foo', function() { this.timeout(0); }); It can also be used to share data between tests and other hooks, which looks like a terrible idea: https://github.com/mochajs/mocha/wiki/Shared-Behaviours I'm kind of on the fence as to whether it's okay to bind these functions to `this`: test('foo', function() { this.getSettingsPage(); }.bind(this)); I don't think that will break Mocha, but it can lead to an annoyingly high number of .bind calls with nested suites. https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:90: assertFalse(self.bluetoothApi_.enabled); On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > yeah, this just a case of |this| changing when introducing a new scope these functions are .call()d with a particular context object. i think it's either: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mocha/... or https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mocha/...
https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:13: // the Mocha Suite or Test instance. On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > this is probably more a general understanding of |this| in JS, though I suppose > it doesn't hurt. are the test cases actually being .call()'d or .apply()'d with > different objects? There has been some confusion and discussion about the right model (whether to use 'self' or .bind(this)), which is why I added the comment. I'm not certain how mocha invokes the suites/tests. We may want to discuss this further. https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... File chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js (right): https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:90: assertFalse(self.bluetoothApi_.enabled); On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > yeah, this just a case of |this| changing when introducing a new scope Acknowledged. https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:115: test('device list', function() { On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > I think we still might be better off doing the minimal amount per browser_test, > due to timeouts. > > This might be accomplished by moving the setup stuff from the top of > SettingsBluetoothPageBrowserTest.BlueTooth to the fixture, i.e. > > SettingsBluetoothPageBrowserTest.prototype = { > ... > setUp: function() { > ... getPage('advanced') ... > ... set this.* stuff to be used in test cases ... > }, > }; > > And then doing TEST_F('test 1', ... suite(), test() ...), TEST_F('test 2', > suite(), test() ...) > > I'm still really worried about timeouts (and the fact that you have to add this > test with a MAYBE_ is kind of an indicator we all should be as well). Understood, but I am also worried about per-test overhead. The vast majority of the overhead is the initial page load, I don't think each test() invocation costs very much, but I can verify that. https://codereview.chromium.org/1466433002/diff/390001/chrome/test/data/webui... chrome/test/data/webui/settings/bluetooth_page_browsertest_chromeos.js:115: test('device list', function() { On 2015/12/28 20:01:39, Dan Beam (ooo till 12-26) wrote: > I think we still might be better off doing the minimal amount per browser_test, > due to timeouts. > > This might be accomplished by moving the setup stuff from the top of > SettingsBluetoothPageBrowserTest.BlueTooth to the fixture, i.e. > > SettingsBluetoothPageBrowserTest.prototype = { > ... > setUp: function() { > ... getPage('advanced') ... > ... set this.* stuff to be used in test cases ... > }, > }; > > And then doing TEST_F('test 1', ... suite(), test() ...), TEST_F('test 2', > suite(), test() ...) > > I'm still really worried about timeouts (and the fact that you have to add this > test with a MAYBE_ is kind of an indicator we all should be as well). Understood, but I am also worried about per-test overhead. The vast majority of the overhead is the initial page load, I don't think each test() invocation costs very much, but I can verify that.
Description was changed from ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 ========== to ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR ==========
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, dbeam@chromium.org, michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1466433002/#ps430001 (title: "Chrome OS only Take 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/430001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR ========== to ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR=sky@chromium.org ==========
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466433002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466433002/430001
still lgtm though I agree with michaelpg@ about sticking to precedence where possible re: cr.define or cr.exportPath (i'd rather keep the code similar for all webui, even if cr.* doesn't carry over in the eventual future) https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/1466433002/diff/330001/chrome/browser/resourc... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:21: var bluetoothPage = bluetoothPage || { On 2015/12/17 22:13:57, stevenjb wrote: > On 2015/12/17 22:07:16, michaelpg wrote: > > use cr.define? > > Hm, interesting question. I'm a bit reluctant to "pollute" the cr namespace with > test specific objects. I'd be curious to see what Dan has to say, but if you're > pretty certain that would be preferable I can change this now, otherwise we can > re-visit it later. cr.exportPath('bluetoothPage'); seems to do something like this i wasn't sure if this was just an object or a namespace. the distinction is hazy in JS. I'm fine with either way but define() or exportPath() have mild benefits as we can change all the behaviors of these at once (from both the runtime and compiler pass's perspectives).
Message was sent while issue was closed.
Description was changed from ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR=sky@chromium.org ========== to ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #18 (id:430001)
Message was sent while issue was closed.
Description was changed from ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR=sky@chromium.org ========== to ========== Add Settings bluetooth page test This includes a minor change to settings_page_browsertest.js to use $$ instead of shadowRoot.querySelector (thanks to dschuyler@ for the reminder). BUG=543299 For simple change to BUILD.gn: TBR=sky@chromium.org Committed: https://crrev.com/dc099e3095d5152131e84991125db6311cfb1568 Cr-Commit-Position: refs/heads/master@{#367018} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/dc099e3095d5152131e84991125db6311cfb1568 Cr-Commit-Position: refs/heads/master@{#367018} |
