Add #getCombinedCheckState to the Check interface
This method is useful for callers who want to know the
combined check state of a patch set. For example,
in the follow-up change, the "ChecksSubmitRule" would
simply call this API to get the combined check state
of the latest patch set and then use that state to
decide whether a change is submittable or not.
Basic tests are added to verify the bahavior of this
API, more specific behaviors are covered by existing
"CombinedCheckStateTest".
Change-Id: I341dfdab8beaccd28d567d31196ab9a542661d24
diff --git a/java/com/google/gerrit/plugins/checks/Checker.java b/java/com/google/gerrit/plugins/checks/Checker.java
index 333e4fe..5bae4c7 100644
--- a/java/com/google/gerrit/plugins/checks/Checker.java
+++ b/java/com/google/gerrit/plugins/checks/Checker.java
@@ -16,6 +16,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.index.query.IndexPredicate;
@@ -229,6 +230,24 @@
}
}
+ /**
+ * Checks whether a {@link Checker} is required for submission or not.
+ *
+ * @return true if the {@link Checker} required for submission.
+ */
+ public boolean isRequired() {
+ ImmutableSet<BlockingCondition> blockingConditions = getBlockingConditions();
+ if (blockingConditions.isEmpty()) {
+ return false;
+ } else if (blockingConditions.size() > 1
+ || !blockingConditions.contains(BlockingCondition.STATE_NOT_PASSING)) {
+ // When a new blocking condition is introduced, this needs to be adjusted to respect that.
+ String errorMessage = String.format("illegal blocking conditions %s", blockingConditions);
+ throw new IllegalStateException(errorMessage);
+ }
+ return true;
+ }
+
/** A builder for an {@link Checker}. */
@AutoValue.Builder
public abstract static class Builder {
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index 768908b..7d6cd03 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -16,6 +16,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gwtorm.server.OrmException;
@@ -61,6 +62,18 @@
Optional<Check> getCheck(CheckKey checkKey, GetCheckOptions options)
throws OrmException, IOException;
+ /**
+ * Returns the combined check state of a given patch set.
+ *
+ * @param projectName the name of the project.
+ * @param patchSetId the ID of the patch set
+ * @return the {@link CombinedCheckState} of the current patch set.
+ * @throws IOException if failed to get the {@link CombinedCheckState}.
+ * @throws OrmException if failed to get the {@link CombinedCheckState}.
+ */
+ CombinedCheckState getCombinedCheckState(Project.NameKey projectName, PatchSet.Id patchSetId)
+ throws IOException, OrmException;
+
@AutoValue
abstract class GetCheckOptions {
public static GetCheckOptions defaults() {
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
index 33119e5..dfd3892 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
@@ -15,7 +15,6 @@
package com.google.gerrit.plugins.checks.db;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
@@ -25,9 +24,7 @@
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.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.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -40,31 +37,25 @@
@Singleton
class CheckBackfiller {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final ChangeData.Factory changeDataFactory;
private final Checkers checkers;
private final Provider<AnonymousUser> anonymousUserProvider;
private final Provider<ChangeQueryBuilder> queryBuilderProvider;
@Inject
CheckBackfiller(
- Factory changeDataFactory,
Checkers checkers,
Provider<AnonymousUser> anonymousUserProvider,
Provider<ChangeQueryBuilder> queryBuilderProvider) {
- this.changeDataFactory = changeDataFactory;
this.checkers = checkers;
this.anonymousUserProvider = anonymousUserProvider;
this.queryBuilderProvider = queryBuilderProvider;
}
ImmutableList<Check> getBackfilledChecksForRelevantCheckers(
- Collection<Checker> candidates, ChangeNotes notes, PatchSet.Id psId) throws OrmException {
+ Collection<Checker> candidates, ChangeData cd, PatchSet.Id psId) throws OrmException {
if (candidates.isEmpty()) {
return ImmutableList.of();
}
- ChangeData cd = changeDataFactory.create(notes);
if (!psId.equals(cd.change().currentPatchSetId())) {
// The query system can only match against the current patch set; it doesn't make sense to
// backfill checkers for old patch sets.
@@ -85,8 +76,7 @@
}
Optional<Check> getBackfilledCheckForRelevantChecker(
- CheckerUuid candidate, ChangeNotes notes, PatchSet.Id psId) throws OrmException, IOException {
- ChangeData cd = changeDataFactory.create(notes);
+ CheckerUuid candidate, ChangeData cd, PatchSet.Id psId) throws OrmException, IOException {
if (!psId.equals(cd.change().currentPatchSetId())) {
// The query system can only match against the current patch set; it doesn't make sense to
// backfill checkers for old patch sets.
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index be242f7..c01b359 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -18,6 +18,8 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
@@ -25,39 +27,50 @@
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.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSet.Id;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.stream.Stream;
/** Class to read checks from NoteDb. */
@Singleton
class NoteDbChecks implements Checks {
- private final ChangeNotes.Factory changeNotesFactory;
- private final PatchSetUtil psUtil;
+ private final ChangeData.Factory changeDataFactory;
private final CheckNotes.Factory checkNotesFactory;
private final Checkers checkers;
private final CheckBackfiller checkBackfiller;
+ private final Provider<AnonymousUser> anonymousUserProvider;
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
@Inject
NoteDbChecks(
- ChangeNotes.Factory changeNotesFactory,
- PatchSetUtil psUtil,
+ ChangeData.Factory changeDataFactory,
CheckNotes.Factory checkNotesFactory,
Checkers checkers,
- CheckBackfiller checkBackfiller) {
- this.changeNotesFactory = changeNotesFactory;
- this.psUtil = psUtil;
+ CheckBackfiller checkBackfiller,
+ Provider<AnonymousUser> anonymousUserProvider,
+ Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ this.changeDataFactory = changeDataFactory;
this.checkNotesFactory = checkNotesFactory;
this.checkers = checkers;
this.checkBackfiller = checkBackfiller;
+ this.anonymousUserProvider = anonymousUserProvider;
+ this.queryBuilderProvider = queryBuilderProvider;
}
@Override
@@ -78,10 +91,10 @@
.findAny();
if (!result.isPresent() && options.backfillChecks()) {
- ChangeNotes notes =
- changeNotesFactory.create(checkKey.project(), checkKey.patchSet().getParentKey());
+ ChangeData changeData =
+ changeDataFactory.create(checkKey.project(), checkKey.patchSet().getParentKey());
return checkBackfiller.getBackfilledCheckForRelevantChecker(
- checkKey.checkerUuid(), notes, checkKey.patchSet());
+ checkKey.checkerUuid(), changeData, checkKey.patchSet());
}
return result;
@@ -91,9 +104,9 @@
Project.NameKey projectName, PatchSet.Id psId, GetCheckOptions options)
throws OrmException, IOException {
// TODO(gerrit-team): Instead of reading the complete notes map, read just one note.
- ChangeNotes notes = changeNotesFactory.create(projectName, psId.getParentKey());
- PatchSet patchSet = psUtil.get(notes, psId);
- CheckNotes checkNotes = checkNotesFactory.create(notes.getChange());
+ ChangeData changeData = changeDataFactory.create(projectName, psId.getParentKey());
+ PatchSet patchSet = changeData.patchSet(psId);
+ CheckNotes checkNotes = checkNotesFactory.create(changeData.change());
checkNotes.load();
ImmutableList<Check> existingChecks =
@@ -109,12 +122,53 @@
ImmutableList<Checker> checkersForBackfiller =
getCheckersForBackfiller(projectName, existingChecks);
ImmutableList<Check> backfilledChecks =
- checkBackfiller.getBackfilledChecksForRelevantCheckers(checkersForBackfiller, notes, psId);
+ checkBackfiller.getBackfilledChecksForRelevantCheckers(
+ checkersForBackfiller, changeData, psId);
return Stream.concat(existingChecks.stream(), backfilledChecks.stream())
.collect(toImmutableList());
}
+ @Override
+ public CombinedCheckState getCombinedCheckState(NameKey projectName, Id patchSetId)
+ throws IOException, OrmException {
+ ChangeData changeData = changeDataFactory.create(projectName, patchSetId.changeId);
+ ImmutableMap<String, Checker> allCheckersOfProject =
+ checkers.checkersOf(projectName).stream()
+ .collect(ImmutableMap.toImmutableMap(c -> c.getUuid().toString(), c -> c));
+
+ // Always backfilling checks to have a meaningful "CombinedCheckState" even when there are some
+ // or all checks missing.
+ ImmutableMap<String, Check> checks =
+ getChecks(projectName, patchSetId, GetCheckOptions.withBackfilling()).stream()
+ .collect(ImmutableMap.toImmutableMap(c -> c.key().checkerUuid().toString(), c -> c));
+
+ ChangeQueryBuilder queryBuilder =
+ queryBuilderProvider.get().asUser(anonymousUserProvider.get());
+ ImmutableListMultimap.Builder<CheckState, Boolean> statesAndRequired =
+ ImmutableListMultimap.builder();
+
+ for (Map.Entry<String, Check> entry : checks.entrySet()) {
+ String checkerUuid = entry.getKey();
+ Check check = entry.getValue();
+
+ Checker checker = allCheckersOfProject.get(checkerUuid);
+ if (checker == null) {
+ // A not-relevant check.
+ statesAndRequired.put(check.state(), false);
+ continue;
+ }
+
+ boolean isRequired =
+ checker.getStatus() == CheckerStatus.ENABLED
+ && checker.isRequired()
+ && checker.isCheckerRelevant(changeData, queryBuilder);
+ statesAndRequired.put(check.state(), isRequired);
+ }
+
+ return CombinedCheckState.combine(statesAndRequired.build());
+ }
+
private ImmutableList<Checker> getCheckersForBackfiller(
Project.NameKey projectName, List<Check> existingChecks) throws IOException {
ImmutableSet<CheckerUuid> checkersWithExistingChecks =
diff --git a/javatests/com/google/gerrit/plugins/checks/BUILD b/javatests/com/google/gerrit/plugins/checks/BUILD
index d87309f..814e5b8 100644
--- a/javatests/com/google/gerrit/plugins/checks/BUILD
+++ b/javatests/com/google/gerrit/plugins/checks/BUILD
@@ -11,6 +11,7 @@
"//java/com/google/gerrit/index:query_exception",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:guava",
"//lib/guice",
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
new file mode 100644
index 0000000..7b9481e
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerDefinitionTest.java
@@ -0,0 +1,60 @@
+// 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;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.checks.api.BlockingCondition.STATE_NOT_PASSING;
+
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.gerrit.plugins.checks.api.BlockingCondition;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.server.util.time.TimeUtil;
+import java.util.EnumSet;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Test;
+
+public class CheckerDefinitionTest {
+
+ @Test
+ public void notRequiredIfNoBlockingCondition() {
+ Checker checker = newChecker().setBlockingConditions(ImmutableSortedSet.of()).build();
+
+ assertThat(checker.isRequired()).isFalse();
+ }
+
+ @Test
+ public void requiredIfHasBlockingConditionStateNotPassing() {
+ Checker checker =
+ newChecker().setBlockingConditions(ImmutableSortedSet.of(STATE_NOT_PASSING)).build();
+
+ assertThat(checker.isRequired()).isTrue();
+ }
+
+ @Test
+ public void allBlockingConditionsConsidered() {
+ assertThat(EnumSet.allOf(BlockingCondition.class)).containsExactly(STATE_NOT_PASSING);
+ }
+
+ private Checker.Builder newChecker() {
+ return Checker.builder()
+ .setRepository(new NameKey("test-repo"))
+ .setStatus(CheckerStatus.ENABLED)
+ .setUuid(CheckerUuid.parse("schema:any-id"))
+ .setCreatedOn(TimeUtil.nowTs())
+ .setUpdatedOn(TimeUtil.nowTs())
+ .setRefState(ObjectId.zeroId());
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD b/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD
new file mode 100644
index 0000000..03caa7d
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/db/BUILD
@@ -0,0 +1,17 @@
+package(default_visibility = ["//plugins/checks:visibility"])
+
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "get_combined_check_state",
+ labels = [
+ "get_combined_check_state",
+ ],
+ deps = [
+ "//java/com/google/gerrit/git/testing",
+ "//plugins/checks:checks__plugin",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance",
+ "//plugins/checks/java/com/google/gerrit/plugins/checks/acceptance/testsuite",
+ ],
+)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java
new file mode 100644
index 0000000..0eb82ae
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/db/GetCombinedCheckStateIT.java
@@ -0,0 +1,121 @@
+// 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.acceptance.db;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.Checks;
+import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.acceptance.testsuite.TestCheckerCreation;
+import com.google.gerrit.plugins.checks.api.BlockingCondition;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckerStatus;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import org.junit.Before;
+import org.junit.Test;
+
+public class GetCombinedCheckStateIT extends AbstractCheckersTest {
+ // Tests against Gerrit Java API once it exists.
+ private Checks checks;
+ private PatchSet.Id patchSetId;
+
+ @Before
+ public void setUp() throws Exception {
+ checks = plugin.getHttpInjector().getInstance(Checks.class);
+
+ patchSetId = createChange().getPatchSetId();
+ }
+
+ @Test
+ public void returnsNotRelevantWhenNoCheck() throws Exception {
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.NOT_RELEVANT);
+ }
+
+ @Test
+ public void returnsSuccessfulWhenForSuccessfulCheckWhoseCheckerIsDisabled() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().status(CheckerStatus.DISABLED).create();
+ setCheckSuccessful(checkerUuid);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.SUCCESSFUL);
+ }
+
+ @Test
+ public void returnsWarningForFailedCheckWhoseCheckerIsNotRelevant() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().query("status:merged").create();
+ setCheckState(checkerUuid, CheckState.FAILED);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.WARNING);
+ }
+
+ @Test
+ public void returnsFailedWhenAnyRequiredCheckerFailed() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ setCheckSuccessful(checkerUuid);
+ CheckerUuid otherCheckerUuid = newRequiredChecker().create();
+ setCheckState(otherCheckerUuid, CheckState.FAILED);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.FAILED);
+ }
+
+ @Test
+ public void returnsSuccessfulWhenAllRequiredCheckersSucceeded() throws Exception {
+ CheckerUuid checkerUuid = newRequiredChecker().create();
+ setCheckSuccessful(checkerUuid);
+ CheckerUuid otherCheckerUuid = newRequiredChecker().create();
+ setCheckSuccessful(otherCheckerUuid);
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.SUCCESSFUL);
+ }
+
+ @Test
+ public void returnsInProgressWithOnlyBackFilledCheck() throws Exception {
+ newRequiredChecker().create();
+ // No check state is created.
+
+ CombinedCheckState combinedCheckState = checks.getCombinedCheckState(project, patchSetId);
+
+ assertThat(combinedCheckState).isEqualTo(CombinedCheckState.IN_PROGRESS);
+ }
+
+ private TestCheckerCreation.Builder newRequiredChecker() {
+ return checkerOperations
+ .newChecker()
+ .repository(project)
+ .status(CheckerStatus.ENABLED)
+ .blockingConditions(BlockingCondition.STATE_NOT_PASSING);
+ }
+
+ private void setCheckSuccessful(CheckerUuid checkerUuid) {
+ setCheckState(checkerUuid, CheckState.SUCCESSFUL);
+ }
+
+ private void setCheckState(CheckerUuid checkerUuid, CheckState checkState) {
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).setState(checkState).upsert();
+ }
+}
