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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /** @fileoverview Suite of tests for the Settings basic page. */
 
 GEN_INCLUDE(['settings_page_browsertest.js']);
 
 /**
  * @constructor
  * @extends {SettingsPageBrowserTest}
  */
 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
 // the Settings page can take several seconds to load in a Release build
 // and several times that in a Debug build. See https://crbug.com/558434.
 GEN('#if defined(MEMORY_SANITIZER) || !defined(NDEBUG)');
 GEN('#define MAYBE_Load DISABLED_Load');
 GEN('#else');
 GEN('#define MAYBE_Load Load');
 GEN('#endif');
 
 TEST_F('SettingsBasicPageBrowserTest', 'MAYBE_Load', function() {
   // Assign |self| to |this| instead of binding since 'this' in suite()
   // 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() {
       // This will fail if there are any asserts or errors in the Settings page.
     });
 
     test('basic pages', function() {
       var page = self.getPage('basic');
       var sections = ['appearance', 'onStartup', 'people', 'search'];
       expectTrue(!!self.getSection(page, 'appearance'));
@@ skipping 31 matching lines @@
 
         return new Promise(function(resolve) {
           listenOnce(window, 'popstate', resolve);
           settings.navigateToPreviousRoute();
         });
       }).then(function() {
         // Should be at the top of the page after going Back from the section.
         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.
   mocha.run();
 });