From e3822992121f40c41e6239b67a1adf23f9dc3a63 Mon Sep 17 00:00:00 2001
From: shicong <shicong@google.com>
Date: Mon, 6 May 2019 15:03:42 -0700
Subject: [PATCH] Resolve carry-over state issue in TestServerRule

The TestServerRule object is shared between retries in the same test,
so the testServer object constructed in TestServerRule's constructor
will be shared as well. This should be the reason why the test retry
carries over some state. (The log in the test proves that the
testServer object is shared because it listens to the same port in
all retries, which should not happen if its constructor is invoked
every time. You can find multiple "TestServerRule is listening on:
[]in this test
[]

So, this CL delayed the construction to rule.before() method which is
invoked before every retry.(You can see each retry has a test server
listening to different port and the error is "It differed by <16> pixels."
for all attempts instead of not clickable button []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=246904446
---
 .../registry/webdriver/RepeatableRunner.java  |  2 +-
 .../registry/webdriver/TestServerRule.java    | 35 +++++++++++++------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/javatests/google/registry/webdriver/RepeatableRunner.java b/javatests/google/registry/webdriver/RepeatableRunner.java
index 53536bafb..36c9feb8d 100644
--- a/javatests/google/registry/webdriver/RepeatableRunner.java
+++ b/javatests/google/registry/webdriver/RepeatableRunner.java
@@ -153,7 +153,7 @@ public class RepeatableRunner extends BlockJUnit4ClassRunner {
         } catch (Throwable e) {
           numFailure++;
           lastException = e;
-          logger.atWarning().log(
+          logger.atWarning().withCause(e).log(
               "[%s] Attempt %d of %d failed!\n", method.getName(), attempt, MAX_ATTEMPTS);
         }
       }
diff --git a/javatests/google/registry/webdriver/TestServerRule.java b/javatests/google/registry/webdriver/TestServerRule.java
index aa4b96839..2ab547225 100644
--- a/javatests/google/registry/webdriver/TestServerRule.java
+++ b/javatests/google/registry/webdriver/TestServerRule.java
@@ -54,8 +54,11 @@ public final class TestServerRule extends ExternalResource {
   private final ImmutableList<Fixture> fixtures;
   private final AppEngineRule appEngineRule;
   private final BlockingQueue<FutureTask<?>> jobs = new LinkedBlockingDeque<>();
-  private final TestServer testServer;
+  private final ImmutableMap<String, Path> runfiles;
+  private final ImmutableList<Route> routes;
+  private final ImmutableList<Class<? extends Filter>> filters;
 
+  private TestServer testServer;
   private Thread serverThread;
 
   private TestServerRule(
@@ -64,6 +67,9 @@ public final class TestServerRule extends ExternalResource {
       ImmutableList<Class<? extends Filter>> filters,
       ImmutableList<Fixture> fixtures,
       String email) {
+    this.runfiles = runfiles;
+    this.routes = routes;
+    this.filters = filters;
     this.fixtures = fixtures;
     // We create an GAE-Admin user, and then use AuthenticatedRegistrarAccessor.bypassAdminCheck to
     // choose whether the user is an admin or not.
@@ -74,8 +80,12 @@ public final class TestServerRule extends ExternalResource {
         .withTaskQueue()
         .withUserService(UserInfo.createAdmin(email, THE_REGISTRAR_GAE_USER_ID))
         .build();
+  }
+
+  @Override
+  protected void before() throws InterruptedException {
     try {
-      this.testServer =
+      testServer =
           new TestServer(
               HostAndPort.fromParts(
                   // Use external IP address here so the browser running inside Docker container
@@ -87,15 +97,14 @@ public final class TestServerRule extends ExternalResource {
     } catch (UnknownHostException e) {
       throw new IllegalStateException(e);
     }
-  }
-
-  @Override
-  protected void before() throws InterruptedException {
     setIsAdmin(false);
-    serverThread = new Thread(new Server());
-    synchronized (testServer) {
+    Server server = new Server();
+    serverThread = new Thread(server);
+    synchronized (this) {
       serverThread.start();
-      testServer.wait();
+      while (!server.isRunning) {
+        this.wait();
+      }
     }
   }
 
@@ -112,6 +121,7 @@ public final class TestServerRule extends ExternalResource {
     } finally {
       serverThread = null;
       jobs.clear();
+      testServer = null;
     }
   }
 
@@ -149,6 +159,8 @@ public final class TestServerRule extends ExternalResource {
   }
 
   private final class Server extends Statement implements Runnable {
+    private volatile boolean isRunning = false;
+
     @Override
     public void run() {
       try {
@@ -169,8 +181,9 @@ public final class TestServerRule extends ExternalResource {
       }
       testServer.start();
       System.out.printf("TestServerRule is listening on: %s\n", testServer.getUrl("/"));
-      synchronized (testServer) {
-        testServer.notify();
+      synchronized (TestServerRule.this) {
+        isRunning = true;
+        TestServerRule.this.notify();
       }
       try {
         while (true) {