From f893c4f87ced3d1c53e7fbf944240c20cfae9d63 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 11 Mar 2015 10:35:31 -0700 Subject: [PATCH] Use TestProtocol to clone in-process for AbstractDaemonTest This new JGit feature associates a particular request object with a URL and processes all git protocol requests in-memory in a separate thread. For acceptance tests, this request object also describes a request scope context, such that each connection runs in its own connection thread with its own Guice request context. The TestProtocol instance uses custom Upload/ReceivePackFactory implementations that parallel the implementations within the HTTP and SSH code paths. On the one hand, this is yet another implementation that must be modified when changing the git filter stack, but on the other hand, there were already multiple stacks to begin with. In this case, I think the performance improvement is also worth the cost. With this change, single acceptance test methods can run in about 100ms even on my Macbook Air. Startup cost is still relatively high due to server initialization, but this change opens the door to reducing that well by removing HTTP and SSH servers from tests entirely. Change-Id: Icddd7d2ac448ea3f59909f8ef61f95feb1d67b64 --- gerrit-acceptance-tests/BUCK | 1 + .../gerrit/acceptance/AbstractDaemonTest.java | 40 +- .../gerrit/acceptance/AccountCreator.java | 8 + .../gerrit/acceptance/GerritServer.java | 7 + .../gerrit/acceptance/InProcessProtocol.java | 352 ++++++++++++++++++ .../gerrit/acceptance/TestProjectInput.java | 8 + .../gerrit/acceptance/edit/ChangeEditIT.java | 3 +- .../acceptance/git/AbstractPushForReview.java | 5 +- .../acceptance/rest/change/ChangeOwnerIT.java | 13 +- .../rest/project/ProjectLevelConfigIT.java | 3 +- .../acceptance/ssh/JschVerifyFalseBugIT.java | 4 +- 11 files changed, 425 insertions(+), 19 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java diff --git a/gerrit-acceptance-tests/BUCK b/gerrit-acceptance-tests/BUCK index d328071207..b6a7b29510 100644 --- a/gerrit-acceptance-tests/BUCK +++ b/gerrit-acceptance-tests/BUCK @@ -35,6 +35,7 @@ java_library( '//lib/log:log4j', '//lib/guice:guice', '//lib/guice:guice-assistedinject', + '//lib/guice:guice-servlet', '//lib/jgit:jgit', '//lib/jgit:junit', '//lib/mina:sshd', diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index e6a4017bba..8741d30ee1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -14,7 +14,6 @@ package com.google.gerrit.acceptance; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; import static com.google.gerrit.acceptance.GitUtil.initSsh; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.server.project.Util.block; @@ -66,6 +65,8 @@ 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.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.Transport; import org.junit.AfterClass; import org.junit.Rule; import org.junit.rules.TestRule; @@ -74,6 +75,7 @@ import org.junit.runner.RunWith; import org.junit.runners.model.Statement; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -132,6 +134,9 @@ public abstract class AbstractDaemonTest { @Inject protected @GerritServerConfig Config cfg; + @Inject + private InProcessProtocol inProcessProtocol; + protected TestRepository testRepo; protected GerritServer server; protected TestAccount admin; @@ -143,6 +148,7 @@ public abstract class AbstractDaemonTest { protected Project.NameKey project; private String resourcePrefix; + private List toClose; @Rule public TestRule testRunner = new TestRule() { @@ -207,6 +213,8 @@ public abstract class AbstractDaemonTest { } server.getTestInjector().injectMembers(this); + Transport.register(inProcessProtocol); + toClose = Collections.synchronizedList(new ArrayList()); admin = accounts.admin(); user = accounts.user(); adminSession = new RestSession(server, admin); @@ -222,7 +230,12 @@ public abstract class AbstractDaemonTest { + description.getMethodName() + "_").replaceAll(""); project = createProject(projectInput(description)); - testRepo = cloneProject(project, sshSession); + testRepo = cloneProject(project, getCloneAsAccount(description)); + } + + private TestAccount getCloneAsAccount(Description description) { + TestProjectInput ann = description.getAnnotation(TestProjectInput.class); + return accounts.get(ann != null ? ann.cloneAs() : "admin"); } private ProjectInput projectInput(Description description) { @@ -306,7 +319,26 @@ public abstract class AbstractDaemonTest { // Default implementation does nothing. } + protected TestRepository cloneProject(Project.NameKey p) + throws Exception { + return cloneProject(p, admin); + } + + protected TestRepository cloneProject(Project.NameKey p, + TestAccount testAccount) throws Exception { + InProcessProtocol.Context ctx = new InProcessProtocol.Context( + reviewDbProvider, identifiedUserFactory, testAccount.getId(), p); + Repository repo = repoManager.openRepository(p); + toClose.add(repo); + return GitUtil.cloneProject( + p, inProcessProtocol.register(ctx, repo).toString()); + } + private void afterTest() throws Exception { + Transport.unregister(inProcessProtocol); + for (Repository repo : toClose) { + repo.close(); + } db.close(); sshSession.close(); if (server != commonServer) { @@ -334,7 +366,9 @@ public abstract class AbstractDaemonTest { protected PushOneCommit.Result createChange() throws Exception { PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo); - return push.to("refs/for/master"); + PushOneCommit.Result result = push.to("refs/for/master"); + result.assertOkStatus(); + return result; } private static final List RANDOM = diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AccountCreator.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AccountCreator.java index 51c8e3df43..f2474633e8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AccountCreator.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AccountCreator.java @@ -14,6 +14,8 @@ package com.google.gerrit.acceptance; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountExternalId; @@ -150,6 +152,12 @@ public class AccountCreator { return create("user2", "user2@example.com", "User2"); } + public TestAccount get(String username) { + return checkNotNull( + accounts.get(username), + "No TestAccount created for %s", username); + } + private AccountExternalId.Key getEmailKey(String email) { return new AccountExternalId.Key(AccountExternalId.SCHEME_MAILTO, email); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java index 70bcae8f21..804bac0bc2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java @@ -23,7 +23,10 @@ import com.google.gerrit.pgm.Daemon; import com.google.gerrit.pgm.Init; import com.google.gerrit.server.config.FactoryModule; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.AsyncReceiveCommits; +import com.google.gerrit.server.git.SubmoduleOp; import com.google.gerrit.server.index.ChangeSchemas; +import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.server.util.SocketUtil; import com.google.gerrit.testutil.TempFileUtil; import com.google.inject.Injector; @@ -198,6 +201,10 @@ public class GerritServer { protected void configure() { bind(AccountCreator.class); factory(PushOneCommit.Factory.class); + factory(SubmoduleOp.Factory.class); + install(InProcessProtocol.module()); + install(new NoSshModule()); + install(new AsyncReceiveCommits.Module()); } }; return sysInjector.createChildInjector(module); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java new file mode 100644 index 0000000000..c02b9e53ae --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java @@ -0,0 +1,352 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance; + +import com.google.common.collect.Lists; +import com.google.gerrit.acceptance.InProcessProtocol.Context; +import com.google.gerrit.common.data.Capable; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.AccessPath; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.RemotePeer; +import com.google.gerrit.server.RequestCleanup; +import com.google.gerrit.server.config.GerritRequestModule; +import com.google.gerrit.server.config.RequestScopedReviewDbProvider; +import com.google.gerrit.server.git.AsyncReceiveCommits; +import com.google.gerrit.server.git.ChangeCache; +import com.google.gerrit.server.git.ReceiveCommits; +import com.google.gerrit.server.git.ReceivePackInitializer; +import com.google.gerrit.server.git.TagCache; +import com.google.gerrit.server.git.TransferConfig; +import com.google.gerrit.server.git.VisibleRefFilter; +import com.google.gerrit.server.git.validators.UploadValidators; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.ProjectControl; +import com.google.gerrit.server.util.RequestContext; +import com.google.gerrit.server.util.RequestScopePropagator; +import com.google.gerrit.server.util.ThreadLocalRequestContext; +import com.google.gerrit.server.util.ThreadLocalRequestScopePropagator; +import com.google.gwtorm.server.SchemaFactory; +import com.google.inject.AbstractModule; +import com.google.inject.Inject; +import com.google.inject.Key; +import com.google.inject.Module; +import com.google.inject.OutOfScopeException; +import com.google.inject.Provider; +import com.google.inject.Provides; +import com.google.inject.Scope; +import com.google.inject.servlet.RequestScoped; +import com.google.inject.util.Providers; + +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.PostReceiveHook; +import org.eclipse.jgit.transport.PostReceiveHookChain; +import org.eclipse.jgit.transport.PreUploadHook; +import org.eclipse.jgit.transport.PreUploadHookChain; +import org.eclipse.jgit.transport.ReceivePack; +import org.eclipse.jgit.transport.TestProtocol; +import org.eclipse.jgit.transport.UploadPack; +import org.eclipse.jgit.transport.resolver.ReceivePackFactory; +import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; +import org.eclipse.jgit.transport.resolver.UploadPackFactory; + +import java.io.IOException; +import java.net.SocketAddress; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +class InProcessProtocol extends TestProtocol { + static Module module() { + return new AbstractModule() { + @Override + public void configure() { + install(new GerritRequestModule()); + bind(RequestScopePropagator.class).to(Propagator.class); + bindScope(RequestScoped.class, InProcessProtocol.REQUEST); + } + + @Provides + @RemotePeer + SocketAddress getSocketAddress() { + // TODO(dborowitz): Could potentially fake this with thread ID or + // something. + throw new OutOfScopeException("No remote peer in acceptance tests"); + } + }; + } + + private static final Scope REQUEST = new Scope() { + @Override + public Provider scope(final Key key, final Provider creator) { + return new Provider() { + @Override + public T get() { + Context ctx = current.get(); + if (ctx == null) { + throw new OutOfScopeException("Not in TestProtocol scope"); + } + return ctx.get(key, creator); + } + + @Override + public String toString() { + return String.format("%s[%s]", creator, REQUEST); + } + }; + } + + @Override + public String toString() { + return "InProcessProtocol.REQUEST"; + } + }; + + private static class Propagator + extends ThreadLocalRequestScopePropagator { + @Inject + Propagator(ThreadLocalRequestContext local, + Provider dbProviderProvider) { + super(REQUEST, current, local, dbProviderProvider); + } + + @Override + protected Context continuingContext(Context ctx) { + return ctx.newContinuingContext(); + } + } + + private static final ThreadLocal current = new ThreadLocal<>(); + + // TODO(dborowitz): Merge this with AcceptanceTestRequestScope. + /** + * Multi-purpose session/context object. + *

+ * Confusingly, Gerrit has two ideas of what a "context" object is: + * one for Guice {@link RequestScoped}, and one for its own simplified + * version of request scoping using {@link ThreadLocalRequestContext}. + * This class provides both, in essence just delegating the {@code + * ThreadLocalRequestContext} scoping to the Guice scoping mechanism. + *

+ * It is also used as the session type for {@code UploadPackFactory} and + * {@code ReceivePackFactory}, since, after all, it encapsulates all the + * information about a single request. + */ + static class Context implements RequestContext { + private static final Key DB_KEY = + Key.get(RequestScopedReviewDbProvider.class); + private static final Key RC_KEY = + Key.get(RequestCleanup.class); + private static final Key USER_KEY = Key.get(CurrentUser.class); + + private final SchemaFactory schemaFactory; + private final IdentifiedUser.GenericFactory userFactory; + private final Account.Id accountId; + private final Project.NameKey project; + private final RequestCleanup cleanup; + private final Map, Object> map; + + Context(SchemaFactory schemaFactory, + IdentifiedUser.GenericFactory userFactory, + Account.Id accountId, + Project.NameKey project) { + this.schemaFactory = schemaFactory; + this.userFactory = userFactory; + this.accountId = accountId; + this.project = project; + map = new HashMap<>(); + cleanup = new RequestCleanup(); + map.put(DB_KEY, + new RequestScopedReviewDbProvider( + schemaFactory, Providers.of(cleanup))); + map.put(RC_KEY, cleanup); + + IdentifiedUser user = userFactory.create(accountId); + user.setAccessPath(AccessPath.GIT); + map.put(USER_KEY, user); + } + + private Context newContinuingContext() { + return new Context(schemaFactory, userFactory, accountId, project); + } + + @Override + public CurrentUser getCurrentUser() { + return get(USER_KEY, null); + } + + @Override + public Provider getReviewDbProvider() { + return get(DB_KEY, null); + } + + private synchronized T get(Key key, Provider creator) { + @SuppressWarnings("unchecked") + T t = (T) map.get(key); + if (t == null) { + t = creator.get(); + map.put(key, t); + } + return t; + } + } + + private static class Upload implements UploadPackFactory { + private final Provider dbProvider; + private final Provider userProvider; + private final TagCache tagCache; + private final ChangeCache changeCache; + private final ProjectControl.GenericFactory projectControlFactory; + private final TransferConfig transferConfig; + private final DynamicSet preUploadHooks; + private final UploadValidators.Factory uploadValidatorsFactory; + private final ThreadLocalRequestContext threadContext; + + @Inject + Upload( + Provider dbProvider, + Provider userProvider, + TagCache tagCache, + ChangeCache changeCache, + ProjectControl.GenericFactory projectControlFactory, + TransferConfig transferConfig, + DynamicSet preUploadHooks, + UploadValidators.Factory uploadValidatorsFactory, + ThreadLocalRequestContext threadContext) { + this.dbProvider = dbProvider; + this.userProvider = userProvider; + this.tagCache = tagCache; + this.changeCache = changeCache; + this.projectControlFactory = projectControlFactory; + this.transferConfig = transferConfig; + this.preUploadHooks = preUploadHooks; + this.uploadValidatorsFactory = uploadValidatorsFactory; + this.threadContext = threadContext; + } + + @Override + public UploadPack create(Context req, final Repository repo) + throws ServiceNotAuthorizedException { + // Set the request context, but don't bother unsetting, since we don't + // have an easy way to run code when this instance is done being used. + // Each operation is run in its own thread, so we don't need to recover + // its original context anyway. + threadContext.setContext(req); + current.set(req); + try { + ProjectControl ctl = projectControlFactory.controlFor( + req.project, userProvider.get()); + if (!ctl.canRunUploadPack()) { + throw new ServiceNotAuthorizedException(); + } + + UploadPack up = new UploadPack(repo); + up.setPackConfig(transferConfig.getPackConfig()); + up.setTimeout(transferConfig.getTimeout()); + + if (!ctl.allRefsAreVisible()) { + up.setAdvertiseRefsHook(new VisibleRefFilter( + tagCache, changeCache, repo, ctl, dbProvider.get(), true)); + } + List hooks = Lists.newArrayList(preUploadHooks); + hooks.add(uploadValidatorsFactory.create( + ctl.getProject(), repo, "localhost-test")); + up.setPreUploadHook(PreUploadHookChain.newChain(hooks)); + return up; + } catch (NoSuchProjectException | IOException e) { + throw new RuntimeException(e); + } + } + } + + private static class Receive implements ReceivePackFactory { + private final Provider userProvider; + private final ProjectControl.GenericFactory projectControlFactory; + private final AsyncReceiveCommits.Factory factory; + private final TransferConfig config; + private final DynamicSet receivePackInitializers; + private final DynamicSet postReceiveHooks; + private final ThreadLocalRequestContext threadContext; + + @Inject + Receive( + Provider userProvider, + ProjectControl.GenericFactory projectControlFactory, + AsyncReceiveCommits.Factory factory, + TransferConfig config, + DynamicSet receivePackInitializers, + DynamicSet postReceiveHooks, + ThreadLocalRequestContext threadContext) { + this.userProvider = userProvider; + this.projectControlFactory = projectControlFactory; + this.factory = factory; + this.config = config; + this.receivePackInitializers = receivePackInitializers; + this.postReceiveHooks = postReceiveHooks; + this.threadContext = threadContext; + } + + @Override + public ReceivePack create(final Context req, Repository db) + throws ServiceNotAuthorizedException { + // Set the request context, but don't bother unsetting, since we don't + // have an easy way to run code when this instance is done being used. + // Each operation is run in its own thread, so we don't need to recover + // its original context anyway. + threadContext.setContext(req); + current.set(req); + try { + ProjectControl ctl = + projectControlFactory.controlFor(req.project, userProvider.get()); + if (!ctl.canRunReceivePack()) { + throw new ServiceNotAuthorizedException(); + } + + ReceiveCommits rc = factory.create(ctl, db).getReceiveCommits(); + ReceivePack rp = rc.getReceivePack(); + + Capable r = rc.canUpload(); + if (r != Capable.OK) { + throw new ServiceNotAuthorizedException(); + } + + IdentifiedUser user = (IdentifiedUser) ctl.getCurrentUser(); + rp.setRefLogIdent(user.newRefLogIdent()); + rp.setTimeout(config.getTimeout()); + rp.setMaxObjectSizeLimit(config.getMaxObjectSizeLimit()); + + for (ReceivePackInitializer initializer : receivePackInitializers) { + initializer.init(ctl.getProject().getNameKey(), rp); + } + + rp.setPostReceiveHook(PostReceiveHookChain.newChain( + Lists.newArrayList(postReceiveHooks))); + return rp; + } catch (NoSuchProjectException | IOException e) { + throw new RuntimeException(e); + } + } + } + + @Inject + InProcessProtocol(Upload uploadPackFactory, + Receive receivePackFactory) { + super(uploadPackFactory, receivePackFactory); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestProjectInput.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestProjectInput.java index 0a11419475..4ad37e2b74 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestProjectInput.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestProjectInput.java @@ -26,6 +26,8 @@ import java.lang.annotation.Target; @Target({METHOD}) @Retention(RUNTIME) public @interface TestProjectInput { + // Fields from ProjectInput for creating the project. + String parent() default ""; boolean createEmptyCommit() default true; String description() default ""; @@ -38,4 +40,10 @@ public @interface TestProjectInput { InheritableBoolean useSignedOffBy() default InheritableBoolean.INHERIT; InheritableBoolean useContentMerge() default InheritableBoolean.INHERIT; InheritableBoolean requireChangeId() default InheritableBoolean.INHERIT; + + + // Fields specific to acceptance test behavior. + + /** Username to use for initial clone, passed to {@link AccountCreator}. */ + String cloneAs() default "admin"; } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java index 1cf5ad2d68..016a8beb8a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java @@ -16,7 +16,6 @@ package com.google.gerrit.acceptance.edit; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; @@ -270,7 +269,7 @@ public class ChangeEditIT extends AbstractDaemonTest { @TestProjectInput(createEmptyCommit = false) public void updateRootCommitMessage() throws Exception { // Re-clone empty repo; TestRepository doesn't let us reset to unborn head. - testRepo = cloneProject(project, sshSession); + testRepo = cloneProject(project); changeId = newChange(admin.getIdent()); change = getChange(changeId); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 8a35567034..880ad29583 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -16,10 +16,10 @@ package com.google.gerrit.acceptance.git; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; import com.google.common.collect.ImmutableSet; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.extensions.common.ChangeInfo; @@ -46,6 +46,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { private NotesMigration notesMigration; protected enum Protocol { + // TODO(dborowitz): TEST. SSH, HTTP } @@ -68,7 +69,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { default: throw new IllegalArgumentException("unexpected protocol: " + p); } - testRepo = cloneProject(project, url + "/" + project.get()); + testRepo = GitUtil.cloneProject(project, url + "/" + project.get()); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java index 95c70ad582..c6bb4303a1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeOwnerIT.java @@ -15,16 +15,14 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; -import static com.google.gerrit.acceptance.GitUtil.initSsh; import static com.google.gerrit.common.data.Permission.LABEL; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.RestSession; -import com.google.gerrit.acceptance.SshSession; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; @@ -49,28 +47,27 @@ public class ChangeOwnerIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { + setApiUser(user); sessionOwner = new RestSession(server, user); - SshSession sshSession = new SshSession(server, user); - initSsh(user); - sshSession.open(); - testRepo = cloneProject(project, sshSession); - sshSession.close(); user2 = accounts.user2(); sessionDev = new RestSession(server, user2); } @Test + @TestProjectInput(cloneAs = "user") public void testChangeOwner_OwnerACLNotGranted() throws Exception { approve(sessionOwner, createMyChange(), HttpStatus.SC_FORBIDDEN); } @Test + @TestProjectInput(cloneAs = "user") public void testChangeOwner_OwnerACLGranted() throws Exception { grantApproveToChangeOwner(); approve(sessionOwner, createMyChange(), HttpStatus.SC_OK); } @Test + @TestProjectInput(cloneAs = "user") public void testChangeOwner_NotOwnerACLGranted() throws Exception { grantApproveToChangeOwner(); approve(sessionDev, createMyChange(), HttpStatus.SC_FORBIDDEN); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java index 7142ee2838..649534b778 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java @@ -15,7 +15,6 @@ package com.google.gerrit.acceptance.rest.project; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; import static com.google.gerrit.acceptance.GitUtil.fetch; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -75,7 +74,7 @@ public class ProjectLevelConfigIT extends AbstractDaemonTest { .assertOkStatus(); Project.NameKey childProject = createProject("child", project); - TestRepository childTestRepo = cloneProject(childProject, sshSession); + TestRepository childTestRepo = cloneProject(childProject); fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config"); childTestRepo.reset("refs/heads/config"); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java index 4f8a334972..ae0c8c18b3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/JschVerifyFalseBugIT.java @@ -15,9 +15,9 @@ package com.google.gerrit.acceptance.ssh; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.acceptance.GitUtil.cloneProject; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.reviewdb.client.Project; @@ -51,7 +51,7 @@ public class JschVerifyFalseBugIT extends AbstractDaemonTest { for (int i = 1; i < 100; i++) { String p = "p" + i; createProject(p); - cloneProject(new Project.NameKey(p), sshSession); + GitUtil.cloneProject(new Project.NameKey(p), sshSession); } return null; }