[Extensions] SearchApiTest tabs.onUpdated() instead of tabs.query().

The purpose of this CL is flake mitigation.

This paragraph relates to running each apitest 3000x apiece.
These are crashes even when the test is only succeed().
The flake rate is around 1.3% with unrelated crashes.
Relevant tests seem to have a similar heuristic.
Dave mentioned it might be good to file a bug for it.

Flakiness may have been caused by instantly checking tab url.
This CL leverages a TabListener object to await tab updates.
Once a tab has been updated, the url can be chekced more reliably.
A queue of depth one was implemented here not expecting more than that.

For the normal test, didPerformQuery cannot be run first/parallel
due to race condition errors where two succeed occur simultaneously
when the test framework was only expecting one, causing error.

These tests pass locally, as did the previous version. The previous
version was run with `--gtest_repeat=20`, in addition to this version.

Bug: 1119846

Change-Id: Idcb3a3aa82e4ce9df7a36da9a69071634c1a6ff1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368147
Commit-Queue: Solomon Kinard <solomonkinard@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802748}

chrome/browser/extensions/api/search/search_api_apitest.cc

@@ -13,22 +13,19 @@ namespace {
 using SearchApiTest = ExtensionApiTest;
 
 // Test various scenarios, such as the use of input different parameters.
-// Flaky. See crbug.com/1119846.
-IN_PROC_BROWSER_TEST_F(SearchApiTest, DISABLED_Normal) {
+IN_PROC_BROWSER_TEST_F(SearchApiTest, Normal) {
   ASSERT_TRUE(RunExtensionTest("search/query/normal")) << message_;
 }
 
 // Test incognito browser in extension default spanning mode.
-// Flaky. See crbug.com/1119846.
-IN_PROC_BROWSER_TEST_F(SearchApiTest, DISABLED_Incognito) {
+IN_PROC_BROWSER_TEST_F(SearchApiTest, Incognito) {
   ResultCatcher catcher;
   CreateIncognitoBrowser(browser()->profile());
   ASSERT_TRUE(RunExtensionTestIncognito("search/query/incognito")) << message_;
 }
 
 // Test incognito browser in extension split mode.
-// Flaky. See crbug.com/1119846.
-IN_PROC_BROWSER_TEST_F(SearchApiTest, DISABLED_IncognitoSplit) {
+IN_PROC_BROWSER_TEST_F(SearchApiTest, IncognitoSplit) {
   ResultCatcher catcher;
   catcher.RestrictToBrowserContext(
       browser()->profile()->GetPrimaryOTRProfile());

chrome/test/data/extensions/api_test/search/query/incognito/background.js

@@ -9,24 +9,6 @@ var succeed = chrome.test.succeed;
 
 const SEARCH_WORDS = 'search words';
 
-var testHelper = (tabs, queryInfo) => {
-  assertEq(1, tabs.length);
-  const tab = tabs[0];
-  // The browser test should have spun up an incognito browser, which
-  // should be active.
-  assertTrue(tab.incognito);
-  chrome.search.query(queryInfo, () => {
-    assertNoLastError();
-    chrome.tabs.query({windowId: tab.windowId}, (tabs) => {
-      const tab = tabs[0];
-      const url = new URL(tab.pendingUrl || tab.url);
-      // The default search is google.
-      assertEq('www.google.com', url.hostname);
-      succeed();
-    });
-  });
-};
-
 chrome.test.runTests([
 
   // Verify search results shown in specified incognito tab.
@@ -43,4 +25,33 @@ chrome.test.runTests([
       testHelper(tabs, {search: SEARCH_WORDS});
     });
   },
-]);
+]);
\ No newline at end of file
+
+var testHelper = (tabs, queryInfo) => {
+  assertEq(1, tabs.length);
+  const tab = tabs[0];
+  // The browser test should have spun up an incognito browser, which
+  // should be active.
+  assertTrue(tab.incognito);
+  addTabListener(tab.id);
+  chrome.search.query(queryInfo, () => {
+    assertNoLastError();
+    chrome.tabs.query({windowId: tab.windowId}, (tabs) => {});
+  });
+};
+
+let addTabListener = (tabIdExpected) => {
+  chrome.tabs.onUpdated.addListener(function listener(tabId, changeInfo, tab) {
+    if (tabId != tabIdExpected || changeInfo.status !== 'complete') {
+      return;  // Not our tab.
+    }
+    // Note: make sure to stop listening to future events, so that this
+    // doesn't affect future tests.
+    chrome.tabs.onUpdated.removeListener(listener);
+    // The tab finished loading. It should be on google (the default
+    // search engine).
+    const hostname = new URL(tab.url).hostname;
+    assertEq('www.google.com', hostname);
+    succeed();
+  });
+};

chrome/test/data/extensions/api_test/search/query/incognito_split/background.js

@@ -29,22 +29,33 @@ if (chrome.extension.inIncognitoContext) {
       });
     },
   ]);
+}
 
-  var testHelper = (tabs, queryInfo) => {
-    assertEq(1, tabs.length);
-    const tab = tabs[0];
-    // The browser test should have spun up an incognito browser, which
-    // should be active.
-    assertTrue(tab.incognito);
-    chrome.search.query(queryInfo, () => {
-      assertNoLastError();
-      chrome.tabs.query({windowId: tab.windowId}, (tabs) => {
-        const tab = tabs[0];
-        const url = new URL(tab.pendingUrl || tab.url);
-        // The default search is google.
-        assertEq('www.google.com', url.hostname);
-        succeed();
-      });
-    });
-  };
-}
+var testHelper = (tabs, queryInfo) => {
+  assertEq(1, tabs.length);
+  const tab = tabs[0];
+  // The browser test should have spun up an incognito browser, which
+  // should be active.
+  assertTrue(tab.incognito);
+  addTabListener(tab.id);
+  chrome.search.query(queryInfo, () => {
+    assertNoLastError();
+    chrome.tabs.query({windowId: tab.windowId}, (tabs) => {});
+  });
+};
+
+let addTabListener = (tabIdExpected) => {
+  chrome.tabs.onUpdated.addListener(function listener(tabId, changeInfo, tab) {
+    if (tabId != tabIdExpected || changeInfo.status !== 'complete') {
+      return;  // Not our tab.
+    }
\ No newline at end of file
+    // Note: make sure to stop listening to future events, so that this
+    // doesn't affect future tests.
+    chrome.tabs.onUpdated.removeListener(listener);
+    // The tab finished loading. It should be on google (the default
+    // search engine).
+    const hostname = new URL(tab.url).hostname;
+    assertEq('www.google.com', hostname);
+    succeed();
+  });
+};

chrome/test/data/extensions/api_test/search/query/normal/background.js

@@ -21,22 +21,18 @@ chrome.test.runTests([
     });
   },
 
-  // Display results in current tab if no disposition is provided.
   function QueryPopulatedDispositionEmpty() {
-    chrome.tabs.query({active: true}, (tabs) => {
-      chrome.search.query({search: SEARCH_WORDS}, () => {
-        didPerformQuery(tabs[0].id);
-      });
+    chrome.tabs.create({}, (tab) => {
+      addTabListener(tab.id);
+      chrome.search.query({search: SEARCH_WORDS}, () => {});
     });
   },
 
   // Display results in current tab if said disposition is provided.
   function QueryPopulatedDispositionCurrentTab() {
-    chrome.tabs.query({active: true}, (tabs) => {
-      chrome.search.query(
-          {search: SEARCH_WORDS, disposition: 'CURRENT_TAB'}, () => {
-            didPerformQuery(tabs[0].id);
-          });
+    chrome.tabs.create({}, (tab) => {
+      addTabListener(tab.id);
+      chrome.search.query({search: SEARCH_WORDS, disposition: 'CURRENT_TAB'});
     });
   },
 
@@ -44,13 +40,13 @@ chrome.test.runTests([
   function QueryPopulatedDispositionNewTab() {
     chrome.tabs.query({}, (initialTabs) => {
       let initialTabIds = initialTabs.map(tab => tab.id);
+      addAnyTabListener();
       chrome.search.query(
           {search: SEARCH_WORDS, disposition: 'NEW_TAB'}, () => {
             chrome.tabs.query({active: true, currentWindow: true}, (tabs) => {
               assertEq(1, tabs.length);
               // A new tab should have been created.
               assertFalse(initialTabIds.includes(tabs[0].id));
-              didPerformQuery(tabs[0].id);
             });
           });
     });
@@ -60,6 +56,7 @@ chrome.test.runTests([
   function QueryPopulatedDispositionNewWindow() {
     chrome.windows.getAll({}, (initialWindows) => {
       let initialWindowIds = initialWindows.map(window => window.id);
+      addAnyTabListener();
       chrome.search.query(
           {search: SEARCH_WORDS, disposition: 'NEW_WINDOW'}, () => {
             chrome.windows.getAll({}, (windows) => {
@@ -67,9 +64,6 @@ chrome.test.runTests([
               let window =
                   windows.find(window => !initialWindowIds.includes(window.id));
               assertTrue(!!window);
-              chrome.tabs.query({windowId: window.id}, (tabs) => {
-                didPerformQuery(tabs[0].id);
-              });
             });
           });
     });
@@ -78,9 +72,8 @@ chrome.test.runTests([
   // Display results in specified tab if said tabId is provided.
   function QueryPopulatedTabIDValid() {
     chrome.tabs.create({}, (tab) => {
-      chrome.search.query({search: SEARCH_WORDS, tabId: tab.id}, () => {
-        didPerformQuery(tab.id);
-      });
+      addTabListener(tab.id);
+      chrome.search.query({search: SEARCH_WORDS, tabId: tab.id});
     });
   },
 
@@ -105,13 +98,23 @@ chrome.test.runTests([
   },
 ]);
 
-var didPerformQuery = function(tabId) {
-  assertNoLastError();
-  chrome.tabs.get(tabId, (tab) => {
-    const tabUrl = tab.pendingUrl || tab.url;
-    const url = new URL(tabUrl);
-    // The default search is google.
-    assertEq('www.google.com', url.hostname);
+let addTabListener = (tabIdExpected) => {
+  chrome.tabs.onUpdated.addListener(function listener(tabId, changeInfo, tab) {
+    if ((tabIdExpected != -1 && tabId != tabIdExpected) ||
+        changeInfo.status !== 'complete') {
+      return;  // Not our tab.
+    }
+    // Note: make sure to stop listening to future events, so that this
+    // doesn't affect future tests.
+    chrome.tabs.onUpdated.removeListener(listener);
+    // The tab finished loading. It should be on google (the default
+    // search engine).
+    const hostname = new URL(tab.url).hostname;
+    assertEq('www.google.com', hostname);
     succeed();
   });
-}
+};
\ No newline at end of file
+
+let addAnyTabListener = () => {
+  addTabListener(-1);
+};