ChecksSubmitRule: use Check#areAllRequiredCheckersPassing instead

The "IN_PROGRESS" combined check state isn't enough for
the submit rule to tell whether a change is submittable
or not. To resolve this, ChecksSubmitRule can simply
check whether all required checkers have passed through
"Checks#areAllRequiredCheckersPassing".

TODO: we need to find a way to initialize the
CombinedCheckStateCache, which was done by the
ChecksSubmitRule before this change.

Change-Id: Ib1926cb4d4c17966d729ea75b1fe6895f0fbd398
diff --git a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
index 5391d31..bf4fb78 100644
--- a/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
+++ b/java/com/google/gerrit/plugins/checks/rules/ChecksSubmitRule.java
@@ -22,8 +22,7 @@
 import com.google.gerrit.common.data.SubmitRequirement;
 import com.google.gerrit.extensions.annotations.Exports;
 import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
-import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.plugins.checks.Checks;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
@@ -32,6 +31,7 @@
 import com.google.gerrit.server.rules.SubmitRule;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import java.util.Collection;
 
 @Singleton
@@ -53,11 +53,11 @@
     }
   }
 
-  private final CombinedCheckStateCache combinedCheckStateCache;
+  private final Checks checks;
 
   @Inject
-  public ChecksSubmitRule(CombinedCheckStateCache combinedCheckStateCache) {
-    this.combinedCheckStateCache = combinedCheckStateCache;
+  public ChecksSubmitRule(Checks checks) {
+    this.checks = checks;
   }
 
   @Override
@@ -75,11 +75,11 @@
       return singletonRecordForRuleError(errorMessage);
     }
 
-    CombinedCheckState combinedCheckState;
+    boolean areAllRequiredCheckersPassing;
     try {
-      // Reload value in cache to fix up inconsistencies between cache and actual state.
-      combinedCheckState = combinedCheckStateCache.reload(project, currentPathSetId);
-    } catch (RuntimeException e) {
+      areAllRequiredCheckersPassing =
+          checks.areAllRequiredCheckersPassing(project, currentPathSetId);
+    } catch (IOException e) {
       String errorMessage =
           String.format("failed to evaluate check states for change %s", changeId);
       logger.atSevere().withCause(e).log(errorMessage);
@@ -87,7 +87,7 @@
     }
 
     SubmitRecord submitRecord = new SubmitRecord();
-    if (combinedCheckState.isPassing()) {
+    if (areAllRequiredCheckersPassing) {
       submitRecord.status = Status.OK;
       return ImmutableList.of(submitRecord);
     }
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
index 8f84423..a00c2cd 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
@@ -118,16 +118,17 @@
 
     assertThat(queryChangeCheckInfo(changeId))
         .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    // Cache was prepopulated during update.
-    assertThat(cache.getStats()).since(start).hasHitCount(1);
-    assertThat(cache.getStats()).since(start).hasMissCount(0);
+    // Cache hasn't yet populated during update.
+    // TODO(xchangcheng): still initialize the cache early without doing in the submit rule.
+    assertThat(cache.getStats()).since(start).hasHitCount(0);
+    assertThat(cache.getStats()).since(start).hasMissCount(1);
     assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
     assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
 
     assertThat(queryChangeCheckInfo(changeId))
         .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    assertThat(cache.getStats()).since(start).hasHitCount(2);
-    assertThat(cache.getStats()).since(start).hasMissCount(0);
+    assertThat(cache.getStats()).since(start).hasHitCount(1);
+    assertThat(cache.getStats()).since(start).hasMissCount(1);
     assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
     assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
   }
@@ -149,17 +150,17 @@
 
     assertThat(getChangeCheckInfo(changeId))
         .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    // Incurs reloads in both submit rule and attribute factory paths.
-    assertThat(cache.getStats()).since(start).hasHitCount(3);
+    // Incurs reloads in attribute factory paths.
+    assertThat(cache.getStats()).since(start).hasHitCount(2);
     assertThat(cache.getStats()).since(start).hasMissCount(0);
-    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(1);
+    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
     assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
 
     assertThat(queryChangeCheckInfo(changeId))
         .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    assertThat(cache.getStats()).since(start).hasHitCount(4);
+    assertThat(cache.getStats()).since(start).hasHitCount(3);
     assertThat(cache.getStats()).since(start).hasMissCount(0);
-    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(1);
+    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
     assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
   }
 
@@ -206,25 +207,25 @@
 
     assertThat(getChangeCheckInfo(changeId))
         .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    // Incurs reloads in both submit rule and attribute factory paths.
+    // Incurs reloads in attribute factory paths.
+    assertThat(cache.getStats()).since(start).hasHitCount(0);
+    assertThat(cache.getStats()).since(start).hasMissCount(1);
+    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
+    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+
+    assertThat(getChangeCheckInfo(changeId))
+        .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
+    assertThat(cache.getStats()).since(start).hasHitCount(1);
+    assertThat(cache.getStats()).since(start).hasMissCount(1);
+    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(1);
+    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
+
+    assertThat(getChangeCheckInfo(changeId))
+        .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
     assertThat(cache.getStats()).since(start).hasHitCount(2);
-    assertThat(cache.getStats()).since(start).hasMissCount(0);
+    assertThat(cache.getStats()).since(start).hasMissCount(1);
     assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(2);
-    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
-
-    assertThat(getChangeCheckInfo(changeId))
-        .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    assertThat(cache.getStats()).since(start).hasHitCount(4);
-    assertThat(cache.getStats()).since(start).hasMissCount(0);
-    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(4);
-    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
-
-    assertThat(getChangeCheckInfo(changeId))
-        .hasValue(new ChangeCheckInfo("checks", CombinedCheckState.NOT_RELEVANT));
-    assertThat(cache.getStats()).since(start).hasHitCount(6);
-    assertThat(cache.getStats()).since(start).hasMissCount(0);
-    assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(6);
-    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(0);
+    assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
   }
 
   private Optional<ChangeCheckInfo> getChangeCheckInfo(Change.Id id) throws Exception {
diff --git a/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
index d80fec9..afdda35 100644
--- a/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/rules/ChecksSubmitRuleTest.java
@@ -21,7 +21,7 @@
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.common.data.SubmitRecord;
-import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
+import com.google.gerrit.plugins.checks.Checks;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -29,6 +29,7 @@
 import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.util.time.TimeUtil;
+import java.io.IOException;
 import java.util.Collection;
 import org.easymock.EasyMock;
 import org.eclipse.jgit.lib.ObjectId;
@@ -38,7 +39,7 @@
   @Test
   public void loadingCurrentPatchSetFails() throws Exception {
     ChecksSubmitRule checksSubmitRule =
-        new ChecksSubmitRule(EasyMock.createStrictMock(CombinedCheckStateCache.class));
+        new ChecksSubmitRule(EasyMock.createStrictMock(Checks.class));
 
     ChangeData cd = EasyMock.createStrictMock(ChangeData.class);
     expect(cd.project()).andReturn(Project.nameKey("My-Project"));
@@ -53,12 +54,12 @@
 
   @Test
   public void getCombinedCheckStateFails() throws Exception {
-    CombinedCheckStateCache cache = EasyMock.createStrictMock(CombinedCheckStateCache.class);
-    expect(cache.reload(anyObject(), anyObject()))
-        .andThrow(new IllegalStateException("Fail for test"));
-    replay(cache);
+    Checks checks = EasyMock.createStrictMock(Checks.class);
+    expect(checks.areAllRequiredCheckersPassing(anyObject(), anyObject()))
+        .andThrow(new IOException("Fail for test"));
+    replay(checks);
 
-    ChecksSubmitRule checksSubmitRule = new ChecksSubmitRule(cache);
+    ChecksSubmitRule checksSubmitRule = new ChecksSubmitRule(checks);
 
     Change.Id changeId = Change.id(1);
     ChangeData cd = EasyMock.createStrictMock(ChangeData.class);