Merge changes If7a8f1aa,Ida362917
* changes:
Implement List Pending Checks REST endpoint
Move relevant checker logic to "Checker"
diff --git a/java/com/google/gerrit/plugins/checks/Checker.java b/java/com/google/gerrit/plugins/checks/Checker.java
index 8bf6709..333e4fe 100644
--- a/java/com/google/gerrit/plugins/checks/Checker.java
+++ b/java/com/google/gerrit/plugins/checks/Checker.java
@@ -15,17 +15,35 @@
package com.google.gerrit.plugins.checks;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.index.query.IndexPredicate;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.plugins.checks.api.BlockingCondition;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeStatusPredicate;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.query.change.ProjectPredicate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryHelper.ActionType;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Provider;
import java.sql.Timestamp;
+import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
/** Definition of a checker. */
@AutoValue
public abstract class Checker {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
/**
* Returns the UUID of the checker.
@@ -126,6 +144,91 @@
return builder().setUuid(uuid);
}
+ public boolean isCheckerRelevant(ChangeData cd, ChangeQueryBuilder changeQueryBuilder)
+ throws OrmException {
+ if (!getQuery().isPresent()) {
+ return cd.change().isNew();
+ }
+
+ Predicate<ChangeData> predicate;
+ try {
+ predicate = createQueryPredicate(changeQueryBuilder);
+ } catch (ConfigInvalidException e) {
+ logger.atWarning().withCause(e).log("skipping invalid query for checker %s", getUuid());
+ return false;
+ }
+
+ return predicate.asMatchable().match(cd);
+ }
+
+ public List<ChangeData> queryMatchingChanges(
+ RetryHelper retryHelper,
+ ChangeQueryBuilder changeQueryBuilder,
+ Provider<InternalChangeQuery> changeQueryProvider)
+ throws ConfigInvalidException, OrmException {
+ return executeIndexQueryWithRetry(
+ retryHelper, changeQueryProvider, createQueryPredicate(changeQueryBuilder));
+ }
+
+ private Predicate<ChangeData> createQueryPredicate(ChangeQueryBuilder changeQueryBuilder)
+ throws ConfigInvalidException {
+ Predicate<ChangeData> predicate = new ProjectPredicate(getRepository().get());
+
+ if (getQuery().isPresent()) {
+ String query = getQuery().get();
+ Predicate<ChangeData> predicateForQuery;
+ try {
+ predicateForQuery = changeQueryBuilder.parse(query);
+ } catch (QueryParseException e) {
+ throw new ConfigInvalidException(
+ String.format("change query of checker %s is invalid: %s", getUuid(), query), e);
+ }
+
+ if (!predicateForQuery.isMatchable()) {
+ // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
+ // CheckerQuery should not be able to produce non-matchable queries.
+ throw new ConfigInvalidException(
+ String.format("change query of checker %s is non-matchable: %s", getUuid(), query));
+ }
+
+ predicate = Predicate.and(predicate, predicateForQuery);
+ }
+
+ if (!hasStatusPredicate(predicate)) {
+ predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
+ }
+
+ return predicate;
+ }
+
+ private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
+ if (predicate instanceof IndexPredicate) {
+ return ((IndexPredicate<ChangeData>) predicate)
+ .getField()
+ .getName()
+ .equals(ChangeField.STATUS.getName());
+ }
+ return predicate.getChildren().stream().anyMatch(Checker::hasStatusPredicate);
+ }
+
+ // TODO(ekempin): Retrying the query should be done by InternalChangeQuery.
+ private List<ChangeData> executeIndexQueryWithRetry(
+ RetryHelper retryHelper,
+ Provider<InternalChangeQuery> changeQueryProvider,
+ Predicate<ChangeData> predicate)
+ throws OrmException {
+ try {
+ return retryHelper.execute(
+ ActionType.INDEX_QUERY,
+ () -> changeQueryProvider.get().query(predicate),
+ OrmException.class::isInstance);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, OrmException.class);
+ throw new OrmException(e);
+ }
+ }
+
/** A builder for an {@link Checker}. */
@AutoValue.Builder
public abstract static class Builder {
diff --git a/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java b/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
index 9bc5bb6..51e26d5 100644
--- a/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/ListPendingChecks.java
@@ -14,23 +14,47 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.checks.AdministrateCheckersPermission;
+import com.google.gerrit.plugins.checks.Check;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checkers;
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.Checks.GetCheckOptions;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.kohsuke.args4j.Option;
public class ListPendingChecks implements RestReadView<TopLevelResource> {
private final PermissionBackend permissionBackend;
private final AdministrateCheckersPermission permission;
+ private final Checkers checkers;
+ private final Checks checks;
+ private final RetryHelper retryHelper;
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+ private final Provider<InternalChangeQuery> changeQueryProvider;
private CheckerUuid checkerUuid;
private List<CheckState> states = new ArrayList<>(CheckState.values().length);
@@ -51,14 +75,26 @@
@Inject
public ListPendingChecks(
- PermissionBackend permissionBackend, AdministrateCheckersPermission permission) {
+ PermissionBackend permissionBackend,
+ AdministrateCheckersPermission permission,
+ Checkers checkers,
+ Checks checks,
+ RetryHelper retryHelper,
+ Provider<ChangeQueryBuilder> queryBuilderProvider,
+ Provider<InternalChangeQuery> changeQueryProvider) {
this.permissionBackend = permissionBackend;
this.permission = permission;
+ this.checkers = checkers;
+ this.checks = checks;
+ this.retryHelper = retryHelper;
+ this.queryBuilderProvider = queryBuilderProvider;
+ this.changeQueryProvider = changeQueryProvider;
}
@Override
public List<PendingChecksInfo> apply(TopLevelResource resource)
- throws RestApiException, PermissionBackendException {
+ throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException,
+ OrmException {
permissionBackend.currentUser().check(permission);
if (states.isEmpty()) {
@@ -70,7 +106,67 @@
throw new BadRequestException("checker UUID is required");
}
- // TODO(ekempin): Implement this REST endpoint
- throw new MethodNotAllowedException("not implemented");
+ Checker checker =
+ checkers
+ .getChecker(checkerUuid)
+ .orElseThrow(
+ () ->
+ new UnprocessableEntityException(
+ String.format("checker %s not found", checkerUuid)));
+
+ if (checker.getStatus() == CheckerStatus.DISABLED) {
+ return ImmutableList.of();
+ }
+
+ // The query system can only match against the current patch set; ignore non-current patch sets
+ // for now.
+ List<ChangeData> changes =
+ checker.queryMatchingChanges(retryHelper, queryBuilderProvider.get(), changeQueryProvider);
+ List<PendingChecksInfo> pendingChecks = new ArrayList<>(changes.size());
+ for (ChangeData cd : changes) {
+ getPostFilteredPendingChecks(cd.project(), cd.currentPatchSet().getId())
+ .ifPresent(pendingChecks::add);
+ }
+ return pendingChecks;
+ }
+
+ private Optional<PendingChecksInfo> getPostFilteredPendingChecks(
+ Project.NameKey project, PatchSet.Id patchSetId) throws OrmException, IOException {
+ CheckState checkState = getCheckState(project, patchSetId);
+ if (!states.contains(checkState)) {
+ return Optional.empty();
+ }
+ return Optional.of(createPendingChecksInfo(project, patchSetId, checkerUuid, checkState));
+ }
+
+ private CheckState getCheckState(Project.NameKey project, PatchSet.Id patchSetId)
+ throws OrmException, IOException {
+ Optional<Check> check =
+ checks.getCheck(
+ CheckKey.create(project, patchSetId, checkerUuid), GetCheckOptions.defaults());
+
+ // Backfill if check is not present.
+ // Backfilling is only done for relevant checkers (checkers where the repository and the query
+ // matches the change). Since the change was found by executing the query of the checker we know
+ // that the checker is relevant for this patch set and hence backfilling should be done.
+ return check.map(Check::state).orElse(CheckState.NOT_STARTED);
+ }
+
+ private static PendingChecksInfo createPendingChecksInfo(
+ Project.NameKey project,
+ PatchSet.Id patchSetId,
+ CheckerUuid checkerUuid,
+ CheckState checkState) {
+ PendingChecksInfo pendingChecksInfo = new PendingChecksInfo();
+
+ pendingChecksInfo.patchSet = new CheckablePatchSetInfo();
+ pendingChecksInfo.patchSet.project = project.get();
+ pendingChecksInfo.patchSet.changeNumber = patchSetId.getParentKey().get();
+ pendingChecksInfo.patchSet.patchSetId = patchSetId.get();
+
+ pendingChecksInfo.pendingChecks =
+ ImmutableMap.of(checkerUuid.toString(), new PendingCheckInfo(checkState));
+
+ return pendingChecksInfo;
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java b/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
index 5e46bde..6ba6275 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingCheckInfo.java
@@ -14,6 +14,9 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.common.base.MoreObjects;
+import java.util.Objects;
+
/**
* REST API representation of a pending check.
*
@@ -24,4 +27,27 @@
public class PendingCheckInfo {
/** State of the check. */
public CheckState state;
+
+ public PendingCheckInfo(CheckState state) {
+ this.state = state;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(state);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (!(obj instanceof PendingCheckInfo)) {
+ return false;
+ }
+ PendingCheckInfo o = (PendingCheckInfo) obj;
+ return Objects.equals(state, o.state);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("state", state).toString();
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java b/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
index 01944b6..48311de 100644
--- a/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/PendingChecksInfo.java
@@ -14,7 +14,6 @@
package com.google.gerrit.plugins.checks.api;
-import com.google.gerrit.plugins.checks.CheckerUuid;
import java.util.Map;
/** REST API representation of pending checks on patch set. */
@@ -23,5 +22,5 @@
public CheckablePatchSetInfo patchSet;
/** Pending checks on the patch set by checker UUID. */
- public Map<CheckerUuid, PendingCheckInfo> pendingChecks;
+ public Map<String, PendingCheckInfo> pendingChecks;
}
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
index f8c18e4..33119e5 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
@@ -16,9 +16,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
@@ -28,12 +25,10 @@
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.AnonymousUser;
-import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.Factory;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
-import com.google.gerrit.server.query.change.ChangeStatusPredicate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -81,7 +76,7 @@
PatchSet ps = cd.patchSet(psId);
ChangeQueryBuilder queryBuilder = newQueryBuilder();
for (Checker checker : candidates) {
- if (matches(checker, cd, queryBuilder)) {
+ if (checker.isCheckerRelevant(cd, queryBuilder)) {
// Add synthetic check at the creation time of the patch set.
result.add(newBackfilledCheck(cd, ps, checker));
}
@@ -107,7 +102,7 @@
}
if (!checker.isPresent()
|| checker.get().getStatus() != CheckerStatus.ENABLED
- || !matches(checker.get(), cd, newQueryBuilder())) {
+ || !checker.get().isCheckerRelevant(cd, newQueryBuilder())) {
return Optional.empty();
}
return Optional.of(newBackfilledCheck(cd, cd.patchSet(psId), checker.get()));
@@ -124,41 +119,4 @@
private ChangeQueryBuilder newQueryBuilder() {
return queryBuilderProvider.get().asUser(anonymousUserProvider.get());
}
-
- private static boolean matches(Checker checker, ChangeData cd, ChangeQueryBuilder queryBuilder)
- throws OrmException {
- if (!checker.getQuery().isPresent()) {
- return cd.change().isNew();
- }
- String query = checker.getQuery().get();
- Predicate<ChangeData> predicate;
- try {
- predicate = queryBuilder.parse(query);
- } catch (QueryParseException e) {
- logger.atWarning().withCause(e).log(
- "skipping invalid query for checker %s: %s", checker.getUuid(), query);
- return false;
- }
- if (!predicate.isMatchable()) {
- // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
- // CheckerQuery should not be able to produce non-matchable queries.
- logger.atWarning().log(
- "skipping non-matchable query for checker %s: %s", checker.getUuid(), query);
- return false;
- }
- if (!hasStatusPredicate(predicate)) {
- predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
- }
- return predicate.asMatchable().match(cd);
- }
-
- private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
- if (predicate instanceof IndexPredicate) {
- return ((IndexPredicate<ChangeData>) predicate)
- .getField()
- .getName()
- .equals(ChangeField.STATUS.getName());
- }
- return predicate.getChildren().stream().anyMatch(CheckBackfiller::hasStatusPredicate);
- }
}
diff --git a/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java b/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java
new file mode 100644
index 0000000..72c6269
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/testing/PendingChecksInfoSubject.java
@@ -0,0 +1,67 @@
+// Copyright (C) 2019 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.plugins.checks.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.MapSubject;
+import com.google.common.truth.Subject;
+import com.google.gerrit.plugins.checks.api.CheckablePatchSetInfo;
+import com.google.gerrit.plugins.checks.api.PendingCheckInfo;
+import com.google.gerrit.plugins.checks.api.PendingChecksInfo;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import java.util.Map;
+
+public class PendingChecksInfoSubject extends Subject<PendingChecksInfoSubject, PendingChecksInfo> {
+ public static PendingChecksInfoSubject assertThat(PendingChecksInfo pendingChecksInfo) {
+ return assertAbout(PendingChecksInfoSubject::new).that(pendingChecksInfo);
+ }
+
+ private PendingChecksInfoSubject(FailureMetadata metadata, PendingChecksInfo actual) {
+ super(metadata, actual);
+ }
+
+ public void hasProject(Project.NameKey expectedProject) {
+ check("patchSet().project()").that(patchSet().project).isEqualTo(expectedProject.get());
+ }
+
+ public void hasPatchSet(PatchSet.Id expectedPatchSetId) {
+ CheckablePatchSetInfo patchSet = patchSet();
+ check("patchSet().changeNumber()")
+ .that(patchSet.changeNumber)
+ .isEqualTo(expectedPatchSetId.getParentKey().get());
+ check("patchSet().id()").that(patchSet.patchSetId).isEqualTo(expectedPatchSetId.get());
+ }
+
+ public MapSubject hasPendingChecksMapThat() {
+ return check("pendingChecks()").that(pendingChecks());
+ }
+
+ private CheckablePatchSetInfo patchSet() {
+ isNotNull();
+ CheckablePatchSetInfo patchSet = actual().patchSet;
+ check("patchSet()").that(patchSet).isNotNull();
+ return patchSet;
+ }
+
+ private Map<String, PendingCheckInfo> pendingChecks() {
+ isNotNull();
+ Map<String, PendingCheckInfo> pendingChecks = actual().pendingChecks;
+ check("pendingChecks()").that(pendingChecks).isNotNull();
+ return pendingChecks;
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
index 7db608a..b341d12 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/BUILD
@@ -13,5 +13,6 @@
"//plugins/checks:checks__plugin",
"//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
"//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance/testsuite",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/testing",
],
)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
index 2dd95f1..d567cd0 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListPendingChecksIT.java
@@ -15,19 +15,52 @@
package com.google.gerrit.plugins.checks.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.checks.testing.PendingChecksInfoSubject.assertThat;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.PendingCheckInfo;
+import com.google.gerrit.plugins.checks.api.PendingChecksInfo;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
public class ListPendingChecksIT extends AbstractCheckersTest {
@Inject private RequestScopeOperations requestScopeOperations;
+ private PatchSet.Id patchSetId;
+
+ @Before
+ public void setUp() throws Exception {
+ TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
+ TestTimeUtil.setClock(Timestamp.from(Instant.EPOCH));
+
+ patchSetId = createChange().getPatchSetId();
+ }
+
+ @After
+ public void resetTime() {
+ TestTimeUtil.useSystemTime();
+ }
+
@Test
public void specifyingCheckerUuidIsRequired() throws Exception {
// The extension API doesn't allow to not specify a checker UUID. Call the endpoint over REST to
@@ -54,9 +87,238 @@
}
@Test
- public void listPendingChecksForCheckerNotImplemented() throws Exception {
- exception.expect(MethodNotAllowedException.class);
- exception.expectMessage("not implemented");
- pendingChecksApi.list("foo:bar");
+ public void cannotListPendingChecksForNonExistingChecker() throws Exception {
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("checker non:existing not found");
+ pendingChecksApi.list("non:existing");
+ }
+
+ @Test
+ public void listPendingChecksNotStartedStateAssumed() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "NOT_STARTED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "FAILED" that we expect to be ignored.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList = pendingChecksApi.list(checkerUuid);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void listPendingChecksForSpecifiedState() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "FAILED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" that we expect to be ignored.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "FAILED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.FAILED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.FAILED);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.FAILED));
+ }
+
+ @Test
+ public void listPendingChecksForMultipleSpecifiedStates() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ // Create a check with state "NOT_STARTED" that we expect to be returned.
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ // Create a check with state "SCHEDULED" that we expect to be returned.
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.SCHEDULED)
+ .upsert();
+
+ // Create a check with state "SUCCESSFUL" that we expect to be ignored.
+ PatchSet.Id patchSetId3 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId3, checkerUuid))
+ .setState(CheckState.SUCCESSFUL)
+ .upsert();
+
+ // Create a check with state "NOT_STARTED" for other checker that we expect to be ignored.
+ CheckerUuid checkerUuid2 = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid2))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED, CheckState.SCHEDULED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ // The sorting of the pendingChecksList matches the sorting in which the matching changes are
+ // returned from the change index, which is by last updated timestamp. Use this knowledge here
+ // to do the assertions although the REST endpoint doesn't document a guaranteed sort order.
+ PendingChecksInfo pendingChecksChange2 = pendingChecksList.get(0);
+ assertThat(pendingChecksChange2).hasProject(project);
+ assertThat(pendingChecksChange2).hasPatchSet(patchSetId2);
+ assertThat(pendingChecksChange2)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.SCHEDULED));
+
+ PendingChecksInfo pendingChecksChange1 = pendingChecksList.get(1);
+ assertThat(pendingChecksChange1).hasProject(project);
+ assertThat(pendingChecksChange1).hasPatchSet(patchSetId);
+ assertThat(pendingChecksChange1)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void backfillForApplyingChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ List<PendingChecksInfo> pendingChecksList = pendingChecksApi.list(checkerUuid);
+ assertThat(pendingChecksList).hasSize(1);
+ PendingChecksInfo pendingChecks = Iterables.getOnlyElement(pendingChecksList);
+ assertThat(pendingChecks).hasProject(project);
+ assertThat(pendingChecks).hasPatchSet(patchSetId);
+ assertThat(pendingChecks)
+ .hasPendingChecksMapThat()
+ .containsExactly(checkerUuid.toString(), new PendingCheckInfo(CheckState.NOT_STARTED));
+ }
+
+ @Test
+ public void noBackfillForCheckerThatDoesNotApplyToTheProject() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(allProjects).create();
+ assertThat(pendingChecksApi.list(checkerUuid)).isEmpty();
+ }
+
+ @Test
+ public void noBackfillForCheckerThatDoesNotApplyToTheChange() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("message:not-matching").create();
+ assertThat(pendingChecksApi.list(checkerUuid)).isEmpty();
+ }
+
+ @Test
+ public void listPendingChecksForDisabledChecker() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isNotEmpty();
+
+ checkerOperations.checker(checkerUuid).forUpdate().disable().update();
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).isEmpty();
+ }
+
+ @Test
+ public void listPendingChecksFiltersOutChecksForClosedChangesIfQueryDoesntSpecifyStatus()
+ throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).query("").create();
+
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ gApi.changes().id(patchSetId2.getParentKey().toString()).abandon();
+
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(1);
+ }
+
+ @Test
+ public void listPendingChecksReturnsChecksForClosedChangesIfQuerySpecifiesStatus()
+ throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).query("is:open OR is:closed").create();
+
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ PatchSet.Id patchSetId2 = createChange().getPatchSetId();
+ checkOperations
+ .newCheck(CheckKey.create(project, patchSetId2, checkerUuid))
+ .setState(CheckState.NOT_STARTED)
+ .upsert();
+
+ List<PendingChecksInfo> pendingChecksList =
+ pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+
+ gApi.changes().id(patchSetId2.getParentKey().toString()).abandon();
+
+ pendingChecksList = pendingChecksApi.list(checkerUuid, CheckState.NOT_STARTED);
+ assertThat(pendingChecksList).hasSize(2);
+ }
+
+ @Test
+ public void listPendingChecksForInvalidCheckerFails() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidConfig().update();
+
+ exception.expect(RestApiException.class);
+ exception.expectMessage("Cannot list pending checks");
+ exception.expectCause(instanceOf(ConfigInvalidException.class));
+ pendingChecksApi.list(checkerUuid);
}
}
diff --git a/src/main/resources/Documentation/rest-api-pending-checks.md b/src/main/resources/Documentation/rest-api-pending-checks.md
index 62e64c8..2333afc 100644
--- a/src/main/resources/Documentation/rest-api-pending-checks.md
+++ b/src/main/resources/Documentation/rest-api-pending-checks.md
@@ -29,6 +29,8 @@
this option may be specified multiple times to request checks
matching any of several states)
+This REST endpoint only returns pending checks for current patch sets.
+
Note that only users with the [Administrate
Checkers](access-control.md#capability_administrateCheckers) global capability
are permitted to list pending checks.
