Simplify Site Settings tests.

Simplify Site Settings tests.

BUG=543635
Committed: https://crrev.com/52c4c3f00ac6bdef29da094d8d7d29e3c9b3f4ec
Cr-Commit-Position: refs/heads/master@{#384911}

[Finnur, 2016-03-29 16:03:01]

finnur@chromium.org changed reviewers:
+ dpapad@chromium.org

[Finnur, 2016-03-29 16:03:01]

Some test simplification, as you suggested...

(CC-ing Tommy)

[dpapad, 2016-03-29 21:46:34]

Thanks for doing this. Looks much better, still a few comments.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:35: * Array of
sites to display in the widget.
Could you add a type annotation for this? As a reader I can't tell what type do
the contents of this array have.
Something like "@type {!Array<string>}" would be helpful to the reader and also
the compiler.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:152: var prefsProxy
= settings.SiteSettingsPrefsBrowserProxyImpl.getInstance();
Nit(optional): You can simply name this browserProxy, since there is no other
proxy used in this file.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:154: for (var i =
0; i < exceptionList.length; ++i) {
Nit: You can use the Array#some() method instead, as follows.

var allowExists = exceptionList.some(function(exception) {
  return exception.setting == 'allow';
});
if (allowExists) 
  return;

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:155: if
(exceptionList[i].setting == 'allow')
Isn't there an enum or constant for the literal 'allow'? We should make one if
not.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:29:
categoryEnabled: {
Nit (optional): You can shorten this now.

categoryEnabled: Boolean

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/cr_settings_browsertest.js:173: preLoad:
function() {
No need to override this at all anymore. Just delete the preLoad function
entirely.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:267:
assertTrue(testElement.isAllowList_());
Lines 267-273 all call private functions. This is a code smell, reduces the
value of this test, since it tests internal implementation instead of externally
observed behavior. It also makes the tested code hard to refactor. Ideally
modifying @private members of a class, should not break any test, unless the
behavior changed.

Can you modify these assertions to instead check externally observed behavior?
Usually this is something reflected in the local DOM of this element.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:286:
'https://bar-allow.com:443', testElement.sites[0].origin);
Can we avoid repeating these string literals? Maybe use something as follows

assertEquals(prefs.geolocation[1].origin, testElement.sites[0].origin);

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:432: return
browserProxy.whenCalled('getExceptionList').then(
Since you are using done(), there is no need to return the Promise.

[Finnur, 2016-03-30 11:22:43]

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:35: * Array of
sites to display in the widget.
On 2016/03/29 21:46:33, dpapad wrote:
> Could you add a type annotation for this? As a reader I can't tell what type
do
> the contents of this array have.
> Something like "@type {!Array<string>}" would be helpful to the reader and
also
> the compiler.

Done. But this got me thinking about the next var. Should they all have @type,
even the dead-simple ones like allSites?

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:152: var prefsProxy
= settings.SiteSettingsPrefsBrowserProxyImpl.getInstance();
On 2016/03/29 21:46:33, dpapad wrote:
> Nit(optional): You can simply name this browserProxy, since there is no other
> proxy used in this file.

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:154: for (var i =
0; i < exceptionList.length; ++i) {
On 2016/03/29 21:46:33, dpapad wrote:
> Nit: You can use the Array#some() method instead, as follows.
> 
> var allowExists = exceptionList.some(function(exception) {
>   return exception.setting == 'allow';
> });
> if (allowExists) 
>   return;

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:155: if
(exceptionList[i].setting == 'allow')
On 2016/03/29 21:46:33, dpapad wrote:
> Isn't there an enum or constant for the literal 'allow'? We should make one if
> not.

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:29:
categoryEnabled: {
On 2016/03/29 21:46:33, dpapad wrote:
> Nit (optional): You can shorten this now.
> 
> categoryEnabled: Boolean

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/cr_settings_browsertest.js:173: preLoad:
function() {
Ah, of course. Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:267:
assertTrue(testElement.isAllowList_());
On 2016/03/29 21:46:33, dpapad wrote:
> Lines 267-273 all call private functions. This is a code smell, reduces the
> value of this test, since it tests internal implementation instead of
externally
> observed behavior. It also makes the tested code hard to refactor. Ideally
> modifying @private members of a class, should not break any test, unless the
> behavior changed.
> 
> Can you modify these assertions to instead check externally observed behavior?
> Usually this is something reflected in the local DOM of this element.

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:286:
'https://bar-allow.com:443', testElement.sites[0].origin);
On 2016/03/29 21:46:33, dpapad wrote:
> Can we avoid repeating these string literals? Maybe use something as follows
> 
> assertEquals(prefs.geolocation[1].origin, testElement.sites[0].origin);

Done.

https://codereview.chromium.org/1838213002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_list_tests.js:432: return
browserProxy.whenCalled('getExceptionList').then(
On 2016/03/29 21:46:33, dpapad wrote:
> Since you are using done(), there is no need to return the Promise.

Done (no pun intended).

[Finnur, 2016-03-31 10:05:12]

Friendly ping... :)

[michaelpg, 2016-03-31 11:13:30]

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:35: * Array of
sites to display in the widget.
On 2016/03/30 11:22:43, Finnur wrote:
> On 2016/03/29 21:46:33, dpapad wrote:
> > Could you add a type annotation for this? As a reader I can't tell what type
> do
> > the contents of this array have.
> > Something like "@type {!Array<string>}" would be helpful to the reader and
> also
> > the compiler.
> 
> Done. But this got me thinking about the next var. Should they all have @type,
> even the dead-simple ones like allSites?

Nope, the Polymer pass in Closure automatically annotates Polymer properties! So
allSites has @type {boolean}. This works for all properties, but for Objects and
Arrays it's helpful to specify the exact type of object/array elements.

(the same argument applies to readability)

[Finnur, 2016-03-31 15:05:25]

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:35: * Array of
sites to display in the widget.
Good to know. Thanks!

[dpapad, 2016-03-31 17:37:08]

LGTM with nits.

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:32: <template
is="dom-repeat" items="{{sites}}">
Could the binding be [[sites]] here?

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:153: var
browserProxy =
Nit: Instead of calling getInstance() three times in this file, how about
calling it once in ready()?

ready: function() {
  this.browserProxy_ = settings.SiteSettingsPrefsBrowserProxyImpl.getInstance();
  ....
}

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:205: if (content ==
textForHiddenAction)
Nit: This can be shortened as follows

actions.forEach(function(action) {
  assertEquals(content == textForHiddenAction, action.hidden);
});

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:93:
browserProxy.setPrefs(prefsLocationDisabled);
This test is identical to the previous one with the exceptions of a few lines.
Could you reduce code repetition? For example how about the following?

function testCategoryEnabled(enabled) {
  browserProxy.setPrefs(enabled ? prefsLocationEnabled : prefsLocationDisabled);

  testElement.category = settings.ContentSettingsTypes.GEOLOCATION;
  return browserProxy.whenCalled('getDefaultValueForContentType').then(
    function(contentType) {                                                     
                         
      assertEquals(settings.ContentSettingsTypes.GEOLOCATION, contentType);
      assertEquals(enabled, testElement.categoryEnabled);
      MockInteractions.tap(testElement.$.toggle);
      return browserProxy.whenCalled('setDefaultValueForContentType').then(
        function(arguments) {
          assertEquals(settings.ContentSettingsTypes.GEOLOCATION, arguments[0]);

          assertEquals(
              enabled ? settings.PermissionValues.BLOCK :
                  settings.PermissionValues.ASK,
              arguments[1]);
          assertNotEquals(enabled, testElement.categoryEnabled);
        }); 
    }); 
}

 test('categoryEnabled correctly represents prefs (enabled)', function() {
   testCategoryEnabled(true);
 }); 

 test('categoryEnabled correctly represents prefs (disabled)', function() {
   testCategoryEnabled(false);
 });

[Finnur, 2016-04-04 13:59:24]

All addressed. I had to modify one of the test helper function a bit; not sure
it warrants another review pass, but I'd be happy to do a followup if you see
something.

Checking in.

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:32: <template
is="dom-repeat" items="{{sites}}">
On 2016/03/31 17:37:07, dpapad wrote:
> Could the binding be [[sites]] here?

Done.

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.js:153: var
browserProxy =
Converted to this.browserProxy_.

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:205: if (content ==
textForHiddenAction)
I discovered this function doesn't work as expected (never asserts); not sure if
it broke at some point due to underlying changes of the code/html or if it never
worked. But I've modified it to fix the issue and make sure it asserts if the
landscape beneath changes.

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/1838213002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:93:
browserProxy.setPrefs(prefsLocationDisabled);
Good catch. It got really scary, though, while I was making this change because
the tests would break in weird and wonderful ways, until I realized these two
functions need to return the promise. Fun times! :)

[Finnur, 2016-04-04 13:59:33]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-04-04 13:59:33]

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/1838213002/#ps40001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-04-04 13:59:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1838213002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1838213002/40001

[commit-bot: I haz the power, 2016-04-04 15:19:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-04 15:20:36]

Description was changed from

==========
Simplify Site Settings tests.

BUG=543635
==========

to

==========
Simplify Site Settings tests.

BUG=543635

Committed: https://crrev.com/52c4c3f00ac6bdef29da094d8d7d29e3c9b3f4ec
Cr-Commit-Position: refs/heads/master@{#384911}
==========

[commit-bot: I haz the power, 2016-04-04 15:20:37]

Patchset 3 (id:??) landed as
https://crrev.com/52c4c3f00ac6bdef29da094d8d7d29e3c9b3f4ec
Cr-Commit-Position: refs/heads/master@{#384911}