Wrap "required" field in CheckInfo
CheckSubmitImpactInfo was introduced, and it currently only contains
the existing "required" field. In the future, we will be able to add
more details to CheckSubmitImpactInfo since the Boolean field is not the
only parameter impacting the submission.
The follow-up change will have the necessary updates in the docs.
Change-Id: I4b91824c67fc9394bcaf608d930633c149e85117
diff --git a/java/com/google/gerrit/plugins/checks/CheckJson.java b/java/com/google/gerrit/plugins/checks/CheckJson.java
index 7fa17ca..b3225f1 100644
--- a/java/com/google/gerrit/plugins/checks/CheckJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckJson.java
@@ -18,6 +18,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Change;
import com.google.gerrit.plugins.checks.api.CheckInfo;
+import com.google.gerrit.plugins.checks.api.CheckSubmitImpactInfo;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
@@ -94,7 +95,9 @@
info.checkerName = checker.getName();
info.checkerStatus = checker.getStatus();
info.blocking = checker.getBlockingConditions();
- info.required = checks.isRequiredForSubmit(checker, changeId) ? true : null;
+ info.submitImpact = new CheckSubmitImpactInfo();
+ info.submitImpact.required =
+ checks.isRequiredForSubmit(checker, changeId) ? true : null;
info.checkerDescription = checker.getDescription().orElse(null);
});
} catch (ConfigInvalidException e) {
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
index 4fc22c5..4766990 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
@@ -56,8 +56,8 @@
/** Blocking conditions that apply to this check. */
public Set<BlockingCondition> blocking;
- /** True if the check is required for submission, unset otherwise */
- public Boolean required;
+ /** Submit impact of this check. */
+ public CheckSubmitImpactInfo submitImpact;
/** Description of the checker that produced this check */
public String checkerDescription;
@@ -82,7 +82,7 @@
&& Objects.equals(other.checkerName, checkerName)
&& Objects.equals(other.checkerStatus, checkerStatus)
&& Objects.equals(other.blocking, blocking)
- && Objects.equals(other.required, required)
+ && Objects.equals(other.submitImpact, submitImpact)
&& Objects.equals(other.checkerDescription, checkerDescription);
}
@@ -103,7 +103,7 @@
checkerName,
checkerStatus,
blocking,
- required,
+ submitImpact,
checkerDescription);
}
@@ -124,7 +124,7 @@
.add("checkerName", checkerName)
.add("checkerStatus", checkerStatus)
.add("blocking", blocking)
- .add("required", required)
+ .add("submitImpact", submitImpact)
.add("checkerDescription", checkerDescription)
.toString();
}
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckSubmitImpactInfo.java b/java/com/google/gerrit/plugins/checks/api/CheckSubmitImpactInfo.java
new file mode 100644
index 0000000..6c99574
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/api/CheckSubmitImpactInfo.java
@@ -0,0 +1,51 @@
+// 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.plugins.checks.api;
+
+import com.google.common.base.MoreObjects;
+import java.util.Objects;
+
+/**
+ * Summary of the impact the check has on the submission of the change. Besides a general
+ * indication, this may also include related details which help users to better understand that
+ * impact.
+ */
+public class CheckSubmitImpactInfo {
+
+ /**
+ * Indicates whether this check is included in submit considerations of the change. This depends
+ * on various factors and can change over the lifetime of a check.
+ */
+ public Boolean required;
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof CheckSubmitImpactInfo)) {
+ return false;
+ }
+ CheckSubmitImpactInfo other = (CheckSubmitImpactInfo) o;
+ return Objects.equals(other.required, required);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(required);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("required", required).toString();
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
index 638ce4c..97fa1a6 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -43,6 +43,7 @@
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckerTestData;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckSubmitImpactInfo;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.gson.reflect.TypeToken;
@@ -95,7 +96,8 @@
expectedCheckInfo.checkerName = "My Checker";
expectedCheckInfo.checkerStatus = CheckerStatus.ENABLED;
expectedCheckInfo.blocking = ImmutableSortedSet.of();
- expectedCheckInfo.required = null;
+ expectedCheckInfo.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo.submitImpact.required = null;
expectedCheckInfo.checkerDescription = "Description";
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER))
.isEqualTo(expectedCheckInfo);
@@ -114,7 +116,8 @@
expectedCheckInfo.checkerName = "My Checker";
expectedCheckInfo.checkerStatus = CheckerStatus.ENABLED;
expectedCheckInfo.blocking = ImmutableSortedSet.of();
- expectedCheckInfo.required = null;
+ expectedCheckInfo.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo.submitImpact.required = null;
RestResponse r =
adminRestSession.get(
@@ -263,7 +266,7 @@
}
@Test
- public void getCheckReturnsRequiredOnlyForCheckerOption() throws Exception {
+ public void getCheckReturnsSubmitImpactOnlyForCheckerOption() throws Exception {
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).required().create();
@@ -273,8 +276,9 @@
assertThat(getCheckInfo(patchSetId, checkerUuid).blocking).isNull();
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).blocking)
.isNotEmpty();
- assertThat(getCheckInfo(patchSetId, checkerUuid).required).isNull();
- assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isTrue();
+ assertThat(getCheckInfo(patchSetId, checkerUuid).submitImpact).isNull();
+ assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).submitImpact)
+ .isNotNull();
}
@Test
@@ -288,7 +292,9 @@
.create();
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
checkOperations.newCheck(checkKey).upsert();
- assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isNull();
+ assertThat(
+ getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).submitImpact.required)
+ .isNull();
}
@Test
@@ -297,7 +303,9 @@
checkerOperations.newChecker().repository(project).disable().required().create();
CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
checkOperations.newCheck(checkKey).upsert();
- assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isNull();
+ assertThat(
+ getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).submitImpact.required)
+ .isNull();
}
@Test
@@ -316,7 +324,7 @@
// Checker fields are not set.
assertThat(check.checkerName).isNull();
assertThat(check.blocking).isNull();
- assertThat(check.required).isNull();
+ assertThat(check.submitImpact).isNull();
assertThat(check.checkerStatus).isNull();
// Check that at least some non-checker fields are set to ensure that we didn't get a completely
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
index 6ea438e..1be5870 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -34,6 +34,7 @@
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CheckSubmitImpactInfo;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gson.reflect.TypeToken;
import com.google.inject.Inject;
@@ -92,14 +93,16 @@
expectedCheckInfo1.repository = project.get();
expectedCheckInfo1.checkerName = checkerName1;
expectedCheckInfo1.blocking = ImmutableSet.of();
- expectedCheckInfo1.required = null;
+ expectedCheckInfo1.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo1.submitImpact.required = null;
expectedCheckInfo1.checkerStatus = CheckerStatus.ENABLED;
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
- expectedCheckInfo2.required = null;
+ expectedCheckInfo2.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo2.submitImpact.required = null;
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
assertThat(checksApiFactory.revision(patchSetId).list(ListChecksOption.CHECKER))
@@ -126,14 +129,16 @@
expectedCheckInfo1.repository = project.get();
expectedCheckInfo1.checkerName = checkerName1;
expectedCheckInfo1.blocking = ImmutableSet.of();
- expectedCheckInfo1.required = null;
+ expectedCheckInfo1.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo1.submitImpact.required = null;
expectedCheckInfo1.checkerStatus = CheckerStatus.ENABLED;
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
- expectedCheckInfo2.required = null;
+ expectedCheckInfo2.submitImpact = new CheckSubmitImpactInfo();
+ expectedCheckInfo2.submitImpact.required = null;
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
RestResponse r =
@@ -178,7 +183,7 @@
CheckInfo check1 = maybeCheck1.get();
assertThat(check1.checkerName).isEqualTo(checkerName1);
assertThat(check1.blocking).isEmpty();
- assertThat(check1.required).isNull();
+ assertThat(check1.submitImpact).isNotNull();
assertThat(check1.checkerStatus).isEqualTo(CheckerStatus.ENABLED);
Optional<CheckInfo> maybeCheck2 =
@@ -187,7 +192,7 @@
CheckInfo check2 = maybeCheck2.get();
assertThat(check2.checkerName).isNull();
assertThat(check2.blocking).isNull();
- assertThat(check2.required).isNull();
+ assertThat(check2.submitImpact).isNull();
assertThat(check2.checkerStatus).isNull();
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
index 02b187d..f68e716 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
@@ -81,7 +81,7 @@
assertThat(foundCheck.checkerName).isNull();
assertThat(foundCheck.checkerStatus).isNull();
assertThat(foundCheck.blocking).isNull();
- assertThat(foundCheck.required).isNull();
+ assertThat(foundCheck.submitImpact).isNull();
}
@Test
