Simplify Site Settings tests.

chrome/test/data/webui/settings/site_list_tests.js

Index: chrome/test/data/webui/settings/site_list_tests.js
diff --git a/chrome/test/data/webui/settings/site_list_tests.js b/chrome/test/data/webui/settings/site_list_tests.js
index ffe81d8362b56b83cf37959a719de7e7eca0c6fb..c9459d37739d6c71914c579af142829dc1d8ae0f 100644
--- a/chrome/test/data/webui/settings/site_list_tests.js
+++ b/chrome/test/data/webui/settings/site_list_tests.js
@@ -192,22 +192,6 @@ cr.define('site_list', function() {
       });
 
       /**
-       * Returns a promise that resolves once the site list has been updated.
-       * @param {function()} action is executed after the listener is set up.
-       * @return {!Promise} a Promise fulfilled when the selected item changes.
-       */
-      function runAndResolveWhenSitesChanged(action) {
-        return new Promise(function(resolve, reject) {
-          var handler = function() {
-            testElement.removeEventListener('sites-changed', handler);
-            resolve();
-          };
-          testElement.addEventListener('sites-changed', handler);
-          action();
-        });
-      }
-
-      /**
        * Asserts if a menu action is incorrectly hidden.
        * @param {!HTMLElement} parentElement The parent node to start looking
        *     in.
@@ -272,179 +256,220 @@ cr.define('site_list', function() {
       });
 
       test('Empty list', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          setupLocationCategory(settings.PermissionValues.ALLOW, prefsEmpty);
-        }).then(function() {
-          assertEquals(0, testElement.sites.length);
-
-          assertTrue(testElement.isAllowList_());
-          assertFalse(testElement.showSiteList_(testElement.sites, true));
-          assertFalse(testElement.showSiteList_(testElement.sites, false));
-          assertEquals('Allow - 0', testElement.computeSiteListHeader_(
-              testElement.sites, true));
-          assertEquals('Exceptions - 0', testElement.computeSiteListHeader_(
-              testElement.sites, false));
-        }.bind(this));
+        setupLocationCategory(settings.PermissionValues.ALLOW, prefsEmpty);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertEquals(0, testElement.sites.length);
+
+            assertTrue(testElement.isAllowList_());

[dpapad, 2016/03/29 21:46:33]

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.

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

Done.

+            assertFalse(testElement.showSiteList_(testElement.sites, true));
+            assertFalse(testElement.showSiteList_(testElement.sites, false));
+            assertEquals('Allow - 0', testElement.computeSiteListHeader_(
+                testElement.sites, true));
+            assertEquals('Exceptions - 0', testElement.computeSiteListHeader_(
+                testElement.sites, false));
+          });
       });
 
       test('initial ALLOW state is correct', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
-        }).then(function() {
-          assertEquals(2, testElement.sites.length);
-          assertEquals('https://bar-allow.com:443', testElement.sites[0].origin);
-          assertTrue(testElement.isAllowList_());
-          assertMenuActionHidden(testElement, 'Allow');
-          // Site list should show, no matter what category default is set to.
-          assertTrue(testElement.showSiteList_(testElement.sites, true));
-          assertTrue(testElement.showSiteList_(testElement.sites, false));
-          assertEquals('Exceptions - 2', testElement.computeSiteListHeader_(
-              testElement.sites, false));
-          assertEquals('Allow - 2', testElement.computeSiteListHeader_(
-              testElement.sites, true));
-        }.bind(this));
+        setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertEquals(2, testElement.sites.length);
+            assertEquals(
+                'https://bar-allow.com:443', testElement.sites[0].origin);

[dpapad, 2016/03/29 21:46:33]

Can we avoid repeating these string literals? Maybe use something as follows

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

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

Done.

+            assertTrue(testElement.isAllowList_());
+            assertMenuActionHidden(testElement, 'Allow');
+            // Site list should show, no matter what category default is set to.
+            assertTrue(testElement.showSiteList_(testElement.sites, true));
+            assertTrue(testElement.showSiteList_(testElement.sites, false));
+            assertEquals('Exceptions - 2', testElement.computeSiteListHeader_(
+                testElement.sites, false));
+            assertEquals('Allow - 2', testElement.computeSiteListHeader_(
+                testElement.sites, true));
+          });
       });
 
       test('initial BLOCK state is correct', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          setupLocationCategory(settings.PermissionValues.BLOCK, prefs);
-        }).then(function() {
-          assertEquals(2, testElement.sites.length);
-          assertEquals('https://bar-block.com:443', testElement.sites[0].origin);
-
-          assertFalse(testElement.isAllowList_());
-          assertMenuActionHidden(testElement, 'Block');
-          // Site list should only show when category default is enabled.
-          assertFalse(testElement.showSiteList_(testElement.sites, false));
-          assertTrue(testElement.showSiteList_(testElement.sites, true));
-          assertEquals('Block - 2', testElement.computeSiteListHeader_(
-              testElement.sites, true));
-        }.bind(this));
+        setupLocationCategory(settings.PermissionValues.BLOCK, prefs);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertEquals(2, testElement.sites.length);
+            assertEquals('https://bar-block.com:443', testElement.sites[0].origin);
+
+            assertFalse(testElement.isAllowList_());
+            assertMenuActionHidden(testElement, 'Block');
+            // Site list should only show when category default is enabled.
+            assertFalse(testElement.showSiteList_(testElement.sites, false));
+            assertTrue(testElement.showSiteList_(testElement.sites, true));
+            assertEquals('Block - 2', testElement.computeSiteListHeader_(
+                testElement.sites, true));
+        });
       });
 
       test('list items shown and clickable when data is present', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
-        }).then(function() {
-          // Required for firstItem to be found below.
-          Polymer.dom.flush();
-
-          // Validate that the sites gets populated from pre-canned prefs.
-          assertEquals(2, testElement.sites.length);
-          assertEquals('https://bar-allow.com:443', testElement.sites[0].origin);
-          assertEquals(undefined, testElement.selectedOrigin);
-
-          // Validate that the sites are shown in UI and can be selected.
-          var firstItem = testElement.$.listContainer.items[0];
-          var clickable = firstItem.querySelector('.flex paper-item-body');
-          assertNotEquals(undefined, clickable);
-          MockInteractions.tap(clickable);
-          assertEquals(
-              'https://bar-allow.com:443', testElement.selectedSite.origin);
-        }.bind(this));
+        setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            // Required for firstItem to be found below.
+            Polymer.dom.flush();
+
+            // Validate that the sites gets populated from pre-canned prefs.
+            assertEquals(2, testElement.sites.length);
+            assertEquals('https://bar-allow.com:443', testElement.sites[0].origin);
+            assertEquals(undefined, testElement.selectedOrigin);
+
+            // Validate that the sites are shown in UI and can be selected.
+            var firstItem = testElement.$.listContainer.items[0];
+            var clickable = firstItem.querySelector('.flex paper-item-body');
+            assertNotEquals(undefined, clickable);
+            MockInteractions.tap(clickable);
+            assertEquals(
+                'https://bar-allow.com:443', testElement.selectedSite.origin);
+        });
       });
 
       test('Block list open when Allow list is empty', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: One item in Block list, nothing in Allow list.
-          setupLocationCategory(settings.PermissionValues.BLOCK,
-                                prefsOneDisabled);
-        }).then(function() {
-          assertFalse(testElement.$.category.hidden);
-          assertTrue(testElement.$.category.opened);
-          assertNotEquals(0, testElement.$.listContainer.offsetHeight);
-        }.bind(this));
+        // Prefs: One item in Block list, nothing in Allow list.
+        setupLocationCategory(settings.PermissionValues.BLOCK,
+                              prefsOneDisabled);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertFalse(testElement.$.category.hidden);
+            assertTrue(testElement.$.category.opened);
+            assertNotEquals(0, testElement.$.listContainer.offsetHeight);
+        });
       });
 
       test('Block list closed when Allow list is not empty', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: Items in both Block and Allow list.
-          setupLocationCategory(settings.PermissionValues.BLOCK, prefs);
-        }).then(function() {
-          assertFalse(testElement.$.category.hidden);
-          assertFalse(testElement.$.category.opened);
-          assertEquals(0, testElement.$.listContainer.offsetHeight);
-        }.bind(this));
+        // Prefs: Items in both Block and Allow list.
+        setupLocationCategory(settings.PermissionValues.BLOCK, prefs);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertFalse(testElement.$.category.hidden);
+            assertFalse(testElement.$.category.opened);
+            assertEquals(0, testElement.$.listContainer.offsetHeight);
+        });
       });
 
       test('Allow list is always open (Block list empty)', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: One item in Allow list, nothing in Block list.
-          setupLocationCategory(
-              settings.PermissionValues.ALLOW, prefsOneEnabled);
-        }).then(function() {
-          assertFalse(testElement.$.category.hidden);
-          assertTrue(testElement.$.category.opened);
-          assertNotEquals(0, testElement.$.listContainer.offsetHeight);
-        }.bind(this));
+        // Prefs: One item in Allow list, nothing in Block list.
+        setupLocationCategory(
+            settings.PermissionValues.ALLOW, prefsOneEnabled);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertFalse(testElement.$.category.hidden);
+            assertTrue(testElement.$.category.opened);
+            assertNotEquals(0, testElement.$.listContainer.offsetHeight);
+        });
       });
 
       test('Allow list is always open (Block list non-empty)', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: Items in both Block and Allow list.
-          setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
-        }).then(function() {
-          assertFalse(testElement.$.category.hidden);
-          assertTrue(testElement.$.category.opened);
-          assertNotEquals(0, testElement.$.listContainer.offsetHeight);
-        }.bind(this));
+        // Prefs: Items in both Block and Allow list.
+        setupLocationCategory(settings.PermissionValues.ALLOW, prefs);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertFalse(testElement.$.category.hidden);
+            assertTrue(testElement.$.category.opened);
+            assertNotEquals(0, testElement.$.listContainer.offsetHeight);
+        });
       });
 
       test('Block list hidden when empty', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: One item in Allow list, nothing in Block list.
-          setupLocationCategory(
-              settings.PermissionValues.BLOCK, prefsOneEnabled);
-        }).then(function() {
-          assertTrue(testElement.$.category.hidden);
-        }.bind(this));
+        // Prefs: One item in Allow list, nothing in Block list.
+        setupLocationCategory(
+            settings.PermissionValues.BLOCK, prefsOneEnabled);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertTrue(testElement.$.category.hidden);
+        });
       });
 
       test('Allow list hidden when empty', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: One item in Block list, nothing in Allow list.
-          setupLocationCategory(settings.PermissionValues.ALLOW,
-                                prefsOneDisabled);
-        }).then(function() {
-          assertTrue(testElement.$.category.hidden);
-        }.bind(this));
+        // Prefs: One item in Block list, nothing in Allow list.
+        setupLocationCategory(settings.PermissionValues.ALLOW,
+                              prefsOneDisabled);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            assertEquals(
+                settings.ContentSettingsTypes.GEOLOCATION, contentType);
+
+            assertTrue(testElement.$.category.hidden);
+        });
       });
 
-      test('All sites category', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: Multiple and overlapping sites.
-          setupAllSitesCategory(prefsVarious);
-        }).then(function() {
-          // Required for firstItem to be found below.
-          Polymer.dom.flush();
-
-          assertFalse(testElement.$.category.hidden);
-          // Validate that the sites gets populated from pre-canned prefs. If
-          // this fails with 5 instead of the expected 3, then the de-duping of
-          // sites is not working for site_list.
-          assertEquals(3, testElement.sites.length);
-          assertEquals('https://bar.com', testElement.sites[0].origin);
-          assertEquals('https://foo.com', testElement.sites[1].origin);
-          assertEquals('https://google.com', testElement.sites[2].origin);
-          assertEquals(undefined, testElement.selectedOrigin);
-
-          // Validate that the sites are shown in UI and can be selected.
-          var firstItem = testElement.$.listContainer.items[1];
-          var clickable = firstItem.querySelector('.flex paper-item-body');
-          assertNotEquals(undefined, clickable);
-          MockInteractions.tap(clickable);
-          assertEquals('https://foo.com', testElement.selectedSite.origin);
-        }.bind(this));
+      test('All sites category', function(done) {
+        // Prefs: Multiple and overlapping sites.
+        setupAllSitesCategory(prefsVarious);
+
+        return browserProxy.whenCalled('getExceptionList').then(

[dpapad, 2016/03/29 21:46:33]

Since you are using done(), there is no need to return the Promise.

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

Done (no pun intended).

+          function(contentType) {
+            testElement.async(function() {
+              // All Sites calls getExceptionList for all categories, starting
+              // with Cookies.
+              assertEquals(settings.ContentSettingsTypes.COOKIES, contentType);
+
+              // Required for firstItem to be found below.
+              Polymer.dom.flush();
+
+              assertTrue(testElement.$.category.opened);
+              assertFalse(testElement.$.category.hidden);
+              // Validate that the sites gets populated from pre-canned prefs.
+              // If this fails with 5 instead of the expected 3, then the
+              // de-duping of sites is not working for site_list.
+              assertEquals(3, testElement.sites.length);
+              assertEquals('https://bar.com', testElement.sites[0].origin);
+              assertEquals('https://foo.com', testElement.sites[1].origin);
+              assertEquals('https://google.com', testElement.sites[2].origin);
+              assertEquals(undefined, testElement.selectedOrigin);
+
+              // Validate that the sites are shown in UI and can be selected.
+              var firstItem = testElement.$.listContainer.items[1];
+              var clickable = firstItem.querySelector('.flex paper-item-body');
+              assertNotEquals(undefined, clickable);
+              MockInteractions.tap(clickable);
+              assertEquals('https://foo.com', testElement.selectedSite.origin);
+
+              done();
+          });
+        });
       });
 
       test('Mixed schemes (present and absent)', function() {
-        return runAndResolveWhenSitesChanged(function() {
-          // Prefs: One item with scheme and one without.
-          setupLocationCategory(settings.PermissionValues.ALLOW,
-                                prefsMixedSchemes);
-        }).then(function() {
-          // No further checks needed. If this fails, it will hang the test.
-        }.bind(this));
+        // Prefs: One item with scheme and one without.
+        setupLocationCategory(settings.PermissionValues.ALLOW,
+                              prefsMixedSchemes);
+        return browserProxy.whenCalled('getExceptionList').then(
+          function(contentType) {
+            // No further checks needed. If this fails, it will hang the test.
+        });
       });
     });
   }