MD Settings: Scroll correctly for navigations that exit search mode.

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

Index: chrome/test/data/webui/settings/basic_page_browsertest.js
diff --git a/chrome/test/data/webui/settings/basic_page_browsertest.js b/chrome/test/data/webui/settings/basic_page_browsertest.js
index 9fde2be0af3303f876535a5e2aadf5c362832897..0a95b699e72482f84ae7b116fd07404282a4890c 100644
--- a/chrome/test/data/webui/settings/basic_page_browsertest.js
+++ b/chrome/test/data/webui/settings/basic_page_browsertest.js
@@ -13,7 +13,12 @@ GEN_INCLUDE(['settings_page_browsertest.js']);
 function SettingsBasicPageBrowserTest() {}
 
 SettingsBasicPageBrowserTest.prototype = {
-  __proto__: SettingsPageBrowserTest.prototype
+  __proto__: SettingsPageBrowserTest.prototype,
+
+  /** @override */
+  extraLibraries: SettingsPageBrowserTest.prototype.extraLibraries.concat([
+    'test_browser_proxy.js',
+  ]),
 };
 
 // Times out on debug builders and may time out on memory bots because
@@ -30,6 +35,47 @@ TEST_F('SettingsBasicPageBrowserTest', 'MAYBE_Load', function() {
   // and test() will be a Mocha 'Suite' or 'Test' instance.
   var self = this;
 
+  /**
+   * This fake SearchManager just hides and re-displays the sections on search.
+   *
+   * @implements {SearchManager}
+   * @extends {settings.TestBrowserProxy}
+   */
+  var TestSearchManager = function() {
+    settings.TestBrowserProxy.call(this, [
+      'search',
+    ]);
+
+    /** @private {?settings.SearchRequest} */
+    this.searchRequest_ = null;
+  }
+
+  TestSearchManager.prototype = {
+    __proto__: settings.TestBrowserProxy.prototype,
+
+    /** @override */
+    search: function(text, page) {
+      if (this.searchRequest_ == null || !this.searchRequest_.isSame(text)) {
+        this.searchRequest_ = new settings.SearchRequest(text);
+        this.searchRequest_.finished = true;
+        this.searchRequest_.updateMatches(false);
+
+        // The search much be resolved asynchronously just like the real
+        // SearchManager. Otherwise, the race condition doesn't manifest.

[dpapad, 2016/12/08 01:33:54]

Perhaps update the comment to mention that the sections also need to be hidden
for the race condition to manifest. Otherwise it looks odd that we are using a
"test" SearchManager, but we are replicating part of its functionality.

[tommycli, 2016/12/08 01:54:46]

Done.

+        setTimeout(function() {
+          var sections =
+              Polymer.dom().querySelectorAll('* /deep/ settings-section');
+          for (var i = 0; i < sections.length; i++)
+            sections[i].hiddenBySearch = !!text;
+
+          this.searchRequest_.resolver.resolve(this.searchRequest_);
+          this.methodCalled('search', text);
+        }.bind(this), 0);
+      }
+      return this.searchRequest_.resolver.promise;
+    },
+  };
+
   // Register mocha tests.
   suite('SettingsPage', function() {
     test('load page', function() {
@@ -81,6 +127,33 @@ TEST_F('SettingsBasicPageBrowserTest', 'MAYBE_Load', function() {
         assertEquals(0, page.scroller.scrollTop);
       });
     });
+
+    test('scroll to section after exiting search', function() {
+      var searchManager = new TestSearchManager();
+      settings.setSearchManagerForTesting(searchManager);
+
+      settings.navigateTo(settings.Route.BASIC,
+                          new URLSearchParams(`search=foobar`),
+                          /* removeSearch */ false);
+      return searchManager.whenCalled('search').then(function() {
+        return new Promise(function(resolve) {
+          settings.navigateTo(settings.Route.ON_STARTUP,
+                              /* dynamicParams */ null,
+                              /* removeSearch */ true);
+
+          var page = self.getPage('basic');
+          assertTrue(!!page);
+
+          // Should (after some time) be scrolled to the On Startup section.
+          var intervalId = window.setInterval(function() {
+            if (page.scroller.scrollTop != 0) {
+              window.clearInterval(intervalId);
+              resolve();
+            }

[dpapad, 2016/12/08 01:33:54]

If this test was to fail, it would only manifest that via a timeout. Can you add
an
else { reject() }, such that it actually rejects if scrolling did not happen? I
think similar issue would happen on the previous test in this file.

Also need to add reject in the function passed to the Promise constructor.

[tommycli, 2016/12/08 01:54:46]

This test requires the setInterval because sometimes after 55ms, the scroll
position has still not updated, and we need to wait for the next interval. The
only failure mode is a timeout. For the same reason, the above test also needs
this.

We could try hooking into the inSearchMode observer or something... blah... this
test should be equally flaky as the one right above it... Maybe both should be
deleted :D

[dpapad, 2016/12/08 02:01:16]

Ah, I had not noticed that it calls setInterval and not setTimeout. Thanks for
pointing this out. Per my original comment in the CL, this is a side-effect of
the actual code being tested, making speculative calls, which trickles down to
the testing code too.

+          }, 55);
+        });
+      });
+    });
   });
 
   // Run all registered tests.