Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769863006:
[MD settings] unit test for incognito activation (Closed)
|
Created:
3 years, 9 months ago by dschuyler Modified:
3 years, 8 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] unit test for incognito activation
This CL adds a unit test for the content settings updating when an
incognito session begins or ends.
(Related to bug 680619)
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2769863006
Cr-Commit-Position: refs/heads/master@{#459495}
Committed: https://chromium.googlesource.com/chromium/src/+/ee60d118c32e9726c3111bd8bbfd59e18ec7a670
Patch Set 1 : set upstream #
Total comments: 4
Patch Set 2 : reset resolver; .list-item #
Total comments: 2
Patch Set 3 : touchup #Patch Set 4 : checking incognito flag #
Total comments: 2
Patch Set 5 : js format #
Total comments: 6
Depends on Patchset: Messages
Total messages: 36 (24 generated)
Description was changed from ========== [MD settings] unit test for incognito activation This CL adds a unit test for the content settings updating when an incognito session begins or ends. (Related to bug 680619) BUG=None ========== to ========== [MD settings] unit test for incognito activation This CL adds a unit test for the content settings updating when an incognito session begins or ends. (Related to bug 680619) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:623: return browserProxy.whenCalled('getExceptionList'); Are you waiting on a new call to getExceptionList() here? If so, there should be a browserProxy.resetResolver('getExceptionList') call first, otherwise whenCalled() simply resolves because of the previous call to getExceptionList(), not because of the new one. Same below. https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:625: assertEquals(2, testElement.sites.length); Asserting that the |sites| Polymer property has a certain number of entries does not provide as much coverage as we could. For example someone can erroneously remove "items=[[sites]]" from https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s... and this test would not break. Can we assert on the expected DOM structure instead, as follows? assertEquals(2, testElement.$$('.list-item').length) This would increase the test coverage of <site-list>. Also it is not clear to me why |sites| is a public property. From a quick search it seems it is only used within site_list.js and therefore could be made private, which would be one more reason not to access it directly from the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:623: return browserProxy.whenCalled('getExceptionList'); On 2017/03/23 21:31:45, dpapad wrote: > Are you waiting on a new call to getExceptionList() here? If so, there should be > a browserProxy.resetResolver('getExceptionList') call first, otherwise > whenCalled() simply resolves because of the previous call to getExceptionList(), > not because of the new one. > > Same below. Done. https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:625: assertEquals(2, testElement.sites.length); On 2017/03/23 21:31:45, dpapad wrote: > Asserting that the |sites| Polymer property has a certain number of entries does > not provide as much coverage as we could. For example someone can erroneously > remove "items=[[sites]]" from > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s... > and this test would not break. > > Can we assert on the expected DOM structure instead, as follows? > assertEquals(2, testElement.$$('.list-item').length) > This would increase the test coverage of <site-list>. We can; I've switched it. I'm concerned that this makes the test a bit more compound which can make it harder to see where a failure is occurring (but not enough to resist the suggestion). > Also it is not clear to me why |sites| is a public property. From a quick search > it seems it is only used within site_list.js and therefore could be made > private, which would be one more reason not to access it directly from the test. That's reasonable and sounds like a good change for a separate CL.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/23 22:13:51, dschuyler wrote: > https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/site_list_tests.js (right): > > https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... > chrome/test/data/webui/settings/site_list_tests.js:623: return > browserProxy.whenCalled('getExceptionList'); > On 2017/03/23 21:31:45, dpapad wrote: > > Are you waiting on a new call to getExceptionList() here? If so, there should > be > > a browserProxy.resetResolver('getExceptionList') call first, otherwise > > whenCalled() simply resolves because of the previous call to > getExceptionList(), > > not because of the new one. > > > > Same below. > > Done. > > https://codereview.chromium.org/2769863006/diff/20001/chrome/test/data/webui/... > chrome/test/data/webui/settings/site_list_tests.js:625: assertEquals(2, > testElement.sites.length); > On 2017/03/23 21:31:45, dpapad wrote: > > Asserting that the |sites| Polymer property has a certain number of entries > does > > not provide as much coverage as we could. For example someone can erroneously > > remove "items=[[sites]]" from > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s... > > and this test would not break. > > > > Can we assert on the expected DOM structure instead, as follows? > > assertEquals(2, testElement.$$('.list-item').length) > > This would increase the test coverage of <site-list>. > > We can; I've switched it. I'm concerned that this makes the test a bit more > compound which can make it harder to see where a failure is occurring (but not > enough to resist the suggestion). > > > Also it is not clear to me why |sites| is a public property. From a quick > search > > it seems it is only used within site_list.js and therefore could be made > > private, which would be one more reason not to access it directly from the > test. > > That's reasonable and sounds like a good change for a separate CL. See CL https://codereview.chromium.org/2768053004/ for the |sites_| change.
https://codereview.chromium.org/2769863006/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:248: pref = pref.concat(pref.filter(function(p) { I am confused about the usage of filter() here. filter() is expected to return either true or false to signify what stays and what is dropped. Here we are returning either an Object or undefined. Should it simply be pref = pref.concat(pref.filter(function(p) { return p.source == 'policy'; }));
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I've also updated the incognito flag on the prefs and added a check the incognito message. https://codereview.chromium.org/2769863006/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:248: pref = pref.concat(pref.filter(function(p) { On 2017/03/23 22:29:10, dpapad wrote: > I am confused about the usage of filter() here. filter() is expected to return > either true or false to signify what stays and what is dropped. Here we are > returning either an Object or undefined. Should it simply be > > pref = pref.concat(pref.filter(function(p) { return p.source == 'policy'; })); filter() returns the object returned by the function. I've changed this to a loop so that the incognito flag is set correctly.
LGTM https://codereview.chromium.org/2769863006/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:249: for (var i=0; i<pref.length; ++i) Nit: Spaces around "<" and "=" missing. From the styleguide "... On both sides of any binary or ternary operator." https://google.github.io/styleguide/jsguide.html#formatting-horizontal-whites...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2769863006/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:249: for (var i=0; i<pref.length; ++i) On 2017/03/24 00:39:41, dpapad wrote: > Nit: Spaces around "<" and "=" missing. > > From the styleguide > > "... On both sides of any binary or ternary operator." > > https://google.github.io/styleguide/jsguide.html#formatting-horizontal-whites... Thanks! I must have missed the JS format. It has now had format --js run.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2769863006/#ps100001 (title: "js format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490380399578850,
"parent_rev": "d03c005d730e9510dd6afa7f8d8199e370db96f5", "commit_rev":
"ee60d118c32e9726c3111bd8bbfd59e18ec7a670"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] unit test for incognito activation This CL adds a unit test for the content settings updating when an incognito session begins or ends. (Related to bug 680619) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] unit test for incognito activation This CL adds a unit test for the content settings updating when an incognito session begins or ends. (Related to bug 680619) BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2769863006 Cr-Commit-Position: refs/heads/master@{#459495} Committed: https://chromium.googlesource.com/chromium/src/+/ee60d118c32e9726c3111bd8bbfd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ee60d118c32e9726c3111bd8bbfd...
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
I don't remember why I looked at this code, but had some comments lying around. publishing just as FYI https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:456: }; no ; after function declarations https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/... (which differs from function expressions: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/f...) https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:457: one less \n https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:71: this.hasIncognito = false; hasIncognito_
Message was sent while issue was closed.
Moved touch-ups to another CL. https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:456: }; On 2017/03/30 04:50:43, Dan Beam (slow) wrote: > no ; after function declarations > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/... > > (which differs from function expressions: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/f...) See https://codereview.chromium.org/2787923002/ https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:457: On 2017/03/30 04:50:43, Dan Beam (slow) wrote: > one less \n See https://codereview.chromium.org/2787923002/ https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2769863006/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:71: this.hasIncognito = false; On 2017/03/30 04:50:43, Dan Beam (slow) wrote: > hasIncognito_ See https://codereview.chromium.org/2787923002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
