From 33390fe8ac2e6b48d6540901f45128fdb9a2331e Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 20 Oct 2017 11:01:07 +0200 Subject: [PATCH] Enable testing of Gerrit in slave mode Recently we had a few breakages of running Gerrit in slave mode (e.g. due to trying to access secondary indexes which don't exist on slaves). These breakages were only noticed after user reports since we don't have any integration tests for Gerrit slaves at all. This change enables integration tests against Gerrit slaves. It is easy to start the Gerrit daemon in slave mode but then the server is in read-only mode and we can't create any test data using the Gerrit API. To be able to test against a slave server which is populated with test data we first start the daemon as master and then after the test data has been created we restart the daemon in slave mode. If the server is restarted in slave mode we must preserve the content of the test site, the repositories and the database content. Preserving the content of the test site is not a problem since the test site folders are only deleted at the very end after executing all tests. The git repositories and the database may exist only in memory and in this case we must make sure that the same instances are used when the daemon is restarted in slave mode. As in-memory database we use H2 [1]. The contents of an in-memory H2 database are gone as soon as the last connection to the database is closed. This is why we must ensure to keep a database connection open when then server in master mode is shutdown and we intend to restart it as slave. When the server is restarted we must make sure to reopen SSH connections and reinject members into the test class and into the project resettter. Tests that want to run against a Gerrit slave can use the normal Gerrit API to setup the test data and then use the 'restartAsGerritSlave' method from AbstractDaemonTest to restart the daemon in slave mode. For these tests it is required to use the @Sandboxed annotation so that they get an own server instance. Restarting the common server instance as slave would break other tests using this server. As an example the SshCommandsIT test is extended to check the list of available commands in master and slave mode. Also it executes all available commands in both modes. [1] http://www.h2database.com/html/features.html#in_memory_databases Change-Id: I2856e04ffa9fe4aac63747d4fc463581f7a1a6d6 Signed-off-by: Edwin Kempin --- .../gerrit/acceptance/AbstractDaemonTest.java | 65 ++++--- .../gerrit/acceptance/GerritServer.java | 62 ++++++- .../InMemoryTestingDatabaseModule.java | 23 ++- .../gerrit/acceptance/ProjectResetter.java | 11 +- .../gerrit/acceptance/StandaloneSiteTest.java | 2 +- java/com/google/gerrit/pgm/Daemon.java | 4 + .../gerrit/testing/InMemoryDatabase.java | 115 ++++++++----- .../gerrit/acceptance/ssh/SshCommandsIT.java | 158 +++++++++++++----- 8 files changed, 321 insertions(+), 119 deletions(-) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index bcde03c4c1..3c5e458511 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assert_; import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.acceptance.GitUtil.initSsh; import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES; import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG; import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; @@ -129,6 +128,7 @@ import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; +import com.jcraft.jsch.JSchException; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; @@ -201,8 +201,10 @@ public abstract class AbstractDaemonTest { } beforeTest(description); try (ProjectResetter resetter = resetProjects(projectResetter.builder())) { + AbstractDaemonTest.this.resetter = resetter; base.evaluate(); } finally { + AbstractDaemonTest.this.resetter = null; afterTest(); } } @@ -267,6 +269,7 @@ public abstract class AbstractDaemonTest { @Inject private SchemaFactory reviewDbProvider; @Inject private Groups groups; + private ProjectResetter resetter; private List toClose; @Before @@ -330,6 +333,16 @@ public abstract class AbstractDaemonTest { .build(); } + protected void restartAsSlave() throws Exception { + closeSsh(); + server = GerritServer.restartAsSlave(server); + server.getTestInjector().injectMembers(this); + if (resetter != null) { + server.getTestInjector().injectMembers(resetter); + } + initSsh(); + } + protected static Config submitWholeTopicEnabledConfig() { Config cfg = new Config(); cfg.setBoolean("change", null, "submitWholeTopic", true); @@ -396,20 +409,7 @@ public abstract class AbstractDaemonTest { adminRestSession = new RestSession(server, admin); userRestSession = new RestSession(server, user); - if (testRequiresSsh - && SshMode.useSsh() - && (adminSshSession == null || userSshSession == null)) { - // Create Ssh sessions - initSsh(admin); - Context ctx = newRequestContext(user); - atrScope.set(ctx); - userSshSession = ctx.getSession(); - userSshSession.open(); - ctx = newRequestContext(admin); - atrScope.set(ctx); - adminSshSession = ctx.getSession(); - adminSshSession.open(); - } + initSsh(); resourcePrefix = UNSAFE_PROJECT_NAME @@ -422,6 +422,23 @@ public abstract class AbstractDaemonTest { testRepo = cloneProject(project, getCloneAsAccount(description)); } + protected void initSsh() throws JSchException { + if (testRequiresSsh + && SshMode.useSsh() + && (adminSshSession == null || userSshSession == null)) { + // Create Ssh sessions + GitUtil.initSsh(admin); + Context ctx = newRequestContext(user); + atrScope.set(ctx); + userSshSession = ctx.getSession(); + userSshSession.open(); + ctx = newRequestContext(admin); + atrScope.set(ctx); + adminSshSession = ctx.getSession(); + adminSshSession.open(); + } + } + private TestAccount getCloneAsAccount(Description description) { TestProjectInput ann = description.getAnnotation(TestProjectInput.class); return accountCreator.get(ann != null ? ann.cloneAs() : "admin"); @@ -557,12 +574,7 @@ public abstract class AbstractDaemonTest { repo.close(); } db.close(); - if (adminSshSession != null) { - adminSshSession.close(); - } - if (userSshSession != null) { - userSshSession.close(); - } + closeSsh(); if (server != commonServer) { server.close(); server = null; @@ -570,6 +582,17 @@ public abstract class AbstractDaemonTest { NoteDbMode.resetFromEnv(notesMigration); } + protected void closeSsh() { + if (adminSshSession != null) { + adminSshSession.close(); + adminSshSession = null; + } + if (userSshSession != null) { + userSshSession.close(); + userSshSession = null; + } + } + protected TestRepository.CommitBuilder commitBuilder() throws Exception { return testRepo.branch("HEAD").commit().insertChangeId(); } diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 1b9e8aa832..5262b745df 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; @@ -27,6 +28,7 @@ import com.google.gerrit.lucene.LuceneIndexModule; import com.google.gerrit.pgm.Daemon; import com.google.gerrit.pgm.Init; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.config.SitePath; import com.google.gerrit.server.git.receive.AsyncReceiveCommits; import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.server.util.ManualRequestContext; @@ -35,6 +37,8 @@ import com.google.gerrit.server.util.SocketUtil; import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.testing.FakeEmailSender; import com.google.gerrit.testing.GroupNoteDbMode; +import com.google.gerrit.testing.InMemoryDatabase; +import com.google.gerrit.testing.InMemoryRepositoryManager; import com.google.gerrit.testing.NoteDbChecker; import com.google.gerrit.testing.NoteDbMode; import com.google.gerrit.testing.SshMode; @@ -261,7 +265,7 @@ public class GerritServer implements AutoCloseable { if (!desc.memory()) { init(desc, baseConfig, site); } - return start(desc, baseConfig, site, null); + return start(desc, baseConfig, site, null, null, null); } catch (Exception e) { TempFileUtil.recursivelyDelete(site.toFile()); throw e; @@ -278,6 +282,10 @@ public class GerritServer implements AutoCloseable { * initialize this directory. Can be retrieved from the returned instance via {@link * #getSitePath()}. * @param testSysModule optional additional module to add to the system injector. + * @param inMemoryRepoManager {@link InMemoryRepositoryManager} that should be used if the site is + * started in memory + * @param inMemoryDatabaseInstance {@link com.google.gerrit.testing.InMemoryDatabase.Instance} + * that should be used if the site is started in memory * @param additionalArgs additional command-line arguments for the daemon program; only allowed if * the test is not in-memory. * @return started server. @@ -288,6 +296,8 @@ public class GerritServer implements AutoCloseable { Config baseConfig, Path site, @Nullable Module testSysModule, + @Nullable InMemoryRepositoryManager inMemoryRepoManager, + @Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance, String... additionalArgs) throws Exception { checkArgument(site != null, "site is required (even for in-memory server"); @@ -307,16 +317,24 @@ public class GerritServer implements AutoCloseable { daemon.setEmailModuleForTesting(new FakeEmailSender.Module()); daemon.setAdditionalSysModuleForTesting(testSysModule); daemon.setEnableSshd(desc.useSsh()); + daemon.setSlave(baseConfig.getBoolean("container", "slave", false)); if (desc.memory()) { checkArgument(additionalArgs.length == 0, "cannot pass args to in-memory server"); - return startInMemory(desc, site, baseConfig, daemon); + return startInMemory( + desc, site, baseConfig, daemon, inMemoryRepoManager, inMemoryDatabaseInstance); } return startOnDisk(desc, site, daemon, serverStarted, additionalArgs); } private static GerritServer startInMemory( - Description desc, Path site, Config baseConfig, Daemon daemon) throws Exception { + Description desc, + Path site, + Config baseConfig, + Daemon daemon, + @Nullable InMemoryRepositoryManager inMemoryRepoManager, + @Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance) + throws Exception { Config cfg = desc.buildConfig(baseConfig); mergeTestConfig(cfg); // Set the log4j configuration to an invalid one to prevent system logs @@ -329,7 +347,9 @@ public class GerritServer implements AutoCloseable { daemon.setEnableHttpd(desc.httpd()); daemon.setLuceneModule(LuceneIndexModule.singleVersionAllLatest(0)); daemon.setDatabaseForTesting( - ImmutableList.of(new InMemoryTestingDatabaseModule(cfg, site))); + ImmutableList.of( + new InMemoryTestingDatabaseModule( + cfg, site, inMemoryRepoManager, inMemoryDatabaseInstance))); daemon.start(); return new GerritServer(desc, null, createTestInjector(daemon), daemon, null); } @@ -483,6 +503,40 @@ public class GerritServer implements AutoCloseable { return desc; } + public static GerritServer restartAsSlave(GerritServer server) throws Exception { + checkState(server.desc.sandboxed(), "restarting as slave requires @Sandboxed"); + + Path site = server.testInjector.getInstance(Key.get(Path.class, SitePath.class)); + + Config cfg = server.testInjector.getInstance(Key.get(Config.class, GerritServerConfig.class)); + cfg.setBoolean("container", null, "slave", true); + + InMemoryRepositoryManager inMemoryRepoManager = null; + if (hasBinding(server.testInjector, InMemoryRepositoryManager.class)) { + inMemoryRepoManager = server.testInjector.getInstance(InMemoryRepositoryManager.class); + } + + InMemoryDatabase.Instance dbInstance = null; + if (hasBinding(server.testInjector, InMemoryDatabase.class)) { + InMemoryDatabase inMemoryDatabase = server.testInjector.getInstance(InMemoryDatabase.class); + dbInstance = inMemoryDatabase.getDbInstance(); + dbInstance.setKeepOpen(true); + } + try { + server.close(); + server.daemon.stop(); + return start(server.desc, cfg, site, null, inMemoryRepoManager, dbInstance); + } finally { + if (dbInstance != null) { + dbInstance.setKeepOpen(false); + } + } + } + + private static boolean hasBinding(Injector injector, Class clazz) { + return injector.getExistingBinding(Key.get(clazz)) != null; + } + @Override public void close() throws Exception { try { diff --git a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java index 4b1211b828..58dfa9445d 100644 --- a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java +++ b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance; import static com.google.inject.Scopes.SINGLETON; +import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.metrics.DisabledMetricMaker; @@ -48,6 +49,7 @@ import com.google.inject.Provides; import com.google.inject.ProvisionException; import com.google.inject.Singleton; import com.google.inject.TypeLiteral; +import com.google.inject.util.Providers; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -58,10 +60,18 @@ import org.eclipse.jgit.lib.Config; class InMemoryTestingDatabaseModule extends LifecycleModule { private final Config cfg; private final Path sitePath; + @Nullable private final InMemoryRepositoryManager repoManager; + @Nullable private final InMemoryDatabase.Instance inMemoryDatabaseInstance; - InMemoryTestingDatabaseModule(Config cfg, Path sitePath) { + InMemoryTestingDatabaseModule( + Config cfg, + Path sitePath, + @Nullable InMemoryRepositoryManager repoManager, + @Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance) { this.cfg = cfg; this.sitePath = sitePath; + this.repoManager = repoManager; + this.inMemoryDatabaseInstance = inMemoryDatabaseInstance; makeSiteDirs(sitePath); } @@ -72,8 +82,12 @@ class InMemoryTestingDatabaseModule extends LifecycleModule { // TODO(dborowitz): Use jimfs. bind(Path.class).annotatedWith(SitePath.class).toInstance(sitePath); - bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class); - bind(InMemoryRepositoryManager.class).in(SINGLETON); + if (repoManager != null) { + bind(GitRepositoryManager.class).toInstance(repoManager); + } else { + bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class); + bind(InMemoryRepositoryManager.class).in(SINGLETON); + } bind(MetricMaker.class).to(DisabledMetricMaker.class); bind(DataSourceType.class).to(InMemoryH2Type.class); @@ -83,6 +97,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule { TypeLiteral> schemaFactory = new TypeLiteral>() {}; bind(schemaFactory).to(NotesMigrationSchemaFactory.class); + bind(InMemoryDatabase.Instance.class).toProvider(Providers.of(inMemoryDatabaseInstance)); bind(Key.get(schemaFactory, ReviewDbFactory.class)).to(InMemoryDatabase.class); bind(InMemoryDatabase.class).in(SINGLETON); bind(ChangeBundleReader.class).to(GwtormChangeBundleReader.class); @@ -132,7 +147,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule { @Override public void stop() { - mem.drop(); + mem.getDbInstance().drop(); } } diff --git a/java/com/google/gerrit/acceptance/ProjectResetter.java b/java/com/google/gerrit/acceptance/ProjectResetter.java index 637fb2a84a..569a0c14b1 100644 --- a/java/com/google/gerrit/acceptance/ProjectResetter.java +++ b/java/com/google/gerrit/acceptance/ProjectResetter.java @@ -120,11 +120,12 @@ public class ProjectResetter implements AutoCloseable { } } - private final GitRepositoryManager repoManager; - private final AllUsersName allUsersName; - @Nullable private final AccountCreator accountCreator; - @Nullable private final AccountCache accountCache; - @Nullable private final ProjectCache projectCache; + @Inject private GitRepositoryManager repoManager; + @Inject private AllUsersName allUsersName; + @Inject @Nullable private AccountCreator accountCreator; + @Inject @Nullable private AccountCache accountCache; + @Inject @Nullable private ProjectCache projectCache; + private final Multimap refsPatternByProject; private final Multimap savedRefStatesByProject; diff --git a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java index 8790e78c4c..09ffe9d696 100644 --- a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java +++ b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java @@ -149,7 +149,7 @@ public abstract class StandaloneSiteTest { private GerritServer startImpl(@Nullable Module testSysModule, String... additionalArgs) throws Exception { return GerritServer.start( - serverDesc, baseConfig, sitePaths.site_path, testSysModule, additionalArgs); + serverDesc, baseConfig, sitePaths.site_path, testSysModule, null, null, additionalArgs); } protected static void runGerrit(String... args) throws Exception { diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 1c0ce9c297..3e7ea1b8be 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -222,6 +222,10 @@ public class Daemon extends SiteProgram { httpd = enable; } + public void setSlave(boolean slave) { + this.slave = slave; + } + @Override public int run() throws Exception { if (stopOnly) { diff --git a/java/com/google/gerrit/testing/InMemoryDatabase.java b/java/com/google/gerrit/testing/InMemoryDatabase.java index ec98e1670b..a3d7c17b86 100644 --- a/java/com/google/gerrit/testing/InMemoryDatabase.java +++ b/java/com/google/gerrit/testing/InMemoryDatabase.java @@ -55,26 +55,16 @@ public class InMemoryDatabase implements SchemaFactory { return injector.getInstance(InMemoryDatabase.class); } - private static int dbCnt; - - private static synchronized DataSource newDataSource() throws SQLException { - final Properties p = new Properties(); - p.setProperty("driver", org.h2.Driver.class.getName()); - p.setProperty("url", "jdbc:h2:mem:Test_" + (++dbCnt)); - return new SimpleDataSource(p); - } - /** Drop the database from memory; does nothing if the instance was null. */ public static void drop(InMemoryDatabase db) { if (db != null) { - db.drop(); + db.dbInstance.drop(); } } private final SchemaCreator schemaCreator; + private final Instance dbInstance; - private Connection openHandle; - private Database database; private boolean created; @Inject @@ -97,35 +87,26 @@ public class InMemoryDatabase implements SchemaFactory { } }); this.schemaCreator = childInjector.getInstance(SchemaCreator.class); - initDatabase(); + Instance dbInstanceFromInjector = childInjector.getInstance(Instance.class); + if (dbInstanceFromInjector != null) { + this.dbInstance = dbInstanceFromInjector; + this.created = true; + } else { + this.dbInstance = new Instance(); + } } InMemoryDatabase(SchemaCreator schemaCreator) throws OrmException { this.schemaCreator = schemaCreator; - initDatabase(); + this.dbInstance = new Instance(); } - private void initDatabase() throws OrmException { - try { - DataSource dataSource = newDataSource(); - - // Open one connection. This will peg the database into memory - // until someone calls drop on us, allowing subsequent connections - // opened against the same URL to go to the same set of tables. - // - openHandle = dataSource.getConnection(); - - // Build the access layer around the connection factory. - // - database = new Database<>(dataSource, ReviewDb.class); - - } catch (SQLException e) { - throw new OrmException(e); - } + public Instance getDbInstance() { + return dbInstance; } public Database getDatabase() { - return database; + return dbInstance.database; } @Override @@ -146,20 +127,6 @@ public class InMemoryDatabase implements SchemaFactory { return this; } - /** Drop this database from memory so it no longer exists. */ - public void drop() { - if (openHandle != null) { - try { - openHandle.close(); - } catch (SQLException e) { - System.err.println("WARNING: Cannot close database connection"); - e.printStackTrace(System.err); - } - openHandle = null; - database = null; - } - } - public SystemConfig getSystemConfig() throws OrmException { try (ReviewDb c = open()) { return c.systemConfig().get(new SystemConfig.Key()); @@ -175,4 +142,60 @@ public class InMemoryDatabase implements SchemaFactory { public void assertSchemaVersion() throws OrmException { assertThat(getSchemaVersion().versionNbr).isEqualTo(SchemaVersion.getBinaryVersion()); } + + public static class Instance { + private static int dbCnt; + + private Connection openHandle; + private Database database; + private boolean keepOpen; + + private static synchronized DataSource newDataSource() throws SQLException { + final Properties p = new Properties(); + p.setProperty("driver", org.h2.Driver.class.getName()); + p.setProperty("url", "jdbc:h2:mem:Test_" + (++dbCnt)); + return new SimpleDataSource(p); + } + + private Instance() throws OrmException { + try { + DataSource dataSource = newDataSource(); + + // Open one connection. This will peg the database into memory + // until someone calls drop on us, allowing subsequent connections + // opened against the same URL to go to the same set of tables. + // + openHandle = dataSource.getConnection(); + + // Build the access layer around the connection factory. + // + database = new Database<>(dataSource, ReviewDb.class); + + } catch (SQLException e) { + throw new OrmException(e); + } + } + + public void setKeepOpen(boolean keepOpen) { + this.keepOpen = keepOpen; + } + + /** Drop this database from memory so it no longer exists. */ + public void drop() { + if (keepOpen) { + return; + } + + if (openHandle != null) { + try { + openHandle.close(); + } catch (SQLException e) { + System.err.println("WARNING: Cannot close database connection"); + e.printStackTrace(System.err); + } + openHandle = null; + database = null; + } + } + } } diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java index e1c7b6b02c..ea42f47f67 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java +++ b/javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java @@ -22,9 +22,14 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.UseSsh; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.sshd.Commands; +import com.jcraft.jsch.JSchException; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import org.junit.Test; @@ -36,44 +41,59 @@ import org.slf4j.LoggerFactory; public class SshCommandsIT extends AbstractDaemonTest { private static final Logger log = LoggerFactory.getLogger(SshCommandsIT.class); - //TODO: It would be better to dynamically generate this list - private static final Map> COMMANDS = + // TODO: It would be better to dynamically generate these lists + private static final List COMMON_ROOT_COMMANDS = + ImmutableList.of( + "apropos", + "close-connection", + "flush-caches", + "gc", + "logging", + "ls-groups", + "ls-members", + "ls-projects", + "ls-user-refs", + "plugin", + "show-caches", + "show-connections", + "show-queue", + "version"); + + private static final List MASTER_ONLY_ROOT_COMMANDS = + ImmutableList.of( + "ban-commit", + "create-account", + "create-branch", + "create-group", + "create-project", + "gsql", + "index", + "query", + "receive-pack", + "rename-group", + "review", + "set-account", + "set-head", + "set-members", + "set-project", + "set-project-parent", + "set-reviewers", + "stream-events", + "test-submit"); + + private static final Map> MASTER_COMMANDS = ImmutableMap.of( Commands.ROOT, - ImmutableList.of( - "apropos", - "ban-commit", - "close-connection", - "create-account", - "create-branch", - "create-group", - "create-project", - "flush-caches", - "gc", - "gsql", - "index", - "logging", - "ls-groups", - "ls-members", - "ls-projects", - "ls-user-refs", - "plugin", - "query", - "receive-pack", - "rename-group", - "review", - "set-account", - "set-head", - "set-members", - "set-project", - "set-project-parent", - "set-reviewers", - "show-caches", - "show-connections", - "show-queue", - "stream-events", - "test-submit", - "version"), + ImmutableList.copyOf( + new ArrayList() { + private static final long serialVersionUID = 1L; + + { + addAll(COMMON_ROOT_COMMANDS); + addAll(MASTER_ONLY_ROOT_COMMANDS); + Collections.sort(this); + } + }), "index", ImmutableList.of("activate", "changes", "project", "start"), "plugin", @@ -81,13 +101,29 @@ public class SshCommandsIT extends AbstractDaemonTest { "test-submit", ImmutableList.of("rule", "type")); + private static final Map> SLAVE_COMMANDS = + ImmutableMap.of( + Commands.ROOT, + COMMON_ROOT_COMMANDS, + "plugin", + ImmutableList.of("add", "enable", "install", "ls", "reload", "remove", "rm")); + @Test + @Sandboxed public void sshCommandCanBeExecuted() throws Exception { // Access Database capability is required to run the "gerrit gsql" command allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); - for (String root : COMMANDS.keySet()) { - for (String command : COMMANDS.get(root)) { + testCommandExecution(MASTER_COMMANDS); + + restartAsSlave(); + testCommandExecution(SLAVE_COMMANDS); + } + + private void testCommandExecution(Map> commands) + throws JSchException, IOException { + for (String root : commands.keySet()) { + for (String command : commands.get(root)) { // We can't assert that adminSshSession.hasError() is false, because using the --help // option causes the usage info to be written to stderr. Instead, we assert on the // content of the stderr, which will always start with "gerrit command" when the --help @@ -109,4 +145,50 @@ public class SshCommandsIT extends AbstractDaemonTest { assertThat(adminSshSession.getError()) .startsWith("fatal: gerrit: non-existing-command: not found"); } + + @Test + @Sandboxed + public void listCommands() throws Exception { + adminSshSession.exec("gerrit --help"); + List commands = parseCommandsFromGerritHelpText(adminSshSession.getError()); + assertThat(commands).containsExactlyElementsIn(MASTER_COMMANDS.get(Commands.ROOT)).inOrder(); + + restartAsSlave(); + adminSshSession.exec("gerrit --help"); + commands = parseCommandsFromGerritHelpText(adminSshSession.getError()); + assertThat(commands).containsExactlyElementsIn(SLAVE_COMMANDS.get(Commands.ROOT)).inOrder(); + } + + private List parseCommandsFromGerritHelpText(String helpText) { + List commands = new ArrayList<>(); + + String[] lines = helpText.split("\\n"); + + // Skip all lines including the line starting with "Available commands" + int row = 0; + do { + row++; + } while (row < lines.length && !lines[row - 1].startsWith("Available commands")); + + // Skip all empty lines + while (lines[row].trim().isEmpty()) { + row++; + } + + // Parse commands from all lines that are indented (start with a space) + while (row < lines.length && lines[row].startsWith(" ")) { + String line = lines[row].trim(); + // Abort on empty line + if (line.isEmpty()) { + break; + } + + // Cut off command description if there is one + int endOfCommand = line.indexOf(' '); + commands.add(endOfCommand > 0 ? line.substring(0, line.indexOf(' ')) : line); + row++; + } + + return commands; + } }