Merge "Fix temporary file creation and cleanup during testing"

gerrit-acceptance-tests/pom.xml

@@ -126,6 +126,9 @@ limitations under the License.
             <groupId>org.apache.maven.plugins</groupId>
             <artifactId>maven-failsafe-plugin</artifactId>
             <version>2.5</version>
+            <configuration>
+              <argLine>-Djava.io.tmpdir=${project.build.directory}</argLine>
+            </configuration>
             <executions>
               <execution>
                 <id>integration-test</id>

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

@@ -14,17 +14,6 @@
 
 package com.google.gerrit.acceptance;
 
-import java.lang.reflect.Field;
-import java.text.DateFormat;
-import java.text.SimpleDateFormat;
-import java.util.Date;
-import java.util.concurrent.BrokenBarrierException;
-import java.util.concurrent.Callable;
-import java.util.concurrent.CyclicBarrier;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.TimeUnit;
-
 import com.google.gerrit.lifecycle.LifecycleManager;
 import com.google.gerrit.pgm.Daemon;
 import com.google.gerrit.pgm.Init;
@@ -32,15 +21,21 @@ import com.google.gerrit.server.config.FactoryModule;
 import com.google.inject.Injector;
 import com.google.inject.Module;
 
+import java.io.File;
+import java.lang.reflect.Field;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
 class GerritServer {
 
   /** Returns fully started Gerrit server */
   static GerritServer start() throws Exception {
-
+    final File site = initSite();
-    final String sitePath = initSite();
-
     final CyclicBarrier serverStarted = new CyclicBarrier(2);
-
     final Daemon daemon = new Daemon(new Runnable() {
       public void run() {
         try {
@@ -56,10 +51,10 @@ class GerritServer {
     ExecutorService daemonService = Executors.newSingleThreadExecutor();
     daemonService.submit(new Callable<Void>() {
       public Void call() throws Exception {
-        int rc = daemon.main(new String[] {"-d", sitePath, "--headless" });
+        int rc = daemon.main(new String[] {"-d", site.getPath(), "--headless" });
         if (rc != 0) {
           System.out.println("Failed to start Gerrit daemon. Check "
-              + sitePath + "/logs/error_log");
+              + site.getPath() + "/logs/error_log");
           serverStarted.reset();
         }
         return null;
@@ -70,18 +65,18 @@ class GerritServer {
     System.out.println("Gerrit Server Started");
 
     Injector i = createTestInjector(daemon);
-    return new GerritServer(i, daemon, daemonService);
+    return new GerritServer(site, i, daemon, daemonService);
   }
 
-  private static String initSite() throws Exception {
+  private static File initSite() throws Exception {
-    DateFormat df = new SimpleDateFormat("yyyyMMddHHmmss");
+    File tmp = TempFileUtil.createTempDirectory();
-    String path = "target/test_site_" + df.format(new Date());
     Init init = new Init();
-    int rc = init.main(new String[] {"-d", path, "--batch", "--no-auto-start"});
+    int rc = init.main(new String[] {
+        "-d", tmp.getPath(), "--batch", "--no-auto-start"});
     if (rc != 0) {
       throw new RuntimeException("Couldn't initialize site");
     }
-    return path;
+    return tmp;
   }
 
   private static Injector createTestInjector(Daemon daemon) throws Exception {
@@ -103,12 +98,14 @@ class GerritServer {
     return (T) f.get(obj);
   }
 
+  private File sitePath;
   private Daemon daemon;
   private ExecutorService daemonService;
   private Injector testInjector;
 
-  private GerritServer(Injector testInjector,
+  private GerritServer(File sitePath, Injector testInjector,
       Daemon daemon, ExecutorService daemonService) {
+    this.sitePath = sitePath;
     this.testInjector = testInjector;
     this.daemon = daemon;
     this.daemonService = daemonService;
@@ -124,5 +121,6 @@ class GerritServer {
     manager.stop();
     daemonService.shutdownNow();
     daemonService.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
+    TempFileUtil.recursivelyDelete(sitePath);
   }
 }

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

@@ -15,27 +15,41 @@
 package com.google.gerrit.acceptance;
 
 import java.io.File;
-import java.text.DateFormat;
+import java.io.IOException;
-import java.text.SimpleDateFormat;
-import java.util.Date;
 
 public class TempFileUtil {
-
+  public static File createTempDirectory() throws IOException {
-  private static int testCount;
+    File tmp = File.createTempFile("gerrit_test_", "");
-  private static DateFormat df = new SimpleDateFormat("yyyyMMddHHmmss");
+    if (!tmp.delete() || !tmp.mkdir()) {
-  private static final File temp = new File(new File("target"), "temp");
+      throw new IOException("Cannot create " + tmp.getPath());
-
+    }
-  private static String createUniqueTestFolderName() {
+    return tmp;
-    return "test_" + (df.format(new Date()) + "_" + (testCount++));
   }
 
-  public static File createTempDirectory() {
+  public static void recursivelyDelete(File dir) throws IOException {
-    final String name = createUniqueTestFolderName();
+    if (!dir.getPath().equals(dir.getCanonicalPath())) {
-    final File directory = new File(temp, name);
+      // Directory symlink reaching outside of temporary space.
-    if (!directory.mkdirs()) {
+      throw new IOException("Refusing to clear symlink " + dir.getPath());
-      throw new RuntimeException("failed to create folder '"
-          + directory.getAbsolutePath() + "'");
     }
-    return directory;
+    File[] contents = dir.listFiles();
+    if (contents != null) {
+      for (File d : contents) {
+        if (d.isDirectory()) {
+          recursivelyDelete(d);
+        } else {
+          deleteNowOrOnExit(d);
+        }
+      }
+    }
+    deleteNowOrOnExit(dir);
+  }
+
+  private static void deleteNowOrOnExit(File dir) {
+    if (!dir.delete()) {
+      dir.deleteOnExit();
+    }
+  }
+
+  private TempFileUtil() {
   }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java

@@ -77,7 +77,7 @@ public class GitUtil {
     s.exec("gerrit create-project --empty-commit --name \"" + name + "\"");
   }
 
-  public static Git cloneProject(String url) throws GitAPIException {
+  public static Git cloneProject(String url) throws GitAPIException, IOException {
     final File gitDir = TempFileUtil.createTempDirectory();
     final CloneCommand cloneCmd = Git.cloneRepository();
     cloneCmd.setURI(url);

gerrit-server/pom.xml

@@ -232,6 +232,14 @@ limitations under the License.
         </executions>
       </plugin>
 
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <configuration>
+          <argLine>-Djava.io.tmpdir=${project.build.directory}</argLine>
+        </configuration>
+      </plugin>
+
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-source-plugin</artifactId>

gerrit-server/src/test/java/com/google/gerrit/server/config/SitePathsTest.java

@@ -21,10 +21,9 @@ import junit.framework.TestCase;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.util.UUID;
 
 public class SitePathsTest extends TestCase {
-  public void testCreate_NotExisting() throws FileNotFoundException {
+  public void testCreate_NotExisting() throws IOException {
     final File root = random();
     final SitePaths site = new SitePaths(root);
     assertTrue(site.isNew);
@@ -32,7 +31,7 @@ public class SitePathsTest extends TestCase {
     assertEquals(new File(root, "etc"), site.etc_dir);
   }
 
-  public void testCreate_Empty() throws FileNotFoundException {
+  public void testCreate_Empty() throws IOException {
     final File root = random();
     try {
       assertTrue(root.mkdir());
@@ -91,8 +90,11 @@ public class SitePathsTest extends TestCase {
     assertEquals(new File(pfx + "a").getCanonicalFile(), site.resolve(pfx + "a"));
   }
 
-  private File random() {
+  private static File random() throws IOException {
-    final File t = new File("target");
+    File tmp = File.createTempFile("gerrit_test_", "_site");
-    return new File(t, "random-name-" + UUID.randomUUID().toString());
+    if (!tmp.delete()) {
+      throw new IOException("Cannot create " + tmp.getPath());
+    }
+    return tmp;
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/tools/hooks/HookTestCase.java

@@ -52,6 +52,8 @@ package com.google.gerrit.server.tools.hooks;
 
 import static org.junit.Assert.fail;
 
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 
 import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
@@ -64,10 +66,13 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.util.List;
+import java.util.Map;
 
 public abstract class HookTestCase extends LocalDiskRepositoryTestCase {
   protected Repository repository;
-  private File hooksh;
+  private final Map<String, File> hooks = Maps.newTreeMap();
+  private final List<File> cleanup = Lists.newArrayList();
 
   @Override
   @Before
@@ -79,15 +84,21 @@ public abstract class HookTestCase extends LocalDiskRepositoryTestCase {
   @Override
   @After
   public void tearDown() throws Exception {
-    if (hooksh != null) {
+    super.tearDown();
-      if (!hooksh.delete()) {
+    for (File p : cleanup) {
-        hooksh.deleteOnExit();
+      if (!p.delete()) {
+        p.deleteOnExit();
       }
-      hooksh = null;
     }
+    cleanup.clear();
   }
 
   protected File getHook(final String name) throws IOException {
+    File hook = hooks.get(name);
+    if (hook != null) {
+      return hook;
+    }
+
     final String scproot = "com/google/gerrit/server/tools/root";
     final String path = scproot + "/hooks/" + name;
     URL url = cl().getResource(path);
@@ -95,17 +106,22 @@ public abstract class HookTestCase extends LocalDiskRepositoryTestCase {
       fail("Cannot locate " + path + " in CLASSPATH");
     }
 
-    File hook;
     if ("file".equals(url.getProtocol())) {
       hook = new File(url.getPath());
       if (!hook.isFile()) {
         fail("Cannot locate " + path + " in CLASSPATH");
       }
+      long time = hook.lastModified();
+      hook.setExecutable(true);
+      hook.setLastModified(time);
+      hooks.put(name, hook);
+      return hook;
     } else if ("jar".equals(url.getProtocol())) {
-      hooksh = File.createTempFile("hook_", ".sh");
       InputStream in = url.openStream();
       try {
-        FileOutputStream out = new FileOutputStream(hooksh);
+        hook = File.createTempFile("hook_", ".sh");
+        cleanup.add(hook);
+        FileOutputStream out = new FileOutputStream(hook);
         try {
           ByteStreams.copy(in, out);
         } finally {
@@ -114,21 +130,13 @@ public abstract class HookTestCase extends LocalDiskRepositoryTestCase {
       } finally {
         in.close();
       }
-      hook = hooksh;
+      hook.setExecutable(true);
+      hooks.put(name, hook);
+      return hook;
     } else {
       fail("Cannot invoke " + url);
-      hook = null;
+      return null;
     }
-
-    // The hook was copied out of our source control system into the
-    // target area by Java tools. Its not executable in the source
-    // are, nor did the copying Java program make it executable in the
-    // destination area. So we must force it to be executable.
-    //
-    final long time = hook.lastModified();
-    hook.setExecutable(true);
-    hook.setLastModified(time);
-    return hook;
   }
 
   private ClassLoader cl() {