Merge branch 'stable-3.1'
* stable-3.1:
Update git submodules
Update git submodules
gr-rest-api-interface.js: Remove unnecesary Promise resolution
LuceneQueryChangesTest: Remove unneeded override of visible()
e2e-tests: Rename the now reused json filename constant
e2e-tests: Refactor documentation about functional
RevisionActions: Do not alter server response
e2e-tests: Move core json files into scala package
e2e-tests: Support adding/running non-core scenarios
e2e-tests: Add create/delete project to CloneUsingBothProtocols
ChangeQueryBuilder: Throw error on ambiguous visibleto by display name
Change-Id: Idbb499461ecad11da49b3bcbe5d5bf14e90bfe31
diff --git a/Documentation/dev-e2e-tests.txt b/Documentation/dev-e2e-tests.txt
index 9a62f01..8887875 100644
--- a/Documentation/dev-e2e-tests.txt
+++ b/Documentation/dev-e2e-tests.txt
@@ -1,29 +1,38 @@
:linkattrs:
-= Gerrit Code Review - End to end load tests
+= Gerrit Code Review - End to end tests
-This document provides a description of a Gerrit load test scenario implemented using the
+This document provides descriptions of Gerrit end-to-end (`e2e`) test scenarios implemented using
link:https://gatling.io/[Gatling,role=external,window=_blank] framework.
Similar scenarios have been successfully used to compare performance of different Gerrit versions
-or study the Gerrit response under different load profiles.
+or study the Gerrit response under different load profiles. Although mostly for load, scenarios can
+either be for link:https://gatling.io/load-testing-continuous-integration/[load or functional,role=external,window=_blank]
+(e2e) testing purposes. Functional scenarios may then reuse this framework and Gatling's usability
+features such as its protocols (more below) and
+link:https://en.wikipedia.org/wiki/Domain-specific_language[DSL,role=external,window=_blank].
+
+That cross test-scope reusability applies to both Gerrit core scenarios and non-core ones, such as
+for Gerrit plugins or other potential extensions. End-to-end testing may then include scopes like
+feature integration, deployment, smoke (and load) testing. These load and functional test scopes
+should remain orthogonal to the unit and component (aka Gerrit `IT`-suffixed or `acceptance`) ones.
+The term `acceptance` though may still be coined by organizations to target e2e functional testing.
== What is Gatling?
-Gatling is a load testing tool which provides out of the box support for the HTTP protocol.
+Gatling is mostly a load testing tool which provides out of the box support for the HTTP protocol.
Documentation on how to write an HTTP load test can be found
link:https://gatling.io/docs/current/http/http_protocol/[here,role=external,window=_blank].
-
-However, in the scenario we are proposing, we are leveraging the
-link:https://github.com/GerritForge/gatling-git[Gatling Git extension,role=external,window=_blank]
-to run tests at Git protocol level.
+However, in the scenarios that were initially proposed, the
+link:https://github.com/GerritForge/gatling-git[Gatling Git extension,role=external,window=_blank] was
+leveraged to run tests at the Git protocol level.
Gatling is written in Scala, but the abstraction provided by the Gatling DSL makes the scenarios
implementation easy even without any Scala knowledge. The
link:https://gitenterprise.me/2019/12/20/stress-your-gerrit-with-gatling/[Stress your Gerrit with Gatling,role=external,window=_blank]
blog post has more introductory information.
-Examples of scenarios can be found in the `e2e-tests` directory. The files in that directory
-should be formatted using the mainstream
+Examples of scenarios can be found in the `e2e-tests` directory. The files in that directory should
+be formatted using the mainstream
link:https://plugins.jetbrains.com/plugin/1347-scala[Scala plugin for IntelliJ,role=external,window=_blank].
The latter is not mandatory but preferred for `sbt` and Scala IDE purposes in this project.
@@ -86,10 +95,11 @@
=== Input file
The `CloneUsingBothProtocols` scenario is fed with the data coming from the
-`src/test/resources/data/CloneUsingBothProtocols.json` file. Such a file contains the commands and
-repository used during the load test. That file currently looks like below. This scenario serves
-as a simple example with no actual load in it. It can be used to test or validate the local setup.
-More complex scenarios can be further developed, under the `com.google.gerrit.scenarios` package.
+`src/test/resources/data/com/google/gerrit/scenarios/CloneUsingBothProtocols.json` file. Such a
+file contains the commands and repository used during the e2e test. That file currently looks like
+below. This scenario serves as a simple example with no actual load in it. It can be used to test
+or validate the local setup. More complex scenarios can be further developed, under the
+`com.google.gerrit.scenarios` package.
----
[
@@ -111,9 +121,19 @@
* `pull`
* `push`
+=== Project and HTTP credentials
+
The example above assumes that the `loadtest-repo` project exists in the Gerrit under test. The
-`HTTP Credentials` or password obtained from test user's `Settings` (in Gerrit) may be required, in
-`src/test/resources/application.conf`, depending on the above commands used.
+`CloneUsingBothProtocols` scenario already includes creating that project and deleting it once done
+with it. That scenario class can be used as an example of how a scenario can compose itself
+alongside other scenarios (here, `CreateProject` and `DeleteProject`).
+
+The `HTTP Credentials` or password obtained from test user's `Settings` (in Gerrit) may be
+required, in `src/test/resources/application.conf`, depending on the above commands used. That
+file's `http` section shows which shell environment variables can be used to set those credentials.
+
+Executing the `CloneUsingBothProtocols` scenario, as is, does require setting the http credentials.
+That is because of the aforementioned create/delete project (http) scenarios composed within it.
== How to run tests
@@ -142,6 +162,27 @@
docker run -it e2e-tests -s com.google.gerrit.scenarios.CloneUsingBothProtocols
----
+=== How to run non-core scenarios
+
+Locally adding non-core scenarios, for example from Gerrit plugins, is as simple as copying such
+files in. Copying is necessary over linking, unless running using Docker (above) is not required.
+Docker does not support links for files it has to copy over through the Dockerfile (here, the
+scenario files). Here is how to proceed for adding such external (e.g., plugin) scenario files in:
+
+----
+pushd e2e-tests/src/test/scala
+cp -r (or, ln -s) scalaPackageStructure .
+popd
+
+pushd e2e-tests/src/test/resources/data
+cp -r (or, ln -s) jsonFilesPackageStructure .
+popd
+----
+
+The destination folders above readily git-ignore every non-core scenario file added under them. If
+running using Docker, `e2e-tests/Dockerfile` may require another `COPY` line for the hereby added
+scenarios. Aforementioned `sbt` or `docker` commands can then be used to run the added tests.
+
GERRIT
------
Part of link:index.html[Gerrit Code Review]
diff --git a/Documentation/dev-readme.txt b/Documentation/dev-readme.txt
index 5974aca..2748413 100644
--- a/Documentation/dev-readme.txt
+++ b/Documentation/dev-readme.txt
@@ -65,8 +65,8 @@
[[e2e]]
=== End-to-end tests
-<<dev-e2e-tests#,This document>> describes how load test scenarios are
-implemented using link:https://gatling.io/[`Gatling`].
+<<dev-e2e-tests#,This document>> describes how `e2e` (load or functional) test
+scenarios are implemented using link:https://gatling.io/[`Gatling`,role=external,window=_blank].
== Local server
diff --git a/e2e-tests/src/test/resources/data/.gitignore b/e2e-tests/src/test/resources/data/.gitignore
new file mode 100644
index 0000000..7354459
--- /dev/null
+++ b/e2e-tests/src/test/resources/data/.gitignore
@@ -0,0 +1,4 @@
+*
+!*/
+!/com/google/gerrit/scenarios/*
+!/.gitignore
diff --git a/e2e-tests/src/test/resources/data/CloneUsingBothProtocols.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CloneUsingBothProtocols.json
similarity index 100%
rename from e2e-tests/src/test/resources/data/CloneUsingBothProtocols.json
rename to e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CloneUsingBothProtocols.json
diff --git a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json
new file mode 100644
index 0000000..2e54de5
--- /dev/null
+++ b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateProject.json
@@ -0,0 +1,5 @@
+[
+ {
+ "url": "http://localhost:8080/a/projects/loadtest-repo"
+ }
+]
diff --git a/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/DeleteProject.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/DeleteProject.json
new file mode 100644
index 0000000..9312fb4
--- /dev/null
+++ b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/DeleteProject.json
@@ -0,0 +1,5 @@
+[
+ {
+ "url": "http://localhost:8080/a/projects/loadtest-repo/delete-project~delete"
+ }
+]
diff --git a/e2e-tests/src/test/resources/data/ReplayRecordsFromFeeder.json b/e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.json
similarity index 100%
rename from e2e-tests/src/test/resources/data/ReplayRecordsFromFeeder.json
rename to e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.json
diff --git a/e2e-tests/src/test/scala/.gitignore b/e2e-tests/src/test/scala/.gitignore
new file mode 100644
index 0000000..7354459
--- /dev/null
+++ b/e2e-tests/src/test/scala/.gitignore
@@ -0,0 +1,4 @@
+*
+!*/
+!/com/google/gerrit/scenarios/*
+!/.gitignore
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CloneUsingBothProtocols.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CloneUsingBothProtocols.scala
index c5a7cba..19fbf1b 100644
--- a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CloneUsingBothProtocols.scala
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CloneUsingBothProtocols.scala
@@ -15,18 +15,32 @@
package com.google.gerrit.scenarios
import io.gatling.core.Predef._
+import io.gatling.core.feeder.FileBasedFeederBuilder
import io.gatling.core.structure.ScenarioBuilder
import scala.concurrent.duration._
class CloneUsingBothProtocols extends GitSimulation {
+ private val data: FileBasedFeederBuilder[Any]#F = jsonFile(resource).queue
private val test: ScenarioBuilder = scenario(name)
.feed(data)
- .exec(request)
+ .exec(gitRequest)
+
+ private val createProject = new CreateProject
+ private val deleteProject = new DeleteProject
setUp(
+ createProject.test.inject(
+ atOnceUsers(1)
+ ),
test.inject(
+ nothingFor(1 second),
constantUsersPerSec(1) during (2 seconds)
- )).protocols(protocol)
+ ),
+ deleteProject.test.inject(
+ nothingFor(3 second),
+ atOnceUsers(1)
+ ),
+ ).protocols(gitProtocol, httpProtocol)
}
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala
new file mode 100644
index 0000000..58c8994
--- /dev/null
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateProject.scala
@@ -0,0 +1,32 @@
+// Copyright (C) 2020 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.scenarios
+
+import io.gatling.core.Predef._
+import io.gatling.core.feeder.FileBasedFeederBuilder
+import io.gatling.core.structure.ScenarioBuilder
+
+class CreateProject extends GerritSimulation {
+ private val data: FileBasedFeederBuilder[Any]#F = jsonFile(resource).queue
+
+ val test: ScenarioBuilder = scenario(name)
+ .feed(data)
+ .exec(httpRequest)
+
+ setUp(
+ test.inject(
+ atOnceUsers(1)
+ )).protocols(httpProtocol)
+}
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/DeleteProject.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/DeleteProject.scala
new file mode 100644
index 0000000..4b723cb
--- /dev/null
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/DeleteProject.scala
@@ -0,0 +1,32 @@
+// Copyright (C) 2020 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.scenarios
+
+import io.gatling.core.Predef._
+import io.gatling.core.feeder.FileBasedFeederBuilder
+import io.gatling.core.structure.ScenarioBuilder
+
+class DeleteProject extends GerritSimulation {
+ private val data: FileBasedFeederBuilder[Any]#F = jsonFile(resource).queue
+
+ val test: ScenarioBuilder = scenario(name)
+ .feed(data)
+ .exec(httpRequest)
+
+ setUp(
+ test.inject(
+ atOnceUsers(1)
+ )).protocols(httpProtocol)
+}
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala
new file mode 100644
index 0000000..b628bc7
--- /dev/null
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala
@@ -0,0 +1,34 @@
+// Copyright (C) 2020 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.scenarios
+
+import com.github.barbasa.gatling.git.GatlingGitConfiguration
+import io.gatling.core.Predef._
+import io.gatling.http.Predef.http
+import io.gatling.http.protocol.HttpProtocolBuilder
+import io.gatling.http.request.builder.HttpRequestBuilder
+
+class GerritSimulation extends Simulation {
+ implicit val conf: GatlingGitConfiguration = GatlingGitConfiguration()
+
+ private val path: String = this.getClass.getPackage.getName.replaceAllLiterally(".", "/")
+ protected val name: String = this.getClass.getSimpleName
+ protected val resource: String = s"data/$path/$name.json"
+
+ protected val httpRequest: HttpRequestBuilder = http(name).post("${url}")
+ protected val httpProtocol: HttpProtocolBuilder = http.basicAuth(
+ conf.httpConfiguration.userName,
+ conf.httpConfiguration.password)
+}
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GitSimulation.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GitSimulation.scala
index 4d5130f..e2f13a4 100644
--- a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GitSimulation.scala
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/GitSimulation.scala
@@ -16,23 +16,18 @@
import java.io.{File, IOException}
+import com.github.barbasa.gatling.git.GitRequestSession
import com.github.barbasa.gatling.git.protocol.GitProtocol
import com.github.barbasa.gatling.git.request.builder.GitRequestBuilder
-import com.github.barbasa.gatling.git.{GatlingGitConfiguration, GitRequestSession}
import io.gatling.core.Predef._
-import io.gatling.core.feeder.FileBasedFeederBuilder
import org.apache.commons.io.FileUtils
import org.eclipse.jgit.hooks.CommitMsgHook
-class GitSimulation extends Simulation {
-
- implicit val conf: GatlingGitConfiguration = GatlingGitConfiguration()
+class GitSimulation extends GerritSimulation {
implicit val postMessageHook: Option[String] = Some(s"hooks/${CommitMsgHook.NAME}")
- protected val name: String = this.getClass.getSimpleName
- protected val data: FileBasedFeederBuilder[Any]#F = jsonFile(s"data/$name.json").circular
- protected val request = new GitRequestBuilder(GitRequestSession("${cmd}", "${url}"))
- protected val protocol: GitProtocol = GitProtocol()
+ protected val gitRequest = new GitRequestBuilder(GitRequestSession("${cmd}", "${url}"))
+ protected val gitProtocol: GitProtocol = GitProtocol()
after {
Thread.sleep(5000)
diff --git a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.scala b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.scala
index 82342be..32df1b5 100644
--- a/e2e-tests/src/test/scala/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.scala
+++ b/e2e-tests/src/test/scala/com/google/gerrit/scenarios/ReplayRecordsFromFeeder.scala
@@ -15,16 +15,18 @@
package com.google.gerrit.scenarios
import io.gatling.core.Predef._
+import io.gatling.core.feeder.FileBasedFeederBuilder
import io.gatling.core.structure.ScenarioBuilder
import scala.concurrent.duration._
class ReplayRecordsFromFeeder extends GitSimulation {
+ private val data: FileBasedFeederBuilder[Any]#F = jsonFile(resource).circular
private val test: ScenarioBuilder = scenario(name)
.repeat(10000) {
feed(data)
- .exec(request)
+ .exec(gitRequest)
}
setUp(
@@ -34,6 +36,6 @@
rampUsers(10) during (5 seconds),
constantUsersPerSec(20) during (15 seconds),
constantUsersPerSec(20) during (15 seconds) randomized
- )).protocols(protocol)
+ )).protocols(gitProtocol)
.maxDuration(60 seconds)
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 43d7fc9..edd5411 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.query.change;
-import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.Change.CHANGE_ID_PATTERN;
import static com.google.gerrit.server.account.AccountResolver.isSelf;
import static com.google.gerrit.server.query.change.ChangeData.asChanges;
@@ -24,6 +23,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -985,18 +985,23 @@
if (isSelf(who)) {
return isVisible();
}
+ Set<Account.Id> accounts = null;
try {
- return Predicate.or(
- parseAccount(who).stream()
- .map(a -> visibleto(args.userFactory.create(a)))
- .collect(toImmutableList()));
+ accounts = parseAccount(who);
} catch (QueryParseException e) {
if (e instanceof QueryRequiresAuthException) {
throw e;
}
- // Otherwise continue: if it's not an account, maybe it's a group?
+ }
+ if (accounts != null) {
+ if (accounts.size() == 1) {
+ return visibleto(args.userFactory.create(Iterables.getOnlyElement(accounts)));
+ } else if (accounts.size() > 1) {
+ throw error(String.format("\"%s\" resolves to multiple accounts", who));
+ }
}
+ // If its not an account, maybe its a group?
Collection<GroupReference> suggestions = args.groupBackend.suggest(who, null);
if (!suggestions.isEmpty()) {
HashSet<AccountGroup.UUID> ids = new HashSet<>();
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index fb09c9f..040e2eb 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1784,16 +1784,43 @@
assertQuery(q + " visibleto:self", change2, change1);
// Second user cannot see first user's private change
- Account.Id user2 = createAccount("anotheruser");
+ Account.Id user2 = createAccount("user2");
assertQuery(q + " visibleto:" + user2.get(), change1);
- assertQuery(q + " visibleto:anotheruser", change1);
+ assertQuery(q + " visibleto:user2", change1);
String g1 = createGroup("group1", "Administrators");
- gApi.groups().id(g1).addMembers("anotheruser");
+ gApi.groups().id(g1).addMembers("user2");
assertQuery(q + " visibleto:" + g1, change1);
requestContext.setContext(newRequestContext(user2));
assertQuery("is:visible", change1);
+
+ Account.Id user3 = createAccount("user3");
+
+ // Explicitly authenticate user2 and user3 so that display name gets set
+ AuthRequest authRequest = AuthRequest.forUser("user2");
+ authRequest.setDisplayName("Another User");
+ authRequest.setEmailAddress("user2@example.com");
+ accountManager.authenticate(authRequest);
+ authRequest = AuthRequest.forUser("user3");
+ authRequest.setDisplayName("Another User");
+ authRequest.setEmailAddress("user3@example.com");
+ accountManager.authenticate(authRequest);
+
+ // Switch to user3
+ requestContext.setContext(newRequestContext(user3));
+ Change change3 = insert(repo, newChange(repo), user3);
+ Change change4 = insert(repo, newChangePrivate(repo), user3);
+
+ // User3 can see both their changes and the first user's change
+ assertQuery(q + " visibleto:" + user3.get(), change4, change3, change1);
+
+ // User2 cannot see user3's private change
+ assertQuery(q + " visibleto:" + user2.get(), change3, change1);
+
+ // Query as user3 by display name matching user2 and user3; bad request
+ assertFailingQuery(
+ q + " visibleto:\"Another User\"", "\"Another User\" resolves to multiple accounts");
}
@Test
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index 2b4e158..a3db19f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -496,9 +496,7 @@
}
_getRebaseAction(revisionActions) {
- return this._getRevisionAction(revisionActions, 'rebase',
- {rebaseOnCurrent: null}
- );
+ return this._getRevisionAction(revisionActions, 'rebase', null);
}
_getRevisionAction(revisionActions, actionName, emptyActionValue) {
@@ -523,7 +521,7 @@
.then(revisionActions => {
if (!revisionActions) { return; }
- this.revisionActions = this._updateRebaseAction(revisionActions);
+ this.revisionActions = revisionActions;
this._sendShowRevisionActions({
change: this.change,
revisionActions,
@@ -548,18 +546,6 @@
);
}
- _updateRebaseAction(revisionActions) {
- if (revisionActions && revisionActions.rebase) {
- revisionActions.rebase.rebaseOnCurrent =
- !!revisionActions.rebase.enabled;
- this._parentIsCurrent = !revisionActions.rebase.enabled;
- revisionActions.rebase.enabled = true;
- } else {
- this._parentIsCurrent = true;
- }
- return revisionActions;
- }
-
_changeChanged() {
this.reload();
}
@@ -1121,8 +1107,9 @@
}
_calculateDisabled(action, hasKnownChainState) {
- if (action.__key === 'rebase' && hasKnownChainState === false) {
- return true;
+ if (action.__key === 'rebase') {
+ // Rebase button is only disabled when change has no parent(s).
+ return hasKnownChainState === false;
}
return !action.enabled;
}
@@ -1602,6 +1589,13 @@
});
}
+ _computeRebaseOnCurrent(revisionRebaseAction) {
+ if (revisionRebaseAction) {
+ return !!revisionRebaseAction.enabled;
+ }
+ return null;
+ }
+
/**
* Occasionally, a change created by a change action is not yet knwon to the
* API for a brief time. Wait for the given change number to be recognized.
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.js
index 7aa5ecd..b66beed 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_html.js
@@ -101,7 +101,7 @@
</gr-dropdown>
</div>
<gr-overlay id="overlay" with-backdrop="">
- <gr-confirm-rebase-dialog id="confirmRebase" class="confirmDialog" change-number="[[change._number]]" on-confirm="_handleRebaseConfirm" on-cancel="_handleConfirmDialogCancel" branch="[[change.branch]]" has-parent="[[hasParent]]" rebase-on-current="[[_revisionRebaseAction.rebaseOnCurrent]]" hidden=""></gr-confirm-rebase-dialog>
+ <gr-confirm-rebase-dialog id="confirmRebase" class="confirmDialog" change-number="[[change._number]]" on-confirm="_handleRebaseConfirm" on-cancel="_handleConfirmDialogCancel" branch="[[change.branch]]" has-parent="[[hasParent]]" rebase-on-current="[[_computeRebaseOnCurrent(_revisionRebaseAction)]]" hidden=""></gr-confirm-rebase-dialog>
<gr-confirm-cherrypick-dialog id="confirmCherrypick" class="confirmDialog" change-status="[[changeStatus]]" commit-message="[[commitMessage]]" commit-num="[[commitNum]]" on-confirm="_handleCherrypickConfirm" on-cancel="_handleConfirmDialogCancel" project="[[change.project]]" hidden=""></gr-confirm-cherrypick-dialog>
<gr-confirm-cherrypick-conflict-dialog id="confirmCherrypickConflict" class="confirmDialog" on-confirm="_handleCherrypickConflictConfirm" on-cancel="_handleConfirmDialogCancel" hidden=""></gr-confirm-cherrypick-conflict-dialog>
<gr-confirm-move-dialog id="confirmMove" class="confirmDialog" on-confirm="_handleMoveConfirm" on-cancel="_handleConfirmDialogCancel" project="[[change.project]]" hidden=""></gr-confirm-move-dialog>
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 7a074a9..f8215f9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -393,7 +393,7 @@
action.enabled = false;
assert.equal(
- element._calculateDisabled(action, hasKnownChainState), true);
+ element._calculateDisabled(action, hasKnownChainState), false);
});
test('rebase change', done => {
@@ -416,7 +416,6 @@
};
assert.isTrue(fetchChangesStub.called);
element._handleRebaseConfirm({detail: {base: '1234'}});
- rebaseAction.rebaseOnCurrent = true;
assert.deepEqual(fireActionStub.lastCall.args,
['/rebase', rebaseAction, true, {base: '1234'}]);
done();
@@ -1924,57 +1923,23 @@
assert.strictEqual(element.$.confirmRebase.rebaseOnCurrent, null);
});
- test('_updateRebaseAction sets _parentIsCurrent on no rebase', () => {
- const currentRevisionActions = {
- cherrypick: {
- enabled: true,
- label: 'Cherry Pick',
- method: 'POST',
- title: 'cherrypick',
- },
+ test('_computeRebaseOnCurrent', () => {
+ const rebaseAction = {
+ enabled: true,
+ label: 'Rebase',
+ method: 'POST',
+ title: 'Rebase onto tip of branch or parent change',
};
- element._parentIsCurrent = undefined;
- element._updateRebaseAction(currentRevisionActions);
- assert.isTrue(element._parentIsCurrent);
- });
- test('_updateRebaseAction', () => {
- const currentRevisionActions = {
- cherrypick: {
- enabled: true,
- label: 'Cherry Pick',
- method: 'POST',
- title: 'cherrypick',
- },
- rebase: {
- enabled: true,
- label: 'Rebase',
- method: 'POST',
- title: 'Rebase onto tip of branch or parent change',
- },
- };
- element._parentIsCurrent = undefined;
-
- // Rebase enabled should always end up true.
// When rebase is enabled initially, rebaseOnCurrent should be set to
// true.
- assert.equal(element._updateRebaseAction(currentRevisionActions),
- currentRevisionActions);
+ assert.isTrue(element._computeRebaseOnCurrent(rebaseAction));
- assert.isTrue(currentRevisionActions.rebase.enabled);
- assert.isTrue(currentRevisionActions.rebase.rebaseOnCurrent);
- assert.isFalse(element._parentIsCurrent);
-
- delete currentRevisionActions.rebase.enabled;
+ delete rebaseAction.enabled;
// When rebase is not enabled initially, rebaseOnCurrent should be set to
// false.
- assert.equal(element._updateRebaseAction(currentRevisionActions),
- currentRevisionActions);
-
- assert.isTrue(currentRevisionActions.rebase.enabled);
- assert.isFalse(currentRevisionActions.rebase.rebaseOnCurrent);
- assert.isTrue(element._parentIsCurrent);
+ assert.isFalse(element._computeRebaseOnCurrent(rebaseAction));
});
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index b951237..d62ea04 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -292,7 +292,6 @@
_loading: Boolean,
/** @type {?} */
_projectConfig: Object,
- _rebaseOnCurrent: Boolean,
_replyButtonLabel: {
type: String,
value: 'Reply',
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index fea44b4..849be00 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1218,15 +1218,7 @@
patchNum,
reportEndpointAsIs: true,
};
- return this._getChangeURLAndFetch(req).then(revisionActions => {
- // The rebase button on change screen is always enabled.
- if (revisionActions.rebase) {
- revisionActions.rebase.rebaseOnCurrent =
- !!revisionActions.rebase.enabled;
- revisionActions.rebase.enabled = true;
- }
- return revisionActions;
- });
+ return this._getChangeURLAndFetch(req);
}
/**
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index 13ed562..72c875f 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -332,36 +332,6 @@
]);
});
- suite('rebase action', () => {
- let resolve_fetchJSON;
- setup(() => {
- sandbox.stub(element._restApiHelper, 'fetchJSON').returns(
- new Promise(resolve => {
- resolve_fetchJSON = resolve;
- }));
- });
-
- test('no rebase on current', done => {
- element.getChangeRevisionActions('42', '1337').then(
- response => {
- assert.isTrue(response.rebase.enabled);
- assert.isFalse(response.rebase.rebaseOnCurrent);
- done();
- });
- resolve_fetchJSON({rebase: {}});
- });
-
- test('rebase on current', done => {
- element.getChangeRevisionActions('42', '1337').then(
- response => {
- assert.isTrue(response.rebase.enabled);
- assert.isTrue(response.rebase.rebaseOnCurrent);
- done();
- });
- resolve_fetchJSON({rebase: {enabled: true}});
- });
- });
-
test('server error', done => {
const getResponseObjectStub = sandbox.stub(element, 'getResponseObject');
window.fetch.returns(Promise.resolve({ok: false}));
