Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1462723002:
[MD settings] adding a test for Appearance settings (Closed)
|
Created:
5 years, 1 month ago by dschuyler Modified:
5 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] adding a test for Appearance settings
This CL is the start for unit tests for the Appearance MD settings.
Further tests will be added over time.
BUG=531786
Committed: https://crrev.com/a3ae925f79c8d86fb1a4c7651e38c56cb6e1fe84
Cr-Commit-Position: refs/heads/master@{#361461}
Patch Set 1 #Patch Set 2 : unwrapping line #
Total comments: 10
Patch Set 3 : review changes #Patch Set 4 : review changes #
Total comments: 10
Patch Set 5 : review changes #Patch Set 6 : review change #Patch Set 7 : patch fix #
Messages
Total messages: 26 (9 generated)
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, I'm still looking at how others have been using the mocha testing. I wanted to begin somewhere, so this CL has a couple of simple tests to get started with. I'm hoping for review feedback on whether is a reasonable start, it's not intended to be a thorough set tests yet. Thanks!
just some unsolicited feedback :D https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:35: /** @return {String} */ string (lowercase) https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:39: var settingsUi = crSettings.shadowRoot.querySelector('settings-ui'); Polymer provides $$ to querySelector a Polymer elment's shadow root: crSettings.$$('settings-ui') https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:53: // Run all tests. comment isn't really relevant here https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:61: assertTrue(fontSize.textContent == "Medium"); assertEquals('Medium', fS.tC); https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:66: assertTrue(homeButton.attributes['aria-pressed'].nodeValue == "false"); assertEquals('false', ...) also: why not homeButton.getAttribute('aria-pressed')?
Now with Michael's advice incorporated. https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:35: /** @return {String} */ On 2015/11/19 02:44:05, michaelpg wrote: > string (lowercase) Done. https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:39: var settingsUi = crSettings.shadowRoot.querySelector('settings-ui'); On 2015/11/19 02:44:04, michaelpg wrote: > Polymer provides $$ to querySelector a Polymer elment's shadow root: > > crSettings.$$('settings-ui') Done. https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:53: // Run all tests. On 2015/11/19 02:44:04, michaelpg wrote: > comment isn't really relevant here Done. https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:61: assertTrue(fontSize.textContent == "Medium"); On 2015/11/19 02:44:05, michaelpg wrote: > assertEquals('Medium', fS.tC); Done. https://codereview.chromium.org/1462723002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:66: assertTrue(homeButton.attributes['aria-pressed'].nodeValue == "false"); On 2015/11/19 02:44:04, michaelpg wrote: > assertEquals('false', ...) > > also: why not homeButton.getAttribute('aria-pressed')? Done.
This looks good, but you should go ahead and re-factor it with the support code I added here (apologies for not including you the CL, I should have): https://codereview.chromium.org/1457543004/ For an example of how I am using that in the bluetooth page test, see: https://codereview.chromium.org/1466433002/
Like this?
Yep! lgtm w/ comment fix https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:13: * @extends {PolymerTest} @extends {SettingsPageBrowserTest} https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); nit: Personally I would just assertTrue(!!fontSize.textContent) to ensure that it is defined and not empty so that the test doesn't break if we change the default, but I'm fine with this as-is.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); Dumb question: how do we know the prefs have loaded before this suite runs? (How do Options tests ensure the fetchPrefs callback has been run?)
Description was changed from ========== [MD settings] adding a test for Appearance settings This CL is the start for unit tests for the Appearance MD settings. Further tests will be added over time. BUG=531786 ========== to ========== [MD settings] adding a test for Appearance settings This CL is the start for unit tests for the Appearance MD settings. Further tests will be added over time. BUG=531786 ==========
michaelpg@chromium.org changed reviewers: - michaelpg@chromium.org
https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:7: // Polymer BrowserTest fixture. Michael pointed out this comment is no longer accurate and should probably just be removed. https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); On 2015/11/20 17:23:41, michaelpg wrote: > Dumb question: how do we know the prefs have loaded before this suite runs? > > (How do Options tests ensure the fetchPrefs callback has been run?) That's actually a good question. My guess is "they don't" but in practice we have enough "deferred layers"?? (I'm not sure what the right JS terminology here is) that any initial API callbacks will complete before we get here. i.e. The API call will execute some C++ code that will (effectively) immediately insert the JS callback to be run, and that will occur before the test has a chance to run. (I don't actually have any idea how task scheduling works in JS). Probably we ought to add a wrapper to mocha.run() in SettingsPageBrowserTest that waits until Prefs has loaded before calling mocha.run() ?
https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); On 2015/11/20 17:59:37, stevenjb wrote: > On 2015/11/20 17:23:41, michaelpg wrote: > > Dumb question: how do we know the prefs have loaded before this suite runs? > > > > (How do Options tests ensure the fetchPrefs callback has been run?) > > That's actually a good question. My guess is "they don't" but in practice we > have enough "deferred layers"?? (I'm not sure what the right JS terminology here > is) that any initial API callbacks will complete before we get here. i.e. The > API call will execute some C++ code that will (effectively) immediately insert > the JS callback to be run, and that will occur before the test has a chance to > run. (I don't actually have any idea how task scheduling works in JS). > > Probably we ought to add a wrapper to mocha.run() in SettingsPageBrowserTest > that waits until Prefs has loaded before calling mocha.run() ? I'm thinking that the prefs in this test will be replaced with the prefs fake. Since that will be JS, I think we will be able to control that more easily.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); On 2015/11/20 17:59:37, stevenjb wrote: > On 2015/11/20 17:23:41, michaelpg wrote: > > Dumb question: how do we know the prefs have loaded before this suite runs? > > > > (How do Options tests ensure the fetchPrefs callback has been run?) > > That's actually a good question. My guess is "they don't" but in practice we > have enough "deferred layers"?? (I'm not sure what the right JS terminology here > is) that any initial API callbacks will complete before we get here. i.e. The > API call will execute some C++ code that will (effectively) immediately insert > the JS callback to be run, and that will occur before the test has a chance to > run. (I don't actually have any idea how task scheduling works in JS). > > Probably we ought to add a wrapper to mocha.run() in SettingsPageBrowserTest > that waits until Prefs has loaded before calling mocha.run() ? We don't need a wrapper per se. In settings_page_browsertest.js we can override setUp (from testing.Test) and register a suite setup method that waits on the prefs initialized promise: /** @override */ setUp: function() { suiteSetup(function() { return CrSettingsPrefs.initialized; }); }
https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:7: // Polymer BrowserTest fixture. On 2015/11/20 17:59:37, stevenjb wrote: > Michael pointed out this comment is no longer accurate and should probably just > be removed. Done. https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:13: * @extends {PolymerTest} On 2015/11/20 17:16:07, stevenjb wrote: > @extends {SettingsPageBrowserTest} Done. https://codereview.chromium.org/1462723002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: assertEquals('Medium', fontSize.textContent); On 2015/11/20 17:16:07, stevenjb wrote: > nit: Personally I would just assertTrue(!!fontSize.textContent) to ensure that > it is defined and not empty so that the test doesn't break if we change the > default, but I'm fine with this as-is. Cool. I'd like to think about explicitly setting it to the medium font size and then test that. I'll put up a follow-up CL shortly and accept your offer to keep this as-is for a day or so.
Added the CrSettingsPrefs.initialized Michael suggested.
lgtm
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1462723002/#ps120001 (title: "patch fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462723002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462723002/120001
The CQ bit was unchecked by dschuyler@chromium.org
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462723002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462723002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a3ae925f79c8d86fb1a4c7651e38c56cb6e1fe84 Cr-Commit-Position: refs/heads/master@{#361461} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
