Return bool from PurgeCachedWebViewPages() helper.

PurgeCachedWebViewPages() is presumably fail to wait for pageload and
-[ChromeEarlGrey waitForPageToFinishLoading] causes assert. Because
PurgeCachedWebViewPages() is called multiple times inside a single test
it's unclear where the bug actually happens. And the test only fails on
bots, so the logs are the only way to debug the failure.

This CL makes PurgeCachedWebViewPages() return bool, which pushes assert
call up to the test, which should clarify the failure point.

PurgeCachedWebViewPages() now uses
chrome_test_util::WaitForPageToFinishLoading helper function, because
-[ChromeEarlGrey waitForPageToFinishLoading] is not suppose to return
bool result. -[ChromeEarlGrey waitForPageToFinishLoading] now uses
chrome_test_util::WaitForPageToFinishLoading as well to avoid code
duplication.

Bug: 785025
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4e45d30394609825fe9974e0d4077918563295cc
Reviewed-on: https://chromium-review.googlesource.com/780388Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mike Baxley <baxley@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518434}

ios/chrome/browser/web/visible_url_egtest.mm

@@ -4,6 +4,7 @@
 
 #import <EarlGrey/EarlGrey.h>
 
+#include "base/compiler_specific.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
@@ -12,6 +13,7 @@
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "ios/chrome/browser/ui/ui_util.h"
 #import "ios/chrome/test/app/chrome_test_util.h"
+#include "ios/chrome/test/app/navigation_test_util.h"
 #import "ios/chrome/test/earl_grey/chrome_earl_grey.h"
 #import "ios/chrome/test/earl_grey/chrome_earl_grey_ui.h"
 #import "ios/chrome/test/earl_grey/chrome_matchers.h"
@@ -46,12 +48,13 @@ const char kPage3Link[] = "page-3";
 // cached page. Browsers don't have to use fresh version for back forward
 // navigation for HTTP pages and may serve version from the cache even if
 // Cache-Control response header says otherwise.
-void PurgeCachedWebViewPages() {
+bool PurgeCachedWebViewPages() WARN_UNUSED_RESULT;
+bool PurgeCachedWebViewPages() {
   web::WebState* web_state = chrome_test_util::GetCurrentWebState();
   web_state->SetWebUsageEnabled(false);
   web_state->SetWebUsageEnabled(true);
   web_state->GetNavigationManager()->LoadIfNecessary();
-  [ChromeEarlGrey waitForPageToFinishLoading];
+  return chrome_test_util::WaitForPageToFinishLoading();
 }
 
 // Response provider which can be paused. When it is paused it buffers all
@@ -177,7 +180,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testBackForwardNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button in the toolbar and verify that URL2 (committed URL) is
@@ -197,7 +200,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the forward button in the toolbar and verify that URL1 (committed URL)
@@ -221,7 +224,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testHistoryNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Re-enable synchronization here to synchronize EarlGrey LongPress and Tap
@@ -259,7 +262,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testStoppingPendingBackNavigationAndReload {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button, stop pending navigation and reload.
@@ -291,7 +294,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testJSBackForwardNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button on the page and verify that URL2 (committed URL) is
@@ -310,7 +313,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the forward button on the page and verify that URL1 (committed URL)
@@ -334,7 +337,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testJSGoNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the go negative delta button on the page and verify that URL2
@@ -354,7 +357,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap go positive delta button on the page and verify that URL1 (committed
@@ -378,7 +381,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testBackNavigationWithPendingReload {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Start reloading the page.
@@ -415,7 +418,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testBackNavigationWithPendingRendererInitiatedNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Start renderer initiated navigation.
@@ -444,7 +447,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 - (void)testRendererInitiatedNavigationWithPendingBackNavigation {
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button in the toolbar and verify that URL2 (committed URL) is
@@ -476,7 +479,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button twice in the toolbar and verify that URL3 (committed
@@ -533,7 +536,7 @@ class PausableResponseProvider : public HtmlResponseProvider {
 
   // Purge web view caches and pause the server to make sure that tests can
   // verify omnibox state before server starts responding.
-  PurgeCachedWebViewPages();
+  GREYAssert(PurgeCachedWebViewPages(), @"Pages were not purged");
   [self setServerPaused:YES];
 
   // Tap the back button twice on the page and verify that URL3 (committed URL)

ios/chrome/test/app/navigation_test_util.h

@@ -5,6 +5,7 @@
 #ifndef IOS_CHROME_TEST_APP_NAVIGATION_TEST_UTIL_H_
 #define IOS_CHROME_TEST_APP_NAVIGATION_TEST_UTIL_H_
 
+#include "base/compiler_specific.h"
 #include "url/gurl.h"
 
 namespace chrome_test_util {
@@ -16,6 +17,10 @@ void LoadUrl(const GURL& url);
 // Returns true if the current page in the current WebState is loading.
 bool IsLoading();
 
+// Returns true if the current page in the current WebState finishes loading
+// within a timeout.
+bool WaitForPageToFinishLoading() WARN_UNUSED_RESULT;
+
 }  // namespace chrome_test_util
 
 #endif  // IOS_CHROME_TEST_APP_NAVIGATION_TEST_UTIL_H_

ios/chrome/test/app/navigation_test_util.mm

@@ -5,12 +5,15 @@
 #include "ios/chrome/test/app/navigation_test_util.h"
 
 #import "ios/chrome/test/app/chrome_test_util.h"
+#import "ios/testing/wait_util.h"
 #import "ios/web/public/test/navigation_test_util.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
+using testing::WaitUntilConditionOrTimeout;
+
 namespace chrome_test_util {
 
 void LoadUrl(const GURL& url) {
@@ -21,4 +24,10 @@ bool IsLoading() {
   return GetCurrentWebState()->IsLoading();
 }
 
+bool WaitForPageToFinishLoading() {
+  return WaitUntilConditionOrTimeout(testing::kWaitForPageLoadTimeout, ^{
+    return !IsLoading();
+  });
+}
+
 }  // namespace chrome_test_util

ios/chrome/test/earl_grey/chrome_earl_grey.mm

@@ -132,12 +132,7 @@ id ExecuteJavaScript(NSString* javascript,
 }
 
 + (void)waitForPageToFinishLoading {
-  GREYCondition* condition =
-      [GREYCondition conditionWithName:@"Wait for page to complete loading."
-                                 block:^BOOL {
-                                   return !chrome_test_util::IsLoading();
-                                 }];
-  GREYAssert([condition waitWithTimeout:testing::kWaitForPageLoadTimeout],
+  GREYAssert(chrome_test_util::WaitForPageToFinishLoading(),
              @"Page did not complete loading.");
 }