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);