Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1457543004:
Add Mocha based tests for Settings pages (Closed)
DescriptionAdd Mocha based tests for Settings pages
This CL introduces a base Prototype and some simple tests for the main
page to ensure that the correct sections are present.
We should add additional tests for each section as we make any changes
to them (and for any new sections of course).
BUG=425627
Committed: https://crrev.com/9dd2c84b4dfbf7aa6478b3086bcf8674314a7e7c
Cr-Commit-Position: refs/heads/master@{#360478}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Feedback #
Total comments: 6
Patch Set 3 : More Feedback #Patch Set 4 : Rebase #
Messages
Total messages: 16 (5 generated)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org, michaelpg@chromium.org
https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:36: }); Nit: How about avoiding "var self = this" by just binding the function to the "this". test('Foo', function() {....}.bind(this)); https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_page_browsertest.js:31: getPage: function(type) { Can we add type annotations for all parameters and return types (@param {string})? https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_page_browsertest.js:33: assertTrue(!!settings); How about assertNotNull(settings); https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_page_browsertest.js:40: var pageType = 'settings-' + type + '-page'; @dbeam Can we start using relatively new v8 feature as follows? var pageType = `settings-${type}-page`;
https://codereview.chromium.org/1457543004/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1457543004/diff/1/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:991: 'test/data/webui/settings/main_page_browsertest.js', alphabetize https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:7: // Polymer BrowserTest fixture. // SettingsPageBrowserTest. (or no comment) https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:12: * @extends {PolymerTest} {SettingsPageBrowserTest} https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:36: }); On 2015/11/18 01:44:54, dpapad wrote: > Nit: How about avoiding "var self = this" by just binding the function to the > "this". > > test('Foo', function() {....}.bind(this)); Not binding was my suggestion. First of all it's annoying to have to do it for every test and suite. Secondly, these functions, when called, already have a non-global context. Calls to suite() or test() result in a mocha Suite or Test object being created; the passed function is then called *on* the created object, so "this" in a test actually means something already. You can use "this" to change properties of the suite or test (and all nested suites/tests): * this.timeout(ms); // changes the timeout to a particular value * this.slow(ms); // changes the "slow" warning threshold, which I don't think we use source: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mocha/... https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:59: this.registerTests(); I don't see much of a reason to register these tests in a separate function -- they're all related to this TEST_F.
Description was changed from ========== Add Mocha based tests for Settings pages This CL introduces a base Prototype and some simple tests for the main page to ensure that the correct sections are present. We should add additional tests for each section as we make any changes to them (and for any new sections of course). BUG=425627 ========== to ========== Add Mocha based tests for Settings pages This CL introduces a base Prototype and some simple tests for the main page to ensure that the correct sections are present. We should add additional tests for each section as we make any changes to them (and for any new sections of course). BUG=425627 ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/1457543004/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1457543004/diff/1/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:991: 'test/data/webui/settings/main_page_browsertest.js', On 2015/11/18 02:00:32, michaelpg wrote: > alphabetize Done. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:7: // Polymer BrowserTest fixture. On 2015/11/18 02:00:32, michaelpg wrote: > // SettingsPageBrowserTest. (or no comment) Done. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:12: * @extends {PolymerTest} On 2015/11/18 02:00:32, michaelpg wrote: > {SettingsPageBrowserTest} Done. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:36: }); On 2015/11/18 02:00:32, michaelpg wrote: > On 2015/11/18 01:44:54, dpapad wrote: > > Nit: How about avoiding "var self = this" by just binding the function to the > > "this". > > > > test('Foo', function() {....}.bind(this)); > > Not binding was my suggestion. First of all it's annoying to have to do it for > every test and suite. Secondly, these functions, when called, already have a > non-global context. > > Calls to suite() or test() result in a mocha Suite or Test object being created; > the passed function is then called *on* the created object, so "this" in a test > actually means something already. > > You can use "this" to change properties of the suite or test (and all nested > suites/tests): > * this.timeout(ms); // changes the timeout to a particular value > * this.slow(ms); // changes the "slow" warning threshold, which I don't think > we use > > source: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mocha/... Added a comment. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:59: this.registerTests(); On 2015/11/18 02:00:32, michaelpg wrote: > I don't see much of a reason to register these tests in a separate function -- > they're all related to this TEST_F. Done. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_page_browsertest.js:31: getPage: function(type) { On 2015/11/18 01:44:54, dpapad wrote: > Can we add type annotations for all parameters and return types (@param > {string})? Done. https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_page_browsertest.js:33: assertTrue(!!settings); On 2015/11/18 01:44:54, dpapad wrote: > How about > assertNotNull(settings); ReferenceError: assertNotNull is not defined I don't think we have this in Chrome.
lgtm
lgtm https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/main_page_browsertest.js:36: }); On 2015/11/18 at 02:00:32, michaelpg wrote: > On 2015/11/18 01:44:54, dpapad wrote: > > Nit: How about avoiding "var self = this" by just binding the function to the > > "this". > > > > test('Foo', function() {....}.bind(this)); > > Not binding was my suggestion. First of all it's annoying to have to do it for every test and suite. Secondly, these functions, when called, already have a non-global context. > > Calls to suite() or test() result in a mocha Suite or Test object being created; the passed function is then called *on* the created object, so "this" in a test actually means something already. > > You can use "this" to change properties of the suite or test (and all nested suites/tests): > * this.timeout(ms); // changes the timeout to a particular value > * this.slow(ms); // changes the "slow" warning threshold, which I don't think we use > > source: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mocha/... Thanks for the information. Did not know that "this" is already bound to a meaningful object by Mocha. https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/main_page_browsertest.js:26: test('load page', function() {}); Remove this test? https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:29: runAccessibilityChecks: false, Nit: @override
https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:63: return s; Indent return(?)
https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/main_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/main_page_browsertest.js:26: test('load page', function() {}); On 2015/11/18 21:09:53, dpapad wrote: > Remove this test? This will fail if the page generates any asserts or console errors. I prefer to have a simple test like this so that it's clear that there is a problem independent e.g. of any page or section loading. I will add a comment. https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_page_browsertest.js (right): https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:29: runAccessibilityChecks: false, On 2015/11/18 21:09:53, dpapad wrote: > Nit: @override Done. https://codereview.chromium.org/1457543004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_page_browsertest.js:63: return s; On 2015/11/18 22:24:55, dschuyler wrote: > Indent return(?) Done.
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, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1457543004/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1457543004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1457543004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9dd2c84b4dfbf7aa6478b3086bcf8674314a7e7c Cr-Commit-Position: refs/heads/master@{#360478} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
