AbstractDaemonTest: Don't delete common server path too early
Chaining the TemporaryFolder with the custom TestRule results in the
directory for the common server being deleted after the first test
method, even though it might still be needed. This means a @UseLocalDisk
class cannot have more than one method, or things break, including but
not limited to the NRT reopen threads throwing exceptions due to the
index files disappearing out from under them. We just got lucky that
none of our local disk tests were actually affected by this.
To make this work, we can't tie the lifetime of the TemporaryFolder to a
single AbstractDaemonTest method invocation. Instead, create the tempdir
within GerritServer#initAndStart, and allow callers to get it back out.
We have to be a bit more careful about managing the lifetime of the
tempdir since some GerritServers, particularly in StandaloneSiteTest,
have a shorter lifetime than their tempdirs. Fortunately, TempFileUtil
make this relatively easy.
As a nice side effect, we can get rid of the hack of passing the tempdir
in via a Gerrit config option.
Change-Id: I80ac66a27bc990f2cef966c9b372c86ce277c471
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index ba0b70c..8868987 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -151,7 +151,6 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.rules.ExpectedException;
-import org.junit.rules.TemporaryFolder;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runner.RunWith;
@@ -268,8 +267,6 @@
}
};
- @Rule public TemporaryFolder tempSiteDir = new TemporaryFolder();
-
@Before
public void clearSender() {
sender.clear();
@@ -337,7 +334,6 @@
GerritServer.Description methodDesc =
GerritServer.Description.forTestMethod(description, configName);
- baseConfig.setString("gerrit", null, "tempSiteDir", tempSiteDir.getRoot().getPath());
baseConfig.setInt("receive", null, "changeUpdateThreads", 4);
if (classDesc.equals(methodDesc) && !classDesc.sandboxed() && !methodDesc.sandboxed()) {
if (commonServer == null) {
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
index bb3cab8..5312c73 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
@@ -36,6 +36,7 @@
import com.google.gerrit.testutil.NoteDbChecker;
import com.google.gerrit.testutil.NoteDbMode;
import com.google.gerrit.testutil.SshMode;
+import com.google.gerrit.testutil.TempFileUtil;
import com.google.inject.Injector;
import com.google.inject.Key;
import com.google.inject.Module;
@@ -45,7 +46,6 @@
import java.net.InetSocketAddress;
import java.net.URI;
import java.nio.file.Path;
-import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.BrokenBarrierException;
@@ -217,20 +217,29 @@
/**
* Initializes new Gerrit site and returns started server.
*
+ * <p>A new temporary directory for the site will be created with {@link TempFileUtil}, even in
+ * the server is otherwise configured in-memory. Closing the server stops the daemon but does not
+ * delete the temporary directory. Callers may either get the directory with {@link
+ * #getSitePath()} and delete it manually, or call {@link TempFileUtil#cleanup()}.
+ *
* @param desc server description.
- * @param baseConfig default config values; merged with config from {@code desc}. Must contain a
- * value for {@code gerrit.tempSiteDir} pointing to a temporary directory. This directory is
- * only actually used for on-disk sites ({@link Description#memory()} returns false), but it
- * must nonetheless exist for in-memory sites.
+ * @param baseConfig default config values; merged with config from {@code desc}.
* @return started server.
* @throws Exception
*/
public static GerritServer initAndStart(Description desc, Config baseConfig) throws Exception {
- Path site = Paths.get(baseConfig.getString("gerrit", null, "tempSiteDir"));
- if (!desc.memory()) {
- init(desc, baseConfig, site);
+ Path site = TempFileUtil.createTempDirectory().toPath();
+ baseConfig = new Config(baseConfig);
+ baseConfig.setString("gerrit", null, "tempSiteDir", site.toString());
+ try {
+ if (!desc.memory()) {
+ init(desc, baseConfig, site);
+ }
+ return start(desc, baseConfig, site);
+ } catch (Exception e) {
+ TempFileUtil.recursivelyDelete(site.toFile());
+ throw e;
}
- return start(desc, baseConfig, site);
}
/**
@@ -267,13 +276,13 @@
daemon.setEnableSshd(SshMode.useSsh());
if (desc.memory()) {
- return startInMemory(desc, baseConfig, daemon);
+ return startInMemory(desc, site, baseConfig, daemon);
}
return startOnDisk(desc, site, daemon, serverStarted);
}
- private static GerritServer startInMemory(Description desc, Config baseConfig, Daemon daemon)
- throws Exception {
+ private static GerritServer startInMemory(
+ Description desc, Path site, Config baseConfig, Daemon daemon) throws Exception {
Config cfg = desc.buildConfig(baseConfig);
mergeTestConfig(cfg);
// Set the log4j configuration to an invalid one to prevent system logs
@@ -285,9 +294,10 @@
cfg.setString("gitweb", null, "cgi", "");
daemon.setEnableHttpd(desc.httpd());
daemon.setLuceneModule(LuceneIndexModule.singleVersionAllLatest(0));
- daemon.setDatabaseForTesting(ImmutableList.<Module>of(new InMemoryTestingDatabaseModule(cfg)));
+ daemon.setDatabaseForTesting(
+ ImmutableList.<Module>of(new InMemoryTestingDatabaseModule(cfg, site)));
daemon.start();
- return new GerritServer(desc, createTestInjector(daemon), daemon, null);
+ return new GerritServer(desc, null, createTestInjector(daemon), daemon, null);
}
private static GerritServer startOnDisk(
@@ -317,7 +327,7 @@
}
System.out.println("Gerrit Server Started");
- return new GerritServer(desc, createTestInjector(daemon), daemon, daemonService);
+ return new GerritServer(desc, site, createTestInjector(daemon), daemon, daemonService);
}
private static void mergeTestConfig(Config cfg) {
@@ -370,6 +380,7 @@
}
private final Description desc;
+ private final Path sitePath;
private Daemon daemon;
private ExecutorService daemonService;
@@ -379,10 +390,15 @@
private InetSocketAddress httpAddress;
private GerritServer(
- Description desc, Injector testInjector, Daemon daemon, ExecutorService daemonService) {
- this.desc = desc;
- this.testInjector = testInjector;
- this.daemon = daemon;
+ Description desc,
+ @Nullable Path sitePath,
+ Injector testInjector,
+ Daemon daemon,
+ @Nullable ExecutorService daemonService) {
+ this.desc = checkNotNull(desc);
+ this.sitePath = sitePath;
+ this.testInjector = checkNotNull(testInjector);
+ this.daemon = checkNotNull(daemon);
this.daemonService = daemonService;
Config cfg = testInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
@@ -428,6 +444,10 @@
}
}
+ public Path getSitePath() {
+ return sitePath;
+ }
+
private void checkNoteDbState() throws Exception {
NoteDbMode mode = NoteDbMode.get();
if (mode != NoteDbMode.CHECK && mode != NoteDbMode.PRIMARY) {
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
index 1096b03..551c26b 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
@@ -51,16 +51,18 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.nio.file.Paths;
import org.apache.sshd.common.keyprovider.KeyPairProvider;
import org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider;
import org.eclipse.jgit.lib.Config;
class InMemoryTestingDatabaseModule extends LifecycleModule {
private final Config cfg;
+ private final Path sitePath;
- InMemoryTestingDatabaseModule(Config cfg) {
+ InMemoryTestingDatabaseModule(Config cfg, Path sitePath) {
this.cfg = cfg;
+ this.sitePath = sitePath;
+ makeSiteDirs(sitePath);
}
@Override
@@ -68,9 +70,7 @@
bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(cfg);
// TODO(dborowitz): Use jimfs.
- Path p = Paths.get(cfg.getString("gerrit", null, "tempSiteDir"));
- bind(Path.class).annotatedWith(SitePath.class).toInstance(p);
- makeSiteDirs(p);
+ bind(Path.class).annotatedWith(SitePath.class).toInstance(sitePath);
bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class);
bind(InMemoryRepositoryManager.class).in(SINGLETON);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
index 64849a4..55ca719 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -27,12 +27,15 @@
import com.google.gerrit.server.config.AllProjectsNameProvider;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.inject.Inject;
import java.nio.file.Files;
import java.nio.file.Path;
import org.junit.Test;
@NoHttpd
public class ServerInfoIT extends AbstractDaemonTest {
+ @Inject private SitePaths sitePaths;
@Test
// auth
@@ -131,7 +134,8 @@
@UseSsh
@GerritConfig(name = "plugins.allowRemoteAdmin", value = "true")
public void serverConfigWithPlugin() throws Exception {
- Path plugins = tempSiteDir.newFolder("plugins").toPath();
+ Path plugins = sitePaths.plugins_dir;
+ Files.createDirectory(plugins);
Path jsplugin = plugins.resolve("js-plugin-1.js");
Files.write(jsplugin, "Gerrit.install(function(self){});\n".getBytes(UTF_8));
adminSshSession.exec("gerrit plugin reload");
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TempFileUtil.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TempFileUtil.java
index eb50b47..f90a4fe 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TempFileUtil.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TempFileUtil.java
@@ -38,7 +38,7 @@
allDirsCreated.clear();
}
- private static void recursivelyDelete(File dir) throws IOException {
+ public static void recursivelyDelete(File dir) throws IOException {
if (!dir.getPath().equals(dir.getCanonicalPath())) {
// Directory symlink reaching outside of temporary space.
return;
