Reuse running daemon in AbstractDaemonTest

Maintain a single common daemon per test runner, and reuse it without
additional modification wherever possible. Associate each server with
an immutable Description class, and use a new, custom daemon when the
Description corresponding to the test class does not match the
Description corresponding to the method.

Some unscientific benchmarks, running tests after a full build.

On my workstation (HP Z420, 12 cores, 32G RAM, SSD):
  buck test --no-results-cache
    before: 4:37.88
    after:  3:22.36
  ChangeIT in Eclipse
    before: 26.153s
    after:  12.512s

On my laptop (11" MBA, 4 cores, 8G RAM, SSD):
  buck test --no-results-cache
    before: 15:34.28
    after:  11:34.93
  ChangeIT in Eclipse
    before: 30.903s
    after:  19.11s

We still pay a significant startup cost (~6-12s for the server), and
contention during buck test with default -j is an issue (3-5x increase
in time per test). But a 50%/25% reduction in single/aggregate test
time is nothing to sneeze at.

There is still plenty of low-hanging fruit that can take better
advantage of the per-test-class savings, like consolidating more
groups of integration tests together and removing HTTP dependencies.

Change-Id: I23b8d0e9839120a07a3fe1230f19184dc910682c

gerrit-acceptance-tests/BUCK

@@ -28,6 +28,7 @@ java_library(
     '//lib:servlet-api-3_1',
     '//lib:truth',
 
+    '//lib/auto:auto-value',
     '//lib/httpcomponents:httpclient',
     '//lib/httpcomponents:httpcore',
     '//lib/log:impl_log4j',

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -66,6 +66,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
+import org.junit.AfterClass;
 import org.junit.Rule;
 import org.junit.rules.TestRule;
 import org.junit.runner.Description;
@@ -81,6 +82,8 @@ import java.util.regex.Pattern;
 
 @RunWith(ConfigSuite.class)
 public abstract class AbstractDaemonTest {
+  private static GerritServer commonServer;
+
   @ConfigSuite.Parameter
   public Config baseConfig;
 
@@ -148,30 +151,24 @@ public abstract class AbstractDaemonTest {
       return new Statement() {
         @Override
         public void evaluate() throws Throwable {
-          boolean mem = description.getAnnotation(UseLocalDisk.class) == null;
-          boolean enableHttpd = description.getAnnotation(NoHttpd.class) == null
-              && description.getTestClass().getAnnotation(NoHttpd.class) == null;
-          beforeTest(description, mem, enableHttpd);
-          base.evaluate();
-          afterTest();
+          beforeTest(description);
+          try {
+            base.evaluate();
+          } finally {
+            afterTest();
+          }
         }
       };
     }
   };
 
-  private Config config(Description description) {
-    GerritConfigs cfgs = description.getAnnotation(GerritConfigs.class);
-    GerritConfig cfg = description.getAnnotation(GerritConfig.class);
-    if (cfgs != null && cfg != null) {
-      throw new IllegalStateException("Use either @GerritConfigs or @GerritConfig not both");
-    }
-    if (cfgs != null) {
-      return ConfigAnnotationParser.parse(baseConfig, cfgs);
-    } else if (cfg != null) {
-      return ConfigAnnotationParser.parse(baseConfig, cfg);
-    } else {
-      return baseConfig;
+  @AfterClass
+  public static void stopCommonServer() throws Exception {
+    if (commonServer != null) {
+      commonServer.stop();
+      commonServer = null;
     }
+    TempFileUtil.cleanup();
   }
 
   protected static Config submitWholeTopicEnabledConfig() {
@@ -194,10 +191,21 @@ public abstract class AbstractDaemonTest {
     return cfg.getBoolean("change", null, "submitWholeTopic", false);
   }
 
-  private void beforeTest(Description description, boolean memory,
-      boolean enableHttpd) throws Exception {
-    Config cfg = config(description);
-    server = startServer(cfg, memory, enableHttpd);
+  private void beforeTest(Description description) throws Exception {
+    GerritServer.Description classDesc =
+      GerritServer.Description.forTestClass(description, configName);
+    GerritServer.Description methodDesc =
+      GerritServer.Description.forTestMethod(description, configName);
+
+    if (classDesc.equals(methodDesc)) {
+      if (commonServer == null) {
+        commonServer = GerritServer.start(classDesc, baseConfig);
+      }
+      server = commonServer;
+    } else {
+      server = GerritServer.start(methodDesc, baseConfig);
+    }
+
     server.getTestInjector().injectMembers(this);
     admin = accounts.admin();
     user = accounts.user();
@@ -298,16 +306,12 @@ public abstract class AbstractDaemonTest {
     // Default implementation does nothing.
   }
 
-  protected GerritServer startServer(Config cfg, boolean memory,
-      boolean enableHttpd) throws Exception {
-    return GerritServer.start(cfg, memory, enableHttpd);
-  }
-
   private void afterTest() throws Exception {
     db.close();
     sshSession.close();
-    server.stop();
-    TempFileUtil.cleanup();
+    if (server != commonServer) {
+      server.stop();
+    }
   }
 
   protected TestRepository<?>.CommitBuilder commitBuilder() throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java

@@ -14,7 +14,10 @@
 
 package com.google.gerrit.acceptance;
 
+import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.lucene.LuceneIndexModule;
 import com.google.gerrit.pgm.Daemon;
 import com.google.gerrit.pgm.Init;
@@ -46,10 +49,55 @@ import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 
 public class GerritServer {
+  @AutoValue
+  static abstract class Description {
+    static Description forTestClass(org.junit.runner.Description testDesc,
+        String configName) {
+      return new AutoValue_GerritServer_Description(
+          configName,
+          true, // @UseLocalDisk is only valid on methods.
+          testDesc.getTestClass().getAnnotation(NoHttpd.class) == null,
+          null, // @GerritConfig is only valid on methods.
+          null); // @GerritConfigs is only valid on methods.
+
+    }
+
+    static Description forTestMethod(org.junit.runner.Description testDesc,
+        String configName) {
+      return new AutoValue_GerritServer_Description(
+          configName,
+          testDesc.getAnnotation(UseLocalDisk.class) == null,
+          testDesc.getAnnotation(NoHttpd.class) == null
+            && testDesc.getTestClass().getAnnotation(NoHttpd.class) == null,
+          testDesc.getAnnotation(GerritConfig.class),
+          testDesc.getAnnotation(GerritConfigs.class));
+    }
+
+    @Nullable abstract String configName();
+    abstract boolean memory();
+    abstract boolean httpd();
+    @Nullable abstract GerritConfig config();
+    @Nullable abstract GerritConfigs configs();
+
+    private Config buildConfig(Config baseConfig) {
+      if (configs() != null && config() != null) {
+        throw new IllegalStateException(
+            "Use either @GerritConfigs or @GerritConfig not both");
+      }
+      if (configs() != null) {
+        return ConfigAnnotationParser.parse(baseConfig, configs());
+      } else if (config() != null) {
+        return ConfigAnnotationParser.parse(baseConfig, config());
+      } else {
+        return baseConfig;
+      }
+    }
+  }
 
   /** Returns fully started Gerrit server */
-  static GerritServer start(Config cfg, boolean memory, boolean enableHttpd)
+  static GerritServer start(Description desc, Config baseConfig)
       throws Exception {
+    Config cfg = desc.buildConfig(baseConfig);
     Logger.getLogger("com.google.gerrit").setLevel(Level.DEBUG);
     final CyclicBarrier serverStarted = new CyclicBarrier(2);
     final Daemon daemon = new Daemon(new Runnable() {
@@ -67,14 +115,14 @@ public class GerritServer {
 
     final File site;
     ExecutorService daemonService = null;
-    if (memory) {
+    if (desc.memory()) {
       site = null;
       mergeTestConfig(cfg);
       cfg.setBoolean("httpd", null, "requestLog", false);
       cfg.setBoolean("sshd", null, "requestLog", false);
       cfg.setBoolean("index", "lucene", "testInmemory", true);
       cfg.setString("gitweb", null, "cgi", "");
-      daemon.setEnableHttpd(enableHttpd);
+      daemon.setEnableHttpd(desc.httpd());
       daemon.setLuceneModule(new LuceneIndexModule(
           ChangeSchemas.getLatest().getVersion(), 0, null));
       daemon.setDatabaseForTesting(ImmutableList.<Module>of(
@@ -100,7 +148,7 @@ public class GerritServer {
     }
 
     Injector i = createTestInjector(daemon);
-    return new GerritServer(i, daemon, daemonService);
+    return new GerritServer(desc, i, daemon, daemonService);
   }
 
   private static File initSite(Config base) throws Exception {
@@ -167,6 +215,8 @@ public class GerritServer {
     return InetAddress.getLoopbackAddress();
   }
 
+  private final Description desc;
+
   private Daemon daemon;
   private ExecutorService daemonService;
   private Injector testInjector;
@@ -174,8 +224,9 @@ public class GerritServer {
   private InetSocketAddress sshdAddress;
   private InetSocketAddress httpAddress;
 
-  private GerritServer(Injector testInjector, Daemon daemon,
+  private GerritServer(Description desc, Injector testInjector, Daemon daemon,
       ExecutorService daemonService) {
+    this.desc = desc;
     this.testInjector = testInjector;
     this.daemon = daemon;
     this.daemonService = daemonService;
@@ -207,6 +258,10 @@ public class GerritServer {
     return testInjector;
   }
 
+  Description getDescription() {
+    return desc;
+  }
+
   void stop() throws Exception {
     daemon.getLifecycleManager().stop();
     if (daemonService != null) {
@@ -216,4 +271,9 @@ public class GerritServer {
     }
     RepositoryCache.clear();
   }
+
+  @Override
+  public String toString() {
+    return MoreObjects.toStringHelper(this).addValue(desc).toString();
+  }
 }