Deflake PageLoadingTest by allowing more time for things to load.

Forefox/Desktop and Chrome/Android were especially flaky.
Refactor the tests and add testPageLoadTimeoutCanBeChanged().

Author: sevaseva

java/client/test/org/openqa/selenium/PageLoadingTest.java

@@ -38,15 +38,16 @@
 import static org.openqa.selenium.support.ui.ExpectedConditions.not;
 import static org.openqa.selenium.support.ui.ExpectedConditions.presenceOfElementLocated;
 import static org.openqa.selenium.support.ui.ExpectedConditions.titleIs;
+import static org.openqa.selenium.support.ui.ExpectedConditions.visibilityOfElementLocated;
 import static org.openqa.selenium.testing.Ignore.Driver.CHROME;
 import static org.openqa.selenium.testing.Ignore.Driver.FIREFOX;
 import static org.openqa.selenium.testing.Ignore.Driver.HTMLUNIT;
 import static org.openqa.selenium.testing.Ignore.Driver.IE;
 import static org.openqa.selenium.testing.Ignore.Driver.MARIONETTE;
 import static org.openqa.selenium.testing.Ignore.Driver.PHANTOMJS;
 import static org.openqa.selenium.testing.Ignore.Driver.SAFARI;
-import static org.openqa.selenium.testing.TestUtilities.isChrome;
 import static org.openqa.selenium.testing.TestUtilities.getEffectivePlatform;
+import static org.openqa.selenium.testing.TestUtilities.isChrome;
 import static org.openqa.selenium.testing.TestUtilities.isFirefox;
 import static org.openqa.selenium.testing.TestUtilities.isLocal;
 import static org.openqa.selenium.testing.TestUtilities.isNativeEventsEnabled;
@@ -198,7 +199,7 @@ public void testShouldFollowMetaRedirects() throws Exception {
   public void testShouldBeAbleToGetAFragmentOnTheCurrentPage() {
     driver.get(pages.xhtmlTestPage);
     driver.get(pages.xhtmlTestPage + "#text");
-    driver.findElement(By.id("id1"));
+    wait.until(presenceOfElementLocated(By.id("id1")));
   }
 
   @Ignore(value = {SAFARI, MARIONETTE}, issues = {4062})
@@ -281,7 +282,7 @@ public void testShouldDoNothingIfThereIsNothingToGoBackTo() {
   public void testShouldBeAbleToNavigateBackInTheBrowserHistory() {
     driver.get(pages.formPage);
 
-    driver.findElement(By.id("imageButton")).submit();
+    wait.until(visibilityOfElementLocated(By.id("imageButton"))).submit();
     wait.until(titleIs("We Arrive Here"));
 
     driver.navigate().back();
@@ -293,7 +294,7 @@ public void testShouldBeAbleToNavigateBackInTheBrowserHistory() {
   public void testShouldBeAbleToNavigateBackInTheBrowserHistoryInPresenceOfIframes() {
     driver.get(pages.xhtmlTestPage);
 
-    driver.findElement(By.name("sameWindow")).click();
+    wait.until(visibilityOfElementLocated(By.name("sameWindow"))).click();
     wait.until(titleIs("This page has iframes"));
 
     driver.navigate().back();
@@ -305,7 +306,7 @@ public void testShouldBeAbleToNavigateBackInTheBrowserHistoryInPresenceOfIframes
   public void testShouldBeAbleToNavigateForwardsInTheBrowserHistory() {
     driver.get(pages.formPage);
 
-    driver.findElement(By.id("imageButton")).submit();
+    wait.until(visibilityOfElementLocated(By.id("imageButton"))).submit();
     wait.until(titleIs("We Arrive Here"));
 
     driver.navigate().back();
@@ -370,60 +371,58 @@ public void testShouldNotHangIfDocumentOpenCallIsNeverFollowedByDocumentCloseCal
     driver.get(pages.documentWrite);
 
     // If this command succeeds, then all is well.
-    WebElement body = driver.findElement(By.tagName("body"));
+    WebElement body = wait.until(visibilityOfElementLocated(By.tagName("body")));
     wait.until(elementTextToContain(body, "world"));
   }
 
+  // Note: If this test ever fixed/enabled on Firefox, check if it also needs @NoDriverAfterTest OR
+  // if @NoDriverAfterTest can be removed from some other tests in this class.
+  @Ignore(value = {HTMLUNIT, FIREFOX, MARIONETTE, SAFARI, PHANTOMJS},
+          reason = "Firefox: fails; Marionette: Not implemented; Safari: see issue 687, comment 41;"
+              + "PHANTOMJS, HTMLUNIT: not tested",
+          issues = {687})
+  @NeedsLocalEnvironment
+  @Test
+  public void testPageLoadTimeoutCanBeChanged() {
+    try {
+      testPageLoadTimeoutIsEnforced(2);
+      testPageLoadTimeoutIsEnforced(3);
+    } finally {
+      driver.manage().timeouts().pageLoadTimeout(-1, SECONDS);
+    }
+  }
+
   @Ignore(value = {SAFARI, MARIONETTE},
           reason = "Not implemented; Safari: see issue 687, comment 41",
           issues = {687})
   @NeedsLocalEnvironment
+  @NoDriverAfterTest // Subsequent tests sometimes fail on Firefox.
   @Test
   public void testShouldTimeoutIfAPageTakesTooLongToLoad() {
-    driver.manage().timeouts().pageLoadTimeout(2, SECONDS);
-
-    // Get the sleeping servlet with a pause of 5 seconds
-    String slowPage = appServer.whereIs("sleep?time=5");
-
-    long start = System.currentTimeMillis();
     try {
-      driver.get(slowPage);
-      fail("I should have timed out");
-    } catch (RuntimeException e) {
-      long end = System.currentTimeMillis();
-
-      assertThat(e, is(instanceOf(TimeoutException.class)));
-
-      int duration = (int) (end - start);
-      assertThat(duration, greaterThan(2000));
-      assertThat(duration, lessThan(5000));
-
-      // check that after the exception another page can be loaded
-
-      start = System.currentTimeMillis();
-      driver.get(pages.xhtmlTestPage);
-      wait.until(titleIs("XHTML Test Page"));
-      end = System.currentTimeMillis();
-      duration = (int) (end - start);
-      assertThat(duration, lessThan(2000));
-
+      testPageLoadTimeoutIsEnforced(2);
     } finally {
       driver.manage().timeouts().pageLoadTimeout(-1, SECONDS);
     }
+
+    // Load another page after get() timed out but before test HTTP server served previous page.
+    driver.get(pages.xhtmlTestPage);
+    wait.until(titleIs("XHTML Test Page"));
   }
 
   @Ignore(value = {HTMLUNIT, SAFARI, MARIONETTE},
           reason = "Not implemented; Safari: see issue 687, comment 41",
           issues = {687})
   @NeedsLocalEnvironment
+  @NoDriverAfterTest // Subsequent tests sometimes fail on Firefox.
   @Test
   public void testShouldTimeoutIfAPageTakesTooLongToLoadAfterClick() {
     assumeFalse(isFirefox(driver) && isNativeEventsEnabled(driver));
 
     driver.manage().timeouts().pageLoadTimeout(2, SECONDS);
 
     driver.get(appServer.whereIs("page_with_link_to_slow_loading_page.html"));
-    WebElement link = driver.findElement(By.id("link-to-slow-loading-page"));
+    WebElement link = wait.until(visibilityOfElementLocated(By.id("link-to-slow-loading-page")));
 
     long start = System.currentTimeMillis();
     try {
@@ -437,25 +436,20 @@ public void testShouldTimeoutIfAPageTakesTooLongToLoadAfterClick() {
       int duration = (int) (end - start);
       assertThat(duration, greaterThan(2000));
       assertThat(duration, lessThan(5000));
-
-      // check that after the exception another page can be loaded
-
-      start = System.currentTimeMillis();
-      driver.get(pages.xhtmlTestPage);
-      wait.until(titleIs("XHTML Test Page"));
-      end = System.currentTimeMillis();
-      duration = (int) (end - start);
-      assertThat(duration, lessThan(2000));
-
     } finally {
       driver.manage().timeouts().pageLoadTimeout(-1, SECONDS);
     }
+
+    // Load another page after get() timed out but before test HTTP server served previous page.
+    driver.get(pages.xhtmlTestPage);
+    wait.until(titleIs("XHTML Test Page"));
   }
 
   @Ignore(value = {SAFARI, MARIONETTE},
           reason = "Not implemented; Safari: see issue 687, comment 41",
           issues = {687})
   @NeedsLocalEnvironment
+  @NoDriverAfterTest // Subsequent tests sometimes fail on Firefox.
   @Test
   public void testShouldTimeoutIfAPageTakesTooLongToRefresh() {
     // Get the sleeping servlet with a pause of 5 seconds
@@ -477,55 +471,31 @@ public void testShouldTimeoutIfAPageTakesTooLongToRefresh() {
       int duration = (int) (end - start);
       assertThat(duration, greaterThan(2000));
       assertThat(duration, lessThan(5000));
-
-      // check that after the exception another page can be loaded
-
-      start = System.currentTimeMillis();
-      driver.get(pages.xhtmlTestPage);
-      wait.until(titleIs("XHTML Test Page"));
-      end = System.currentTimeMillis();
-      duration = (int) (end - start);
-      assertThat(duration, lessThan(2000));
-
     } finally {
       driver.manage().timeouts().pageLoadTimeout(-1, SECONDS);
     }
+
+    // Load another page after get() timed out but before test HTTP server served previous page.
+    driver.get(pages.xhtmlTestPage);
+    wait.until(titleIs("XHTML Test Page"));
   }
 
   @Ignore(value = {CHROME, HTMLUNIT, SAFARI, MARIONETTE},
           reason = "Not implemented; Safari: see issue 687, comment 41",
           issues = {687})
   @NeedsLocalEnvironment
+  @NoDriverAfterTest // Subsequent tests sometimes fail on Firefox.
   @Test
   public void testShouldNotStopLoadingPageAfterTimeout() {
-    driver.manage().timeouts().pageLoadTimeout(2, SECONDS);
-
-    // Get the sleeping servlet with a pause of 5 seconds
-    String slowPage = appServer.whereIs("sleep?time=5");
-
-    long start = System.currentTimeMillis();
     try {
-      driver.get(slowPage);
-      fail("I should have timed out");
-    } catch (RuntimeException e) {
-      long end = System.currentTimeMillis();
-
-      assertThat(e, is(instanceOf(TimeoutException.class)));
-
-      int duration = (int) (end - start);
-      assertThat(duration, greaterThan(2000));
-      assertThat(duration, lessThan(5000));
-
-      new WebDriverWait(driver, 30)
-          .ignoring(StaleElementReferenceException.class)
-          .until(elementTextToEqual(By.tagName("body"), "Slept for 5s"));
-      end = System.currentTimeMillis();
-      duration = (int) (end - start);
-      assertThat(duration, greaterThan(5000));
-
+      testPageLoadTimeoutIsEnforced(1);
     } finally {
       driver.manage().timeouts().pageLoadTimeout(-1, SECONDS);
     }
+
+    new WebDriverWait(driver, 30)
+        .ignoring(StaleElementReferenceException.class)
+        .until(elementTextToEqual(By.tagName("body"), "Slept for 11s"));
   }
 
   @After
@@ -536,4 +506,32 @@ public void quitDriver() {
     }
   }
 
+  /**
+   * Sets given pageLoadTimeout to the {@link #driver} and asserts that attempt to navigate to a
+   * page that takes much longer (10 seconds longer) to load results in a TimeoutException.
+   * <p>
+   * Side effects: 1) {@link #driver} is configured to use given pageLoadTimeout,
+   * 2) test HTTP server still didn't serve the page to browser (some browsers may still
+   * be waiting for the page to load despite the fact that driver responded with the timeout).
+   */
+  private void testPageLoadTimeoutIsEnforced(long webDriverPageLoadTimeout) {
+    // Test page will load this many seconds longer than WD pageLoadTimeout.
+    long pageLoadTimeBuffer = 10;
+    driver.manage().timeouts().pageLoadTimeout(webDriverPageLoadTimeout, SECONDS);
+
+    long start = System.currentTimeMillis();
+    try {
+      driver
+          .get(appServer.whereIs("sleep?time=" + (webDriverPageLoadTimeout + pageLoadTimeBuffer)));
+      fail("I should have timed out after " + webDriverPageLoadTimeout + " seconds");
+    } catch (RuntimeException e) {
+      long end = System.currentTimeMillis();
+
+      assertThat(e, is(instanceOf(TimeoutException.class)));
+
+      long duration = end - start;
+      assertThat(duration, greaterThan(webDriverPageLoadTimeout * 1000));
+      assertThat(duration, lessThan((webDriverPageLoadTimeout + pageLoadTimeBuffer) * 1000));
+    }
+  }
 }