diff --git a/javatests/google/registry/webdriver/ActualScreenshot.java b/javatests/google/registry/webdriver/ActualScreenshot.java index b83f0befb..e43cbbce4 100644 --- a/javatests/google/registry/webdriver/ActualScreenshot.java +++ b/javatests/google/registry/webdriver/ActualScreenshot.java @@ -31,14 +31,16 @@ public class ActualScreenshot { public static final String IMAGE_FORMAT = "png"; private String imageKey; private BufferedImage bufferedImage; + private int attempt; - private ActualScreenshot(String imageKey, BufferedImage bufferedImage) { + private ActualScreenshot(String imageKey, BufferedImage bufferedImage, int attempt) { this.imageKey = imageKey; this.bufferedImage = bufferedImage; + this.attempt = attempt; } /** Creates an ActualScreenshot from the given image format and byte array. */ - public static ActualScreenshot create(String imageKey, byte[] imageBytes) { + public static ActualScreenshot create(String imageKey, int attempt, byte[] imageBytes) { checkNotNull(imageKey); checkNotNull(imageBytes); byte[] imageBytesClone = Arrays.copyOf(imageBytes, imageBytes.length); @@ -47,7 +49,7 @@ public class ActualScreenshot { try { imageReader.setInput(checkNotNull(ImageIO.createImageInputStream(imageInputStream))); BufferedImage bufferedImage = checkNotNull(imageReader.read(0)); - return new ActualScreenshot(imageKey, bufferedImage); + return new ActualScreenshot(imageKey, bufferedImage, attempt); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -55,7 +57,7 @@ public class ActualScreenshot { /** {@link BufferedImage#getSubimage(int, int, int, int)} */ public ActualScreenshot getSubimage(int x, int y, int w, int h) { - return new ActualScreenshot(imageKey, bufferedImage.getSubimage(x, y, w, h)); + return new ActualScreenshot(imageKey, bufferedImage.getSubimage(x, y, w, h), attempt); } /** {@link BufferedImage#getWidth()} */ @@ -82,9 +84,9 @@ public class ActualScreenshot { } } - /** Returns the imageKey of the screenshot. */ - public String getImageKey() { - return imageKey; + /** Returns the attempt number of the test. */ + public int getAttempt() { + return attempt; } /** Returns the concat of imageKey and imageFormat. */ diff --git a/javatests/google/registry/webdriver/ChromeWebDriverPlusScreenDiffer.java b/javatests/google/registry/webdriver/ChromeWebDriverPlusScreenDiffer.java index 70135b305..9c8b2d102 100644 --- a/javatests/google/registry/webdriver/ChromeWebDriverPlusScreenDiffer.java +++ b/javatests/google/registry/webdriver/ChromeWebDriverPlusScreenDiffer.java @@ -16,7 +16,6 @@ package google.registry.webdriver; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; -import static google.registry.webdriver.RepeatableRunner.currentAttemptIndex; import static java.lang.Math.abs; import static java.util.stream.Collectors.joining; @@ -123,9 +122,9 @@ class ChromeWebDriverPlusScreenDiffer implements WebDriverPlusScreenDiffer { } @Override - public void diffElement(String imageKey, WebElement element) { + public void diffElement(WebElement element, String imageKey, int attempt) { ActualScreenshot elementImage = - takeScreenshot(imageKey) + ActualScreenshot.create(imageKey, attempt, takeScreenshot()) .getSubimage( element.getLocation().getX(), element.getLocation().getY(), @@ -135,8 +134,8 @@ class ChromeWebDriverPlusScreenDiffer implements WebDriverPlusScreenDiffer { } @Override - public void diffPage(String imageKey) { - actualScreenshots.add(takeScreenshot(imageKey)); + public void diffPage(String imageKey, int attempt) { + actualScreenshots.add(ActualScreenshot.create(imageKey, attempt, takeScreenshot())); } @Override @@ -190,10 +189,10 @@ class ChromeWebDriverPlusScreenDiffer implements WebDriverPlusScreenDiffer { } } - private ActualScreenshot takeScreenshot(String imageKey) { + private byte[] takeScreenshot() { checkArgument(webDriver instanceof TakesScreenshot); TakesScreenshot takesScreenshot = (TakesScreenshot) webDriver; - return ActualScreenshot.create(imageKey, takesScreenshot.getScreenshotAs(OutputType.BYTES)); + return takesScreenshot.getScreenshotAs(OutputType.BYTES); } private ComparisonResult compareScreenshots(ActualScreenshot screenshot) { @@ -246,7 +245,7 @@ class ChromeWebDriverPlusScreenDiffer implements WebDriverPlusScreenDiffer { File thisScreenshotDir = Paths.get( screenshotDir, - "attempt_" + currentAttemptIndex, + "attempt_" + result.actualScreenshot().getAttempt(), result.isConsideredSimilar() ? "similar" : "different") .toFile(); thisScreenshotDir.mkdirs(); diff --git a/javatests/google/registry/webdriver/OteSetupConsoleScreenshotTest.java b/javatests/google/registry/webdriver/OteSetupConsoleScreenshotTest.java index 40c205d72..17cd0b09d 100644 --- a/javatests/google/registry/webdriver/OteSetupConsoleScreenshotTest.java +++ b/javatests/google/registry/webdriver/OteSetupConsoleScreenshotTest.java @@ -21,6 +21,7 @@ import com.googlecode.objectify.ObjectifyFilter; import google.registry.model.ofy.OfyFilter; import google.registry.module.frontend.FrontendServlet; import google.registry.server.RegistryTestServer; +import google.registry.webdriver.RepeatableRunner.AttemptNumber; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -40,7 +41,8 @@ public class OteSetupConsoleScreenshotTest { .setEmail("Marla.Singer@google.com") .build(); - @Rule public final WebDriverRule driver = new WebDriverRule(); + private final AttemptNumber attemptNumber = new AttemptNumber(); + @Rule public final WebDriverRule driver = new WebDriverRule(attemptNumber); @Test public void get_owner_fails() throws Throwable { diff --git a/javatests/google/registry/webdriver/RegistrarConsoleScreenshotTest.java b/javatests/google/registry/webdriver/RegistrarConsoleScreenshotTest.java index 8eb46bb93..21291eb47 100644 --- a/javatests/google/registry/webdriver/RegistrarConsoleScreenshotTest.java +++ b/javatests/google/registry/webdriver/RegistrarConsoleScreenshotTest.java @@ -27,6 +27,7 @@ import google.registry.model.registrar.Registrar.State; import google.registry.module.frontend.FrontendServlet; import google.registry.server.RegistryTestServer; import google.registry.testing.CertificateSamples; +import google.registry.webdriver.RepeatableRunner.AttemptNumber; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -50,7 +51,8 @@ public class RegistrarConsoleScreenshotTest { .setEmail("Marla.Singer@google.com") .build(); - @Rule public final WebDriverRule driver = new WebDriverRule(); + private final AttemptNumber attemptNumber = new AttemptNumber(); + @Rule public final WebDriverRule driver = new WebDriverRule(attemptNumber); @Test public void index_owner() throws Throwable { diff --git a/javatests/google/registry/webdriver/RegistrarConsoleWebTest.java b/javatests/google/registry/webdriver/RegistrarConsoleWebTest.java index fa667fb07..218a1108b 100644 --- a/javatests/google/registry/webdriver/RegistrarConsoleWebTest.java +++ b/javatests/google/registry/webdriver/RegistrarConsoleWebTest.java @@ -29,15 +29,15 @@ import google.registry.model.registrar.RegistrarContact; import google.registry.module.frontend.FrontendServlet; import google.registry.server.RegistryTestServer; import google.registry.testing.AppEngineRule; +import google.registry.webdriver.RepeatableRunner.AttemptNumber; import org.junit.Rule; import org.junit.Test; import org.junit.rules.Timeout; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; import org.openqa.selenium.By; /** WebDriver tests for Registrar Console UI. */ -@RunWith(JUnit4.class) +@RunWith(RepeatableRunner.class) public class RegistrarConsoleWebTest { @Rule @@ -59,7 +59,8 @@ public class RegistrarConsoleWebTest { .setEmail("Marla.Singer@google.com") .build(); - @Rule public final WebDriverRule driver = new WebDriverRule(); + private final AttemptNumber attemptNumber = new AttemptNumber(); + @Rule public final WebDriverRule driver = new WebDriverRule(attemptNumber); @Rule public final Timeout deathClock = new Timeout(60000); diff --git a/javatests/google/registry/webdriver/RegistrarCreateConsoleScreenshotTest.java b/javatests/google/registry/webdriver/RegistrarCreateConsoleScreenshotTest.java index 896c24bf7..3cf973a2e 100644 --- a/javatests/google/registry/webdriver/RegistrarCreateConsoleScreenshotTest.java +++ b/javatests/google/registry/webdriver/RegistrarCreateConsoleScreenshotTest.java @@ -21,6 +21,7 @@ import com.googlecode.objectify.ObjectifyFilter; import google.registry.model.ofy.OfyFilter; import google.registry.module.frontend.FrontendServlet; import google.registry.server.RegistryTestServer; +import google.registry.webdriver.RepeatableRunner.AttemptNumber; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -40,7 +41,8 @@ public class RegistrarCreateConsoleScreenshotTest { .setEmail("Marla.Singer@google.com") .build(); - @Rule public final WebDriverRule driver = new WebDriverRule(); + private final AttemptNumber attemptNumber = new AttemptNumber(); + @Rule public final WebDriverRule driver = new WebDriverRule(attemptNumber); @Test public void get_owner_fails() throws Throwable { diff --git a/javatests/google/registry/webdriver/RepeatableRunner.java b/javatests/google/registry/webdriver/RepeatableRunner.java index 5eae90b05..76251ea92 100644 --- a/javatests/google/registry/webdriver/RepeatableRunner.java +++ b/javatests/google/registry/webdriver/RepeatableRunner.java @@ -17,6 +17,10 @@ package google.registry.webdriver; import static com.google.common.base.Preconditions.checkState; import com.google.common.flogger.FluentLogger; +import java.lang.reflect.Field; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.junit.runners.BlockJUnit4ClassRunner; import org.junit.runners.model.FrameworkMethod; import org.junit.runners.model.InitializationError; @@ -25,6 +29,21 @@ import org.junit.runners.model.Statement; /** * A JUnit test runner which can retry each test up to 3 times. * + *

To use this runner, annotate the test class with {@link RepeatableRunner} and define a field + * with type of {@link AttemptNumber}: + * + *

+ * @RunWith(RepeatableRunner.class)
+ * public class RepeatableRunnerTest {
+ *   private AttemptNumber attemptNumber = new AttemptNumber();
+ *
+ *   @Test
+ *   public void test() {
+ *     print(attemptNumber.get());
+ *   }
+ * }
+ * 
+ * *

This runner is for our visual regression to prevent flakes during the run. The test is * repeated multiple times until it passes and it only fails if it failed 3 times. */ @@ -38,51 +57,102 @@ public class RepeatableRunner extends BlockJUnit4ClassRunner { private static final int MAX_ATTEMPTS = Integer.parseInt(System.getProperty("test.screenshot.maxAttempts", "3")); - // TODO(b/127984872): Find an elegant way to pass the index of attempt to the test - public static int currentAttemptIndex; + private final Field attemptNumberField; + private AttemptNumber lastAttemptNumber; /** Constructs a new instance of the default runner */ - public RepeatableRunner(Class klass) throws InitializationError { - super(klass); + public RepeatableRunner(Class clazz) throws InitializationError { + super(clazz); + attemptNumberField = getAttemptNumberField(); + } + + private Field getAttemptNumberField() { + List attemptNumberFields = + Stream.of(getTestClass().getJavaClass().getDeclaredFields()) + .filter(declaredField -> declaredField.getType().equals(AttemptNumber.class)) + .collect(Collectors.toList()); + + if (attemptNumberFields.size() == 0) { + throw new IllegalArgumentException("Missing a field with type of AttemptNumber"); + } else if (attemptNumberFields.size() > 1) { + throw new IllegalArgumentException( + "Cannot have more than 1 field with type of AttemptNumber"); + } + Field attemptNumberField = attemptNumberFields.get(0); + // It should not matter if that field is set to private + attemptNumberField.setAccessible(true); + return attemptNumberField; + } + + @Override + protected Object createTest() throws Exception { + Object testObject = super.createTest(); + // lastAttemptNumber must be null at this moment to indicate that we have + // created the RepeatableStatement for the previous AttemptNumber object + // or this is the first time we run the test. + // If it is not the case, either the tests are run in parallel or the + // behavior of BlockJUnit4ClassRunner is changed. + checkState(lastAttemptNumber == null); + lastAttemptNumber = (AttemptNumber) attemptNumberField.get(testObject); + return testObject; } @Override protected Statement methodBlock(FrameworkMethod method) { Statement statement = super.methodBlock(method); - return new RepeatableStatement(statement, method); + RepeatableStatement repeatableStatement = + new RepeatableStatement(statement, method, lastAttemptNumber); + // Explicitly set lastAttemptNumber to null because it should + // not be reused accidentally by the next test. + lastAttemptNumber = null; + return repeatableStatement; + } + + /** A simple POJO to store the number of the test attempt. */ + public static class AttemptNumber { + private int attemptNumber; + + /** Returns the number of the test attempt. */ + public int get() { + return attemptNumber; + } + + private void set(int attemptNumber) { + this.attemptNumber = attemptNumber; + } } private static class RepeatableStatement extends Statement { private Statement statement; private FrameworkMethod method; + private AttemptNumber attemptNumber; - public RepeatableStatement(Statement statement, FrameworkMethod method) { + public RepeatableStatement( + Statement statement, FrameworkMethod method, AttemptNumber attemptNumber) { this.statement = statement; this.method = method; + this.attemptNumber = attemptNumber; } @Override public void evaluate() throws Throwable { checkState(MAX_ATTEMPTS > 0); int numSuccess = 0, numFailure = 0; - Throwable firstException = null; + Throwable lastException = null; for (int attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { - currentAttemptIndex = attempt; + attemptNumber.set(attempt); try { statement.evaluate(); numSuccess++; - if (RUN_ALL_ATTEMPTS) { - logger.atInfo().log( - "[%s] Attempt %d of %d succeeded!\n", method.getName(), attempt, MAX_ATTEMPTS); - continue; + logger.atInfo().log( + "[%s] Attempt %d of %d succeeded!\n", method.getName(), attempt, MAX_ATTEMPTS); + if (!RUN_ALL_ATTEMPTS) { + return; } - return; } catch (Throwable e) { numFailure++; - if (firstException == null) { - firstException = e; - } + lastException = e; logger.atWarning().log( "[%s] Attempt %d of %d failed!\n", method.getName(), attempt, MAX_ATTEMPTS); } @@ -93,7 +163,7 @@ public class RepeatableRunner extends BlockJUnit4ClassRunner { if (numSuccess == 0) { logger.atSevere().log( "[%s] didn't pass after all %d attempts failed!\n", method.getName(), MAX_ATTEMPTS); - throw firstException; + throw lastException; } } } diff --git a/javatests/google/registry/webdriver/WebDriverPlusScreenDiffer.java b/javatests/google/registry/webdriver/WebDriverPlusScreenDiffer.java index 0459393c3..3064b6de8 100644 --- a/javatests/google/registry/webdriver/WebDriverPlusScreenDiffer.java +++ b/javatests/google/registry/webdriver/WebDriverPlusScreenDiffer.java @@ -34,11 +34,12 @@ interface WebDriverPlusScreenDiffer { * other screenshot matches can be executed in the same function - allowing you to approve / * reject all at once. * + * @param element the element on the page to be compared * @param imageKey a unique name such that by prepending the calling class name and method name in * the format of ClassName_MethodName_ will uniquely identify golden image. - * @param element the element on the page to be compared + * @param attempt the attempt number of the test */ - void diffElement(String imageKey, WebElement element); + void diffElement(WebElement element, String imageKey, int attempt); /** * Checks that the screenshot matches the golden image by pixel comparison. {@link @@ -50,8 +51,9 @@ interface WebDriverPlusScreenDiffer { * * @param imageKey a unique name such that by prepending the calling class name and method name in * the format of ClassName_MethodName_ will uniquely identify golden image. + * @param attempt the attempt number of the test */ - void diffPage(String imageKey); + void diffPage(String imageKey, int attempt); /** Asserts that all diffs up to this point have PASSED. */ void verifyAndQuit(); diff --git a/javatests/google/registry/webdriver/WebDriverRule.java b/javatests/google/registry/webdriver/WebDriverRule.java index 74c003a09..12cf3e7a4 100644 --- a/javatests/google/registry/webdriver/WebDriverRule.java +++ b/javatests/google/registry/webdriver/WebDriverRule.java @@ -15,10 +15,11 @@ package google.registry.webdriver; import static com.google.common.io.Resources.getResource; +import static com.google.common.base.Preconditions.checkNotNull; import static java.util.stream.Collectors.joining; import static org.apache.commons.text.StringEscapeUtils.escapeEcmaScript; -import com.google.common.base.Preconditions; +import google.registry.webdriver.RepeatableRunner.AttemptNumber; import java.net.URL; import java.util.List; import java.util.Map; @@ -68,6 +69,7 @@ public final class WebDriverRule extends ExternalResource private static final String GOLDENS_PATH = getResource(WebDriverRule.class, "goldens/chrome-linux").getFile(); + private AttemptNumber attemptNumber; private WebDriver driver; private WebDriverPlusScreenDiffer webDriverPlusScreenDiffer; @@ -75,6 +77,11 @@ public final class WebDriverRule extends ExternalResource // starts. Will be added a user-given imageKey as a suffix, and of course a '.png' at the end. private String imageNamePrefix = null; + /** Constructs a {@link WebDriverRule} instance. */ + public WebDriverRule(AttemptNumber attemptNumber) { + this.attemptNumber = attemptNumber; + } + @Override public Statement apply(Statement base, Description description) { if (imageNamePrefix == null) { @@ -163,7 +170,7 @@ public final class WebDriverRule extends ExternalResource * @param element the element on the page to be compared */ public void diffElement(String imageKey, WebElement element) { - webDriverPlusScreenDiffer.diffElement(getUniqueName(imageKey), element); + webDriverPlusScreenDiffer.diffElement(element, getUniqueName(imageKey), attemptNumber.get()); } /** @@ -192,7 +199,7 @@ public final class WebDriverRule extends ExternalResource * the format of ClassName_MethodName_ will uniquely identify golden image. */ public void diffPage(String imageKey) { - webDriverPlusScreenDiffer.diffPage(getUniqueName(imageKey)); + webDriverPlusScreenDiffer.diffPage(getUniqueName(imageKey), attemptNumber.get()); } @Override @@ -291,7 +298,7 @@ public final class WebDriverRule extends ExternalResource } private String getUniqueName(String imageKey) { - Preconditions.checkNotNull(imageNamePrefix); + checkNotNull(imageNamePrefix); return imageNamePrefix + "_" + imageKey; } }