CombinedCheckState: factor out code for counting check states
Currently, the "IN_PROGRESS" combined check state doesn't
work well for required and optional check states. The
problem is as an "enum", "IN_PROGRESS" can only have one
"passing" state (true or false), which affects whether
we should block submission or not.
But whether a "IN_PROGRESS" is passing or not depends on
whether there are required check states pending. That
means it can be true or false.
To resolve this, we have two possibilities:
1- split "IN_PROGRESS" into two, e.g.
"IN_PROGRESS_BLOCKING" and "IN_PROGRESS_NOT_BLOCKING".
But these names would probably be long and could
confuse users.
2- this commit proposes to keep the current combined
check state definition, but doesn't hard-code the
"passing" state in the "enum". This commit is a no-op
for Checks#getCombinedCheckState but it makes it
much easier to share the code for extracting the
"CheckStateCount".
The follow-up commit will teach the submittable check
to use "CheckStateCount" information and remove
the "pass" boolean from "CombinedCheckState" in the
end.
Change-Id: Ia85f9f9b9d947699f3c34216642814121a772b1e
diff --git a/java/com/google/gerrit/plugins/checks/api/CombinedCheckState.java b/java/com/google/gerrit/plugins/checks/api/CombinedCheckState.java
index 2919c02..0975441 100644
--- a/java/com/google/gerrit/plugins/checks/api/CombinedCheckState.java
+++ b/java/com/google/gerrit/plugins/checks/api/CombinedCheckState.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.checks.api;
+import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableListMultimap;
import java.util.Map;
@@ -62,35 +63,37 @@
*/
public static CombinedCheckState combine(
ImmutableListMultimap<CheckState, Boolean> statesAndRequired) {
- int inProgressCount = 0;
- int failedOptionalCount = 0;
- int successfulCount = 0;
- for (Map.Entry<CheckState, Boolean> e : statesAndRequired.entries()) {
- CheckState state = e.getKey();
- if (state.isInProgress()) {
- inProgressCount++;
- } else if (state == CheckState.FAILED) {
- if (e.getValue()) {
- return CombinedCheckState.FAILED;
- }
- failedOptionalCount++;
- } else if (state == CheckState.SUCCESSFUL) {
- successfulCount++;
- } else if (state == CheckState.NOT_RELEVANT) {
- continue;
- } else {
- throw new IllegalStateException("invalid state: " + state);
- }
+ CheckStateCount checkStateCount = CheckStateCount.create(statesAndRequired);
+ return combine(checkStateCount);
+ }
+
+ /**
+ * Combines multiple per-check states into a single combined state based on the count result of
+ * each check state.
+ *
+ * <p>See documentation of specific enum values for precise semantics.
+ *
+ * @param checkStateCount count of check states.
+ * @return combined state.
+ */
+ private static CombinedCheckState combine(CheckStateCount checkStateCount) {
+ if (checkStateCount.failedRequiredCount() > 0) {
+ return FAILED;
}
- if (inProgressCount > 0) {
+
+ if (checkStateCount.inProgressOptionalCount() > 0
+ || checkStateCount.inProgressRequiredCount() > 0) {
return IN_PROGRESS;
}
- if (failedOptionalCount > 0) {
+
+ if (checkStateCount.failedOptionalCount() > 0) {
return WARNING;
}
- if (successfulCount > 0) {
+
+ if (checkStateCount.successfulCount() > 0) {
return SUCCESSFUL;
}
+
return NOT_RELEVANT;
}
@@ -111,4 +114,85 @@
public boolean isPassing() {
return passing;
}
+
+ @AutoValue
+ public abstract static class CheckStateCount {
+ /**
+ * Get the count of each {@link CheckState}.
+ *
+ * @param statesAndRequired map of state to a list of booleans, one per check, indicating
+ * whether that particular check is required in the context of a particular change.
+ * @return the {@link CheckStateCount} of the given state map.
+ */
+ public static CheckStateCount create(
+ ImmutableListMultimap<CheckState, Boolean> statesAndRequired) {
+ int failedRequiredCount = 0;
+ int failedOptionalCount = 0;
+ int inProgressRequiredCount = 0;
+ int inProgressOptionalCount = 0;
+ int successfulCount = 0;
+
+ for (Map.Entry<CheckState, Boolean> checkStateAndRequiredState :
+ statesAndRequired.entries()) {
+ CheckState state = checkStateAndRequiredState.getKey();
+ if (state.isInProgress()) {
+ if (checkStateAndRequiredState.getValue()) {
+ inProgressRequiredCount++;
+ } else {
+ inProgressOptionalCount++;
+ }
+ } else if (state == CheckState.FAILED) {
+ if (checkStateAndRequiredState.getValue()) {
+ failedRequiredCount++;
+ } else {
+ failedOptionalCount++;
+ }
+ } else if (state == CheckState.SUCCESSFUL) {
+ successfulCount++;
+ } else if (state != CheckState.NOT_RELEVANT) {
+ throw new IllegalStateException("invalid state: " + state);
+ }
+ }
+
+ return new AutoValue_CombinedCheckState_CheckStateCount.Builder()
+ .failedRequiredCount(failedRequiredCount)
+ .failedOptionalCount(failedOptionalCount)
+ .inProgressRequiredCount(inProgressRequiredCount)
+ .inProgressOptionalCount(inProgressOptionalCount)
+ .successfulCount(successfulCount)
+ .build();
+ }
+
+ /** Count of the failed check states which are required for submission. */
+ public abstract int failedRequiredCount();
+
+ /** Count of the failed check states which are not required for submission. */
+ public abstract int failedOptionalCount();
+
+ /** Count of the in-progress check states which are required for submission. */
+ public abstract int inProgressRequiredCount();
+
+ /** Count of the in-progress check states which are not required for submission. */
+ public abstract int inProgressOptionalCount();
+
+ /** Count of the successful check states. */
+ public abstract int successfulCount();
+
+ public abstract Builder toBuilder();
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder inProgressRequiredCount(int inProgressRequiredCount);
+
+ public abstract Builder inProgressOptionalCount(int inProgressOptionalCount);
+
+ public abstract Builder failedRequiredCount(int failedRequiredCount);
+
+ public abstract Builder failedOptionalCount(int failedOptionalCount);
+
+ public abstract Builder successfulCount(int successfulCount);
+
+ public abstract CheckStateCount build();
+ }
+ }
}
