Fix test retries and use TestBench retry rule (#10904)

7 files changed, 19 insertions, 80 deletions

diff --git a/uitest/eclipse-run-selected-test.properties b/uitest/eclipse-run-selected-test.properties
index c73182861d..0315b46a6d 100644
--- a/uitest/eclipse-run-selected-test.properties
+++ b/uitest/eclipse-run-selected-test.properties
@@ -17,7 +17,7 @@ com.vaadin.testbench.screenshot.directory=<enter the full path to the screenshot
 #com.vaadin.testbench.deployment.url=http://<enter your ip here>:8888/
 
 # Simulates @RunLocally with the given value on all test classes without a @RunLocally annotation.
-# Use simple browser name (phantomjs, chrome, firefox, ie8, ie9, ie10, ie11)
+# Use simple browser name (phantomjs, chrome, firefox, ie11)
 
 #com.vaadin.testbench.runLocally=firefox
 
@@ -33,6 +33,11 @@ com.vaadin.testbench.screenshot.directory=<enter the full path to the screenshot
 #com.vaadin.testbench.allowRunLocally=true
 
 #
+# Enable test retries when running tests in an unstable environment.
+#
+#com.vaadin.testbench.Parameters.maxAttempts=3
+
+#
 # Running local instance of XVFB testing cluster. Uncomment all lines below. 
 # Fill in the full path for uitest folder and remember to comment out the other screenshot folder.
 #
diff --git a/uitest/pom.xml b/uitest/pom.xml
index 8ab572879a..6d291ab1a6 100644
--- a/uitest/pom.xml
+++ b/uitest/pom.xml
@@ -304,7 +304,7 @@
 
                         <!-- Optional properties for the test build -->
                         <vaadin.testbench.developer.license>${vaadin.testbench.developer.license}</vaadin.testbench.developer.license>
-                        <com.vaadin.testbench.max.retries>${com.vaadin.testbench.max.retries}</com.vaadin.testbench.max.retries>
+                        <com.vaadin.testbench.Parameters.maxAttempts>${com.vaadin.testbench.Parameters.maxAttempts}</com.vaadin.testbench.Parameters.maxAttempts>
                         <com.vaadin.testbench.Parameters.testsInParallel>${parallel.tests}</com.vaadin.testbench.Parameters.testsInParallel>
                         <com.vaadin.testbench.hub.url>${com.vaadin.testbench.hub.url}</com.vaadin.testbench.hub.url>
                         <browser.factory>${browser.factory}</browser.factory>
diff --git a/uitest/src/test/java/com/vaadin/tests/components/ui/TimeoutRedirectResetsOnActivityTest.java b/uitest/src/test/java/com/vaadin/tests/components/ui/TimeoutRedirectResetsOnActivityTest.java
index 7430f23db1..3a69e4c0df 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/ui/TimeoutRedirectResetsOnActivityTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/ui/TimeoutRedirectResetsOnActivityTest.java
@@ -5,25 +5,16 @@ import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 
 import org.junit.Ignore;
-import org.junit.Rule;
 import org.junit.Test;
 import org.openqa.selenium.WebElement;
 
 import com.vaadin.tests.tb3.MultiBrowserTest;
-import com.vaadin.tests.tb3.RetryOnFail;
 
 public class TimeoutRedirectResetsOnActivityTest extends MultiBrowserTest {
 
-    @Rule
-    // Timing issues are really hard to resolve in a way that this test would be
-    // 100% reliable on all browsers. Hence we shall allow one retry.
-    public RetryOnFail retry = new RetryOnFail();
-
     private int waitBeforeActivity = 4000;
     private int communicationOverhead = 2000;
 
-    private static int i = 0;
-
     @Test
     @Ignore("The test modifies the system messages, which are global and the changes will affect other tests")
     public void verifyRedirectWorks() throws Exception {
diff --git a/uitest/src/test/java/com/vaadin/tests/tb3/AbstractTB3Test.java b/uitest/src/test/java/com/vaadin/tests/tb3/AbstractTB3Test.java
index 411121d5c5..a18573fc04 100644
--- a/uitest/src/test/java/com/vaadin/tests/tb3/AbstractTB3Test.java
+++ b/uitest/src/test/java/com/vaadin/tests/tb3/AbstractTB3Test.java
@@ -87,9 +87,6 @@ public abstract class AbstractTB3Test extends ParallelTest {
     @Rule
     public TestName testName = new TestName();
 
-    @Rule
-    public RetryOnFail retry = new RetryOnFail();
-
     /**
      * Height of the screenshots we want to capture
      */
diff --git a/uitest/src/test/java/com/vaadin/tests/tb3/PrivateTB3Configuration.java b/uitest/src/test/java/com/vaadin/tests/tb3/PrivateTB3Configuration.java
index 4d4f83ea4f..bb8050007a 100644
--- a/uitest/src/test/java/com/vaadin/tests/tb3/PrivateTB3Configuration.java
+++ b/uitest/src/test/java/com/vaadin/tests/tb3/PrivateTB3Configuration.java
@@ -49,6 +49,7 @@ public abstract class PrivateTB3Configuration extends ScreenshotTB3Test {
     private static final String FIREFOX_PATH = "firefox.path";
     private static final String PHANTOMJS_PATH = "phantomjs.binary.path";
     private static final String BROWSERS_EXCLUDE = "browsers.exclude";
+    private static final String MAX_ATTEMPTS = "com.vaadin.testbench.Parameters.maxAttempts";
 
     static {
         if (propertiesFile.exists()) {
@@ -71,6 +72,11 @@ public abstract class PrivateTB3Configuration extends ScreenshotTB3Test {
                 .forEach(property -> System.setProperty(property,
                         properties.getProperty(property)));
 
+        if (properties.containsKey(MAX_ATTEMPTS)) {
+            Parameters.setMaxAttempts(
+                    Integer.parseInt(properties.getProperty(MAX_ATTEMPTS)));
+        }
+
         String dir = System.getProperty(SCREENSHOT_DIRECTORY,
                 properties.getProperty(SCREENSHOT_DIRECTORY));
         if (dir != null && !dir.isEmpty()) {
diff --git a/uitest/src/test/java/com/vaadin/tests/tb3/RetryOnFail.java b/uitest/src/test/java/com/vaadin/tests/tb3/RetryOnFail.java
deleted file mode 100644
index 0f1c72050a..0000000000
--- a/uitest/src/test/java/com/vaadin/tests/tb3/RetryOnFail.java
+++ /dev/null
@@ -1,59 +0,0 @@
-package com.vaadin.tests.tb3;
-
-import java.util.logging.Logger;
-
-import org.junit.rules.TestRule;
-import org.junit.runner.Description;
-import org.junit.runners.model.Statement;
-
-public class RetryOnFail implements TestRule {
-
-    @Override
-    public Statement apply(Statement base, Description description) {
-        return statement(base, description);
-    }
-
-    private Statement statement(final Statement base,
-            final Description description) {
-        return new Statement() {
-            @Override
-            public void evaluate() throws Throwable {
-                Throwable caughtThrowable = null;
-                int retryCount = getRetryCount();
-
-                for (int i = 0; i <= retryCount; i++) {
-                    try {
-                        base.evaluate();
-                        return;
-                    } catch (Throwable t) {
-                        caughtThrowable = t;
-                        System.err
-                                .println(String.format("%s: run %s/%s failed.",
-                                        description.getDisplayName(), i + 1,
-                                        retryCount + 1));
-                        System.err.println(t.getMessage());
-                    }
-                }
-                throw caughtThrowable;
-            }
-
-            private int getRetryCount() {
-                String retryCount = System
-                        .getProperty("com.vaadin.testbench.max.retries");
-
-                if (retryCount != null && !retryCount.trim().isEmpty()) {
-                    try {
-                        return Integer.parseInt(retryCount);
-                    } catch (NumberFormatException e) {
-                        // TODO: See how this was implemented in TestBench
-                        Logger.getLogger(RetryOnFail.class.getName()).warning(
-                                "Could not parse max retry count. Retry count set to 0. Failed value: "
-                                        + retryCount);
-                    }
-                }
-
-                return 0;
-            }
-        };
-    }
-}
diff --git a/uitest/src/test/java/com/vaadin/tests/tb3/ScreenshotTB3Test.java b/uitest/src/test/java/com/vaadin/tests/tb3/ScreenshotTB3Test.java
index fbc103ffd0..9a14fccb4a 100644
--- a/uitest/src/test/java/com/vaadin/tests/tb3/ScreenshotTB3Test.java
+++ b/uitest/src/test/java/com/vaadin/tests/tb3/ScreenshotTB3Test.java
@@ -122,8 +122,7 @@ public abstract class ScreenshotTB3Test extends AbstractTB3Test {
                 match = testBench(driver).compareScreen(referenceFile);
             } else {
                 // Only the element
-                match = customTestBench(driver).compareScreen(element,
-                        referenceFile);
+                match = customTestBench.compareScreen(element, referenceFile);
             }
             if (match) {
                 // There might be failure files because of retries in TestBench.
@@ -178,12 +177,12 @@ public abstract class ScreenshotTB3Test extends AbstractTB3Test {
 
     private CustomTestBenchCommandExecutor customTestBench = null;
 
-    private CustomTestBenchCommandExecutor customTestBench(WebDriver driver) {
-        if (customTestBench == null) {
-            customTestBench = new CustomTestBenchCommandExecutor(driver);
-        }
+    @Override
+    public void setDriver(WebDriver driver) {
+        super.setDriver(driver);
 
-        return customTestBench;
+        // Set custom command executor
+        customTestBench = new CustomTestBenchCommandExecutor(getDriver());
     }
 
     private void enableAutoswitch(File htmlFile)