Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310843010:
Add Polymer tests for cr-settings-prefs (Closed)
|
Created:
5 years, 3 months ago by michaelpg Modified:
5 years, 3 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org, dschuyler Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Polymer tests for cr-settings-prefs
This lays the groundwork for testing our prefs element and includes
some basic tests.
The test includes a mock of settingsPrivate. It would be good to
come up with a better solution for making cr-settings-prefs use
the mock; right now I have it working with a global variable which is
less than ideal.
After this CL, I'll add sanity tests for list and dictionary prefs.
These CLs shouldn't *block* adding list & dict prefs, but I don't want
to commit new features in prefs.js without at least having tests locally.
So I may as well upload the tests for review as well.
BUG=532270
TESTS=CrSettingsBrowserTest
Committed: https://crrev.com/7091d052bfdec82bb28176dd072c10eafd24f7b3
Cr-Commit-Position: refs/heads/master@{#349268}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : preempt stevenjb comments :) #
Total comments: 40
Patch Set 3 : already reviewed #
Total comments: 10
Patch Set 4 : extract test cases and simplify code #Patch Set 5 : new changes #
Total comments: 11
Patch Set 6 : new changes 2 #Patch Set 7 #Patch Set 8 : feedback #Patch Set 9 : moar pretty #
Total comments: 4
Patch Set 10 : clarify test #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (7 generated)
Patchset #1 (id:1) has been deleted
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org, stevenjb@chromium.org
Take a look at your leisure. https://codereview.chromium.org/1310843010/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310843010/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:110: settingsApi_: chrome.settingsPrivate, is this type-able? {chrome.settingsPrivate} itself isn't a type.
I've uploaded a CL which follows up by adding support for lists of nested dictionaries, along with tests: https://codereview.chromium.org/1333473002/ (not for review) It's a WIP (it depends a lot on the outcome of this CL, and it'll be broken up into moar files). But I learned a couple things: * This polymorphic PrefWrapper business is no good. When a preference value can consist of nested lists and objects, it doesn't make sense to have ListPrefWrapper check equality for lists and DictPrefWrapper check equality for dictionaries because they each can contain other values. IMO writing one isEquals and one deepCopy function is simpler. We can still let the PrefWrapper decide so it's opaque to the prefs element. * According to my tests, the changes to prefs.js in that CL work as expected. * Tests are great.
https://codereview.chromium.org/1310843010/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1310843010/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:117: return PolymerTest.async(fn); this doesn't quite work. what I actually want is something like https://codereview.chromium.org/1333473002/diff/20001/chrome/test/data/webui/... (not ready for review)
Some updates. To clarify, *this* CL is ready for review. https://codereview.chromium.org/1310843010/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1310843010/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:117: return PolymerTest.async(fn); On 2015/09/08 06:26:59, michaelpg wrote: > this doesn't quite work. what I actually want is something like > https://codereview.chromium.org/1333473002/diff/20001/chrome/test/data/webui/... > (not ready for review) Ignore this comment, see new patch.
https://codereview.chromium.org/1310843010/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:136: if (!key) Should this normally happen? If not, maybe log an error? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:127: this.addPref_('BOOLEAN', key, value); chrome.settingsPrivate.PrefType.BOOLEAN (throughout) https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:152: return JSON.parse(JSON.stringify(pref)); Can we use Object.Assign here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:226: } nit: Maybe wrap the above in a function to avoid the nested break/continue which is a bit awkward, e.g. var actualPref = getActualPref(key, pref); if (!expectNotEquals(undefined, actualPref)) continue;
https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:104: PolymerTest.async = function(fn) { initialize lastPromise_ to Promise.resolve(), remove if (!PolymerTest.lastPromise_) https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:122: }; wat? why not? return PolymerTest.lastPromise_; so callers can chain? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:37: expectNotEquals(null, this.listener_); ^ why? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:94: assertNotEquals(null, this.listener_); assert(this.listener_); (or just remove this because it'll blow up later) https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:127: this.addPref_('BOOLEAN', key, value); are these values ('BOOLEAN', 'STRING', 'NUMBER') in an @enum {string} somewhere? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:152: return JSON.parse(JSON.stringify(pref)); I guess using Object.create() here would be too much of a hack? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:164: var interval = setInterval(function() { ew https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:169: }); wait, do this every 0ms? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:193: document.body.appendChild(prefs); damn it http://jsfiddle.net/11hefdtv/ https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:198: if (!CrSettingsPrefs.isInitialized) { can haz promise instead? CrSettingsPrefs.initialized.then()? maybe todo? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:236: JSON.stringify(actualPref.value)); just do this for numbers/integers as well? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:244: expectTrue(mockApi.prefs['browser.enable_flash'].value); \n https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:246: expectEquals(1.2, mockApi.prefs['device.overclock'].value); \n https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:252: prefs.set('prefs.browser.enable_flash.value', false); \n https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:254: prefs.set('prefs.device.overclock.value', 2.0); \n https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:278: }).async(function() { can't PolymerTest.async() just return a promise and we can do PolymerTest.async().then()?
https://codereview.chromium.org/1310843010/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:136: if (!key) On 2015/09/11 21:03:47, stevenjb wrote: > Should this normally happen? If not, maybe log an error? I can't recall why or when this may happen. It's probably obsolete and doesn't seem to get happen anymore, so I removed it. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:104: PolymerTest.async = function(fn) { On 2015/09/11 21:49:12, Dan Beam wrote: > initialize lastPromise_ to Promise.resolve(), remove if > (!PolymerTest.lastPromise_) nice! https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:122: }; On 2015/09/11 21:49:12, Dan Beam wrote: > wat? why not? > > return PolymerTest.lastPromise_; > > so callers can chain? Done. this was just sugar. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:37: expectNotEquals(null, this.listener_); On 2015/09/11 21:49:13, Dan Beam wrote: > ^ why? why not? I don't much care; nobody calls this anyway. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:94: assertNotEquals(null, this.listener_); On 2015/09/11 21:49:12, Dan Beam wrote: > assert(this.listener_); (or just remove this because it'll blow up later) I can't figure out why, but assert doesn't play nicely with the rest of the test. Unlike the other assertion functions, assert is defined in ui/webui/resources/js/assert.js, not test_api.js. But it should still work... yet calling it results in mocha catching a global error which it can't provide any information for. I've verified that assert.toString() is the same as in assert.js. And if I copy and paste the function from assert.js into test_api.js, calling it works as expected (we get a nice pretty error message). I can only suspect test_api.js is intercepting the call or the exception in some weird way. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:127: this.addPref_('BOOLEAN', key, value); On 2015/09/11 21:49:12, Dan Beam wrote: > are these values ('BOOLEAN', 'STRING', 'NUMBER') in an @enum {string} somewhere? Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:127: this.addPref_('BOOLEAN', key, value); On 2015/09/11 21:03:47, stevenjb wrote: > chrome.settingsPrivate.PrefType.BOOLEAN (throughout) Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:152: return JSON.parse(JSON.stringify(pref)); On 2015/09/11 21:03:47, stevenjb wrote: > Can we use Object.Assign here? > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:152: return JSON.parse(JSON.stringify(pref)); On 2015/09/11 21:49:12, Dan Beam wrote: > I guess using Object.create() here would be too much of a hack? Used Object.assign instead. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:164: var interval = setInterval(function() { On 2015/09/11 21:49:12, Dan Beam wrote: > ew agreed. not sure how to solve this, though, without imposing specifications on how many tasks (setTimeout) the API and UI are allowed to post. we could make things easier by making the mock API synchronous, but I think our code should be allowed to assume the API is async. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:169: }); On 2015/09/11 21:49:12, Dan Beam wrote: > wait, do this every 0ms? Yeah, it posts a task each time. If a function posts five tasks (using setTimeout) this function will run at most five times. So these calls are interleaved with the running test; it doesn't spin for dozens/hundreds of cycles. Changing the timeout from 0ms to 100ms is unlikely to save much work and will just slow down the test (albeit not by much). Maybe we should impose a limit, e.g. can't be called more than 10 times? https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:193: document.body.appendChild(prefs); On 2015/09/11 21:49:13, Dan Beam wrote: > damn it http://jsfiddle.net/11hefdtv/ Yyyyeeeppp. The browser creates and initializes everything in document.createElement and returns the element ready-to-go... "factoryImpl" should be called "convenientlySetStuffAfterCreation" because that's why it was written -- it's called with the parameters to a "constructor" which simply calls document.createElement followed by el.factoryImpl(arguments). https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:198: if (!CrSettingsPrefs.isInitialized) { On 2015/09/11 21:49:12, Dan Beam wrote: > can haz promise instead? CrSettingsPrefs.initialized.then()? > > maybe todo? Good idea, added the todo to prefs_types.js. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:226: } On 2015/09/11 21:03:47, stevenjb wrote: > nit: Maybe wrap the above in a function to avoid the nested break/continue which > is a bit awkward, e.g. > var actualPref = getActualPref(key, pref); > if (!expectNotEquals(undefined, actualPref)) > continue; > Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:236: JSON.stringify(actualPref.value)); On 2015/09/11 21:49:12, Dan Beam wrote: > just do this for numbers/integers as well? but but but... it's perf season! https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:244: expectTrue(mockApi.prefs['browser.enable_flash'].value); On 2015/09/11 21:49:13, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:246: expectEquals(1.2, mockApi.prefs['device.overclock'].value); On 2015/09/11 21:49:12, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:252: prefs.set('prefs.browser.enable_flash.value', false); On 2015/09/11 21:49:13, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:254: prefs.set('prefs.device.overclock.value', 2.0); On 2015/09/11 21:49:13, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1310843010/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:278: }).async(function() { On 2015/09/11 21:49:12, Dan Beam wrote: > can't PolymerTest.async() just return a promise and we can do > PolymerTest.async().then()? no, we want to chain this logic more than once ("call this function asynchronously and then resolve asynchronously"). Putting the expectEquals in a promise instead of using .async means it would run before any setTimeouts posted inside of fn(), which in practice means pref is checked before the prefs element gets a chance to react to the mock API. In this case we could do .then(setTimeout(...)) but PolymerTest.async lets us do that *and* makes it prettier IMO.
lgtm, but wait for Dan of course (who most certainly follows this all better than I do) https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:174: return; nit: return undefined? (That would be more clear to me, but maybe in JS this is a common enough pattern?)
lgtm https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/polymer_browser_test_base.js:118: /** @type {Promise} */ nit: @type -> @private https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/cr_settings_browsertest.js:27: commandLineSwitches: [{switchName: 'enable-md-settings'}], nit: /** @override */ ? https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:166: * @return {(chrome.settingsPrivate.PrefObject|undefined} nit: https://xkcd.com/859/ https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:174: return; On 2015/09/14 23:18:19, stevenjb wrote: > nit: return undefined? (That would be more clear to me, but maybe in JS this is > a common enough pattern?) not all that common
Patchset #4 (id:80001) has been deleted
Thanks for the l-g-t-ms. If you don't mind a quick re-review, I've made some
changes between patch set 3 ("already reviewed") and 5 ("new changes") that
should simplify things: move test cases outside the file, generalize the tests,
and remove some hacks.
I'll want to do this for the next CL so I'd prefer to have my updates included
now, but if you'd prefer I just land this change as-is let me know.
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/polymer_browser_test_base.js (right):
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/polymer_browser_test_base.js:118: /** @type {Promise} */
On 2015/09/14 23:31:48, Dan Beam wrote:
> nit: @type -> @private
Done.
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:27:
commandLineSwitches: [{switchName: 'enable-md-settings'}],
On 2015/09/14 23:31:48, Dan Beam wrote:
> nit: /** @override */ ?
Not really... the test checks for the presence of this property, but it isn't
defined on any prototype.
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/prefs_tests.js (right):
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/prefs_tests.js:166: * @return
{(chrome.settingsPrivate.PrefObject|undefined}
On 2015/09/14 23:31:48, Dan Beam wrote:
> nit: https://xkcd.com/859/
Done.
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/prefs_tests.js:174: return;
On 2015/09/14 23:18:19, stevenjb wrote:
> nit: return undefined? (That would be more clear to me, but maybe in JS this
is
> a common enough pattern?)
Done.
https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1310843010/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/cr_settings_browsertest.js:27: commandLineSwitches: [{switchName: 'enable-md-settings'}], On 2015/09/15 01:17:57, michaelpg wrote: > On 2015/09/14 23:31:48, Dan Beam wrote: > > nit: /** @override */ ? > > Not really... the test checks for the presence of this property, but it isn't > defined on any prototype. maybe it should be? whatevs https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_test_cases.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_test_cases.js:6: * @type {Array<Object>} Object -> {key: string, type: chrome.settingsPrivate.PrefType, values: Array<*>} https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_test_cases.js:7: * Test cases containing preference data with three possible values per pref. can you expand on what values means? https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:243: PolymerTest.async(function() { can this be PolymerTest.async().then(...)? https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:280: // Shouldn't respond to non-changes. nit: don't not remove double negatives?
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_test_cases.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_test_cases.js:6: * @type {Array<Object>} On 2015/09/15 01:37:18, Dan Beam wrote: > Object -> {key: string, type: chrome.settingsPrivate.PrefType, values: Array<*>} Done. https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_test_cases.js:7: * Test cases containing preference data with three possible values per pref. On 2015/09/15 01:37:18, Dan Beam wrote: > can you expand on what values means? Done. https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:243: PolymerTest.async(function() { On 2015/09/15 01:37:18, Dan Beam wrote: > can this be PolymerTest.async().then(...)? no: * = prefs_tests.js \> = prefs.js depth = stack * prefs.set(key, value); * > settingsApi.setPref(key, value, setPrefCallback); * > * setTimeout(setPrefCallback(false)); * async() * * setTimeout(resolve); * .then(checkEquals); // queue checkEquals === end of task loop === * setPrefCallback(false); * > settingsApi.getPref(key, getPrefCallback); * * setTimeout(getPrefCallback(pref)); === end of task loop === * * resolve(); // trigger checkEquals * * checkEquals(); // NOT EQUAL! === end of task loop === * getPrefCallback(pref); * > restore pref // Now it's equal :'( === end of task loop === // If only we had checked here :'( The second async ensures that checkEquals isn't called until after getPrefCallback is called. https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:280: // Shouldn't respond to non-changes. On 2015/09/15 01:37:18, Dan Beam wrote: > nit: don't not remove double negatives? Not undone.
still lgtm
https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:243: PolymerTest.async(function() { On 2015/09/15 02:20:53, michaelpg wrote: > On 2015/09/15 01:37:18, Dan Beam wrote: > > can this be PolymerTest.async().then(...)? > > no: > > * = prefs_tests.js > \> = prefs.js > depth = stack > > * prefs.set(key, value); > * > settingsApi.setPref(key, value, setPrefCallback); > * > * setTimeout(setPrefCallback(false)); > * async() > * * setTimeout(resolve); > * .then(checkEquals); // queue checkEquals > === end of task loop === > * setPrefCallback(false); > * > settingsApi.getPref(key, getPrefCallback); > * * setTimeout(getPrefCallback(pref)); > === end of task loop === > * * resolve(); // trigger checkEquals > * * checkEquals(); // NOT EQUAL! > === end of task loop === > * getPrefCallback(pref); > * > restore pref // Now it's equal :'( > === end of task loop === > // If only we had checked here :'( > > > The second async ensures that checkEquals isn't called until after > getPrefCallback is called. this doesn't seem good
https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:243: PolymerTest.async(function() { On 2015/09/16 00:54:19, Dan Beam wrote: > On 2015/09/15 02:20:53, michaelpg wrote: > > On 2015/09/15 01:37:18, Dan Beam wrote: > > > can this be PolymerTest.async().then(...)? > > > > no: > > > > * = prefs_tests.js > > \> = prefs.js > > depth = stack > > > > * prefs.set(key, value); > > * > settingsApi.setPref(key, value, setPrefCallback); > > * > * setTimeout(setPrefCallback(false)); > > * async() > > * * setTimeout(resolve); > > * .then(checkEquals); // queue checkEquals > > === end of task loop === > > * setPrefCallback(false); > > * > settingsApi.getPref(key, getPrefCallback); > > * * setTimeout(getPrefCallback(pref)); > > === end of task loop === > > * * resolve(); // trigger checkEquals > > * * checkEquals(); // NOT EQUAL! > > === end of task loop === > > * getPrefCallback(pref); > > * > restore pref // Now it's equal :'( > > === end of task loop === > > // If only we had checked here :'( > > > > > > The second async ensures that checkEquals isn't called until after > > getPrefCallback is called. > > this doesn't seem good it comes down to: do we want to A) write a complex test to model the real world and test the code as is, or B) adapt the code to pass a simpler test, and not test the code under the more complex real-world conditions specifically, we can cut out the async crap if we disallow prefs.js from assuming extension APIs are async. however, a bug in prefs.js that depends on extension APIs being *synchronous* would no longer be caught by the test. I'll see how much needs to change to achieve (B) -- if it's not much then perhaps it's worth going the simpler route.
https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:243: PolymerTest.async(function() { On 2015/09/16 01:14:09, michaelpg wrote: > On 2015/09/16 00:54:19, Dan Beam wrote: > > On 2015/09/15 02:20:53, michaelpg wrote: > > > On 2015/09/15 01:37:18, Dan Beam wrote: > > > > can this be PolymerTest.async().then(...)? > > > > > > no: > > > > > > * = prefs_tests.js > > > \> = prefs.js > > > depth = stack > > > > > > * prefs.set(key, value); > > > * > settingsApi.setPref(key, value, setPrefCallback); > > > * > * setTimeout(setPrefCallback(false)); > > > * async() > > > * * setTimeout(resolve); > > > * .then(checkEquals); // queue checkEquals > > > === end of task loop === > > > * setPrefCallback(false); > > > * > settingsApi.getPref(key, getPrefCallback); > > > * * setTimeout(getPrefCallback(pref)); > > > === end of task loop === > > > * * resolve(); // trigger checkEquals > > > * * checkEquals(); // NOT EQUAL! > > > === end of task loop === > > > * getPrefCallback(pref); > > > * > restore pref // Now it's equal :'( > > > === end of task loop === > > > // If only we had checked here :'( > > > > > > > > > The second async ensures that checkEquals isn't called until after > > > getPrefCallback is called. > > > > this doesn't seem good > > it comes down to: do we want to > A) write a complex test to model the real world and test the code as is, or you're really just testing a complex mock > B) adapt the code to pass a simpler test, and not test the code under the more > complex real-world conditions it's mocked, it's never gonna be real > > specifically, we can cut out the async crap if we disallow prefs.js from > assuming extension APIs are async. however, a bug in prefs.js that depends on > extension APIs being *synchronous* would no longer be caught by the test. eh, not too worried > > I'll see how much needs to change to achieve (B) -- if it's not much then > perhaps it's worth going the simpler route. ++
Patchset #9 (id:220001) has been deleted
it turns out the changes to prefs.js weren't too big, and the test becomes much simpler. Thanks. (and the diff to prefs.js between 8 and 9 won't be needed at all if "Lazy init prefs" lands first! https://codereview.chromium.org/1346833003/)
lgtm https://codereview.chromium.org/1310843010/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310843010/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:135: return; can't you just use: if (!this.$) return; ? https://codereview.chromium.org/1310843010/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:263: // Only expect setPref calls when the changes are actually different. doesn't this mean you need to expectPrefsSet(2) again?
https://codereview.chromium.org/1310843010/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1310843010/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:135: return; On 2015/09/16 05:53:56, Dan Beam wrote: > can't you just use: > > if (!this.$) > return; > > ? removed w/ rebase on https://codereview.chromium.org/1346833003/ https://codereview.chromium.org/1310843010/diff/240001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/240001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:263: // Only expect setPref calls when the changes are actually different. On 2015/09/16 05:53:56, Dan Beam wrote: > doesn't this mean you need to expectPrefsSet(2) again? It wouldn't hurt, but that isn't what I was testing. I've clarified, and removed the redundant call to mockApi.disallowSetPref.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1310843010/#ps260001 (title: "clarify test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310843010/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310843010/260001
still lgtm but question https://codereview.chromium.org/1310843010/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/260001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:267: // Send the same set of changes again -- nothing should happen. how are you verifying that nothing happened?
https://codereview.chromium.org/1310843010/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1310843010/diff/260001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:267: // Send the same set of changes again -- nothing should happen. On 2015/09/16 21:43:19, Dan Beam wrote: > how are you verifying that nothing happened? it's not proof, but: the <cr-settings-pref> has the same pref values still, and mockApi.setPref was not called (due to the call to disallowSetPref)
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7091d052bfdec82bb28176dd072c10eafd24f7b3 Cr-Commit-Position: refs/heads/master@{#349268} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
