opendev/gerrit
Code Issues Proposed changes

Make SubmitRule#evaluate return an Optional instead of a Collection

Browse Source 
The original specification used a Collection so that a single SubmitRule
can return multiple SubmitRecords. The main use-case was inheritance and
being open to more future usages.

Looking at this today, this seems strange because a SubmitRecord keeps a
list of SubmitRequirements, so even if it is not allowed to return a
Collection, it can still contribute a number of SubmitRecords.

This does invalidate the original reasoning from using a Collection
which is why this commit replaces it with an Optional.

Change-Id: I0dce438504b30fe102f5b83af9f7976659f1fae8
This commit is contained in:
Patrick Hiesel
2019-08-13 15:29:27 +02:00
parent e7fc5ee635
commit 7ab1b18ef8
16 changed files with 115 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
Documentation/dev-plugins.txt
View File

@@ -2600,10 +2600,8 @@ If a change is ready to be submitted, `OK`. If it is not ready and requires
modifications, `NOT_READY`. Other statuses are available for particular cases.
A change can be submitted if all the plugins accept the change.
Plugins may also decide not to vote on a given change by returning an empty
Collection (ie: the plugin is not enabled for this repository), or to vote
several times (ie: one SubmitRecord per project in the hierarchy).
The results are handled as if multiple plugins voted for the change.
Plugins may also decide not to vote on a given change by returning an
`Optional.empty()` (ie: the plugin is not enabled for this repository).
If a plugin decides not to vote, it's name will not be displayed in the UI and
it will not be recoded in the database.
@@ -2638,20 +2636,20 @@ changes that are marked as WIP or that are closed (abandoned, merged) can't be m
[source, java]
----
import java.util.Collection;
import java.util.Optional;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitRecord.Status;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
public class MyPluginRules implements SubmitRule {
public Collection<SubmitRecord> evaluate(ChangeData changeData) {
public Optional<SubmitRecord> evaluate(ChangeData changeData) {
// Implement your submitability logic here
// Assuming we want to prevent this change from being submitted:
SubmitRecord record = new SubmitRecord();
record.status = Status.NOT_READY;
return record;
return Optional.of(record);
}
}
----

8
Documentation/rest-api-changes.txt
View File

@@ -4345,8 +4345,10 @@ a project-specific rule.
R = label('Any-Label-Name', reject(_)).
----
The response is a list of link:#submit-record[SubmitRecord] entries
describing the permutations that satisfy the tested submit rule.
The response is a link:#submit-record[SubmitRecord] describing the
permutations that satisfy the tested submit rule.
If the submit rule was a np-op, the response is "`204 No Content`".
.Response
----
@@ -4355,14 +4357,12 @@ describing the permutations that satisfy the tested submit rule.
Content-Type: application/json; charset=UTF-8
)]}'
[
{
"status": "NOT_READY",
"reject": {
"Any-Label-Name": {}
}
}
]
----
When testing with the `curl` command line client the

4
java/com/google/gerrit/extensions/api/changes/RevisionApi.java
View File

@@ -146,7 +146,7 @@ public interface RevisionApi {
SubmitType testSubmitType(TestSubmitRuleInput in) throws RestApiException;
List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException;
TestSubmitRuleInfo testSubmitRule(TestSubmitRuleInput in) throws RestApiException;
MergeListRequest getMergeList() throws RestApiException;
@@ -351,7 +351,7 @@ public interface RevisionApi {
}
@Override
public List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
public TestSubmitRuleInfo testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
throw new NotImplementedException();
}

2
java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@@ -547,7 +547,7 @@ class RevisionApiImpl implements RevisionApi {
}
@Override
public List<TestSubmitRuleInfo> testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
public TestSubmitRuleInfo testSubmitRule(TestSubmitRuleInput in) throws RestApiException {
try {
return testSubmitRule.get().apply(revision, in).value();
} catch (Exception e) {

15
java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
View File

@@ -31,9 +31,9 @@ import com.google.gerrit.server.rules.PrologRule;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
/**
@@ -83,15 +83,15 @@ public class SubmitRuleEvaluator {
this.opts = options;
}
public static List<SubmitRecord> defaultRuleError() {
public static SubmitRecord defaultRuleError() {
return createRuleError(DEFAULT_MSG);
}
public static List<SubmitRecord> createRuleError(String err) {
public static SubmitRecord createRuleError(String err) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.RULE_ERROR;
rec.errorMessage = err;
return Collections.singletonList(rec);
return rec;
}
public static SubmitTypeRecord defaultTypeError() {
@@ -120,7 +120,7 @@ public class SubmitRuleEvaluator {
throw new NoSuchProjectException(cd.project());
}
} catch (StorageException | NoSuchProjectException e) {
return ruleError("Error looking up change " + cd.getId(), e);
return Collections.singletonList(ruleError("Error looking up change " + cd.getId(), e));
}
if ((!opts.allowClosed() || OnlineReindexMode.isActive()) && change.isClosed()) {
@@ -133,12 +133,13 @@ public class SubmitRuleEvaluator {
// and then we collect the results in one list.
return Streams.stream(submitRules)
.map(c -> c.call(s -> s.evaluate(cd)))
.flatMap(Collection::stream)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
}
}
private List<SubmitRecord> ruleError(String err, Exception e) {
private SubmitRecord ruleError(String err, Exception e) {
logger.atSevere().withCause(e).log(err);
return defaultRuleError();
}

16
java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
View File

@@ -15,8 +15,6 @@
package com.google.gerrit.server.restapi.change;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
@@ -37,7 +35,6 @@ import com.google.gerrit.server.rules.PrologRule;
import com.google.gerrit.server.rules.RulesCache;
import com.google.inject.Inject;
import java.util.LinkedHashMap;
import java.util.List;
import org.kohsuke.args4j.Option;
public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubmitRuleInput> {
@@ -65,7 +62,7 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
}
@Override
public Response<List<TestSubmitRuleInfo>> apply(RevisionResource rsrc, TestSubmitRuleInput input)
public Response<TestSubmitRuleInfo> apply(RevisionResource rsrc, TestSubmitRuleInput input)
throws AuthException, PermissionBackendException, BadRequestException {
if (input == null) {
input = new TestSubmitRuleInput();
@@ -83,15 +80,12 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
throw new BadRequestException("project not found");
}
ChangeData cd = changeDataFactory.create(rsrc.getNotes());
List<SubmitRecord> records =
ImmutableList.copyOf(
SubmitRecord record =
prologRule.evaluate(
cd, PrologOptions.dryRunOptions(input.rule, input.filters == Filters.SKIP)));
List<TestSubmitRuleInfo> out = Lists.newArrayListWithCapacity(records.size());
cd, PrologOptions.dryRunOptions(input.rule, input.filters == Filters.SKIP));
AccountLoader accounts = accountInfoFactory.create(true);
for (SubmitRecord r : records) {
out.add(newSubmitRuleInfo(r, accounts));
}
TestSubmitRuleInfo out = newSubmitRuleInfo(record, accounts);
accounts.fill();
return Response.ok(out);
}

12
java/com/google/gerrit/server/rules/DefaultSubmitRule.java
View File

@@ -31,8 +31,8 @@ import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
/**
* Java implementation of Gerrit's default pre-submit rules behavior: check if the labels have the
@@ -62,13 +62,13 @@ public final class DefaultSubmitRule implements SubmitRule {
}
@Override
public Collection<SubmitRecord> evaluate(ChangeData cd) {
public Optional<SubmitRecord> evaluate(ChangeData cd) {
ProjectState projectState = projectCache.get(cd.project());
// In case at least one project has a rules.pl file, we let Prolog handle it.
// The Prolog rules engine will also handle the labels for us.
if (projectState == null || projectState.hasPrologRules()) {
return Collections.emptyList();
return Optional.empty();
}
SubmitRecord submitRecord = new SubmitRecord();
@@ -85,7 +85,7 @@ public final class DefaultSubmitRule implements SubmitRule {
submitRecord.errorMessage = "Unable to fetch labels and approvals for the change";
submitRecord.status = SubmitRecord.Status.RULE_ERROR;
return Collections.singletonList(submitRecord);
return Optional.of(submitRecord);
}
submitRecord.labels = new ArrayList<>(labelTypes.size());
@@ -98,7 +98,7 @@ public final class DefaultSubmitRule implements SubmitRule {
submitRecord.errorMessage = "Unable to find the LabelFunction for label " + t.getName();
submitRecord.status = SubmitRecord.Status.RULE_ERROR;
return Collections.singletonList(submitRecord);
return Optional.of(submitRecord);
}
Collection<PatchSetApproval> approvalsForLabel = getApprovalsForLabel(approvals, t);
@@ -118,7 +118,7 @@ public final class DefaultSubmitRule implements SubmitRule {
}
}
return Collections.singletonList(submitRecord);
return Optional.of(submitRecord);
}
private static List<PatchSetApproval> getApprovalsForLabel(

18
java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.rules;
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
@@ -34,6 +33,7 @@ import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
/**
* Rule to require an approval from a user that did not upload the current patch set or block
@@ -59,7 +59,7 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
IgnoreSelfApprovalRule() {}
@Override
public Collection<SubmitRecord> evaluate(ChangeData cd) {
public Optional<SubmitRecord> evaluate(ChangeData cd) {
List<LabelType> labelTypes;
List<PatchSetApproval> approvals;
try {
@@ -67,13 +67,13 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
approvals = cd.currentApprovals();
} catch (StorageException e) {
logger.atWarning().withCause(e).log(E_UNABLE_TO_FETCH_LABELS);
return singletonRuleError(E_UNABLE_TO_FETCH_LABELS);
return ruleError(E_UNABLE_TO_FETCH_LABELS);
}
boolean shouldIgnoreSelfApproval = labelTypes.stream().anyMatch(LabelType::ignoreSelfApproval);
if (!shouldIgnoreSelfApproval) {
// Shortcut to avoid further processing if no label should ignore uploader approvals
return ImmutableList.of();
return Optional.empty();
}
Account.Id uploader;
@@ -81,7 +81,7 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
uploader = cd.currentPatchSet().uploader();
} catch (StorageException e) {
logger.atWarning().withCause(e).log(E_UNABLE_TO_FETCH_UPLOADER);
return singletonRuleError(E_UNABLE_TO_FETCH_UPLOADER);
return ruleError(E_UNABLE_TO_FETCH_UPLOADER);
}
SubmitRecord submitRecord = new SubmitRecord();
@@ -123,10 +123,10 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
}
if (submitRecord.labels.isEmpty()) {
return ImmutableList.of();
return Optional.empty();
}
return ImmutableList.of(submitRecord);
return Optional.of(submitRecord);
}
private static boolean labelCheckPassed(SubmitRecord.Label label) {
@@ -143,11 +143,11 @@ public class IgnoreSelfApprovalRule implements SubmitRule {
return false;
}
private static Collection<SubmitRecord> singletonRuleError(String reason) {
private static Optional<SubmitRecord> ruleError(String reason) {
SubmitRecord submitRecord = new SubmitRecord();
submitRecord.errorMessage = reason;
submitRecord.status = SubmitRecord.Status.RULE_ERROR;
return ImmutableList.of(submitRecord);
return Optional.of(submitRecord);
}
@VisibleForTesting

11
java/com/google/gerrit/server/rules/PrologRule.java
View File

@@ -21,8 +21,7 @@ import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.Collection;
import java.util.Collections;
import java.util.Optional;
@Singleton
public class PrologRule implements SubmitRule {
@@ -36,17 +35,17 @@ public class PrologRule implements SubmitRule {
}
@Override
public Collection<SubmitRecord> evaluate(ChangeData cd) {
public Optional<SubmitRecord> evaluate(ChangeData cd) {
ProjectState projectState = projectCache.get(cd.project());
// We only want to run the Prolog engine if we have at least one rules.pl file to use.
if ((projectState == null || !projectState.hasPrologRules())) {
return Collections.emptyList();
return Optional.empty();
}
return evaluate(cd, PrologOptions.defaultOptions());
return Optional.of(evaluate(cd, PrologOptions.defaultOptions()));
}
public Collection<SubmitRecord> evaluate(ChangeData cd, PrologOptions opts) {
public SubmitRecord evaluate(ChangeData cd, PrologOptions opts) {
return getEvaluator(cd, opts).evaluate();
}

65
java/com/google/gerrit/server/rules/PrologRuleEvaluator.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.rules;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.project.SubmitRuleEvaluator.createRuleError;
import static com.google.gerrit.server.project.SubmitRuleEvaluator.defaultRuleError;
import static com.google.gerrit.server.project.SubmitRuleEvaluator.defaultTypeError;
@@ -50,7 +51,6 @@ import com.googlecode.prolog_cafe.lang.Term;
import com.googlecode.prolog_cafe.lang.VariableTerm;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
@@ -140,10 +140,9 @@ public class PrologRuleEvaluator {
/**
* Evaluate the submit rules.
*
* @return List of {@link SubmitRecord} objects returned from the evaluated rules, including any
* errors.
* @return {@link SubmitRecord} returned from the evaluated rules. Can include errors.
*/
public Collection<SubmitRecord> evaluate() {
public SubmitRecord evaluate() {
Change change;
try {
change = cd.change();
@@ -193,28 +192,32 @@ public class PrologRuleEvaluator {
* output. Later after the loop the out collection is reversed to restore it to the original
* ordering.
*/
public List<SubmitRecord> resultsToSubmitRecord(Term submitRule, List<Term> results) {
boolean foundOk = false;
List<SubmitRecord> out = new ArrayList<>(results.size());
public SubmitRecord resultsToSubmitRecord(Term submitRule, List<Term> results) {
checkState(!results.isEmpty(), "the list of Prolog terms must not be empty");
SubmitRecord resultSubmitRecord = new SubmitRecord();
resultSubmitRecord.labels = new ArrayList<>();
for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) {
Term submitRecord = results.get(resultIdx);
SubmitRecord rec = new SubmitRecord();
out.add(rec);
if (!(submitRecord instanceof StructureTerm) || 1 != submitRecord.arity()) {
return invalidResult(submitRule, submitRecord);
}
if ("ok".equals(submitRecord.name())) {
rec.status = SubmitRecord.Status.OK;
} else if ("not_ready".equals(submitRecord.name())) {
rec.status = SubmitRecord.Status.NOT_READY;
} else {
if (!"ok".equals(submitRecord.name()) && !"not_ready".equals(submitRecord.name())) {
return invalidResult(submitRule, submitRecord);
}
// This transformation is required to adapt Prolog's behavior to the way Gerrit handles
// SubmitRecords, as defined in the SubmitRecord#allRecordsOK method.
// When several rules are defined in Prolog, they are all matched to a SubmitRecord. We want
// the change to be submittable when at least one result is OK.
if ("ok".equals(submitRecord.name())) {
resultSubmitRecord.status = SubmitRecord.Status.OK;
} else if ("not_ready".equals(submitRecord.name()) && resultSubmitRecord.status == null) {
resultSubmitRecord.status = SubmitRecord.Status.NOT_READY;
}
// Unpack the one argument. This should also be a structure with one
// argument per label that needs to be reported on to the caller.
//
@@ -224,8 +227,6 @@ public class PrologRuleEvaluator {
return invalidResult(submitRule, submitRecord);
}
rec.labels = new ArrayList<>(submitRecord.arity());
for (Term state : ((StructureTerm) submitRecord).args()) {
if (!(state instanceof StructureTerm)
|| 2 != state.arity()
@@ -234,7 +235,7 @@ public class PrologRuleEvaluator {
}
SubmitRecord.Label lbl = new SubmitRecord.Label();
rec.labels.add(lbl);
resultSubmitRecord.labels.add(lbl);
lbl.label = checkLabelName(state.arg(0).name());
Term status = state.arg(1);
@@ -265,24 +266,12 @@ public class PrologRuleEvaluator {
}
}
if (rec.status == SubmitRecord.Status.OK) {
foundOk = true;
if (resultSubmitRecord.status == SubmitRecord.Status.OK) {
break;
}
}
Collections.reverse(out);
// This transformation is required to adapt Prolog's behavior to the way Gerrit handles
// SubmitRecords, as defined in the SubmitRecord#allRecordsOK method.
// When several rules are defined in Prolog, they are all matched to a SubmitRecord. We want
// the change to be submittable when at least one result is OK.
if (foundOk) {
for (SubmitRecord record : out) {
record.status = SubmitRecord.Status.OK;
}
}
return out;
Collections.reverse(resultSubmitRecord.labels);
return resultSubmitRecord;
}
@VisibleForTesting
@@ -299,7 +288,7 @@ public class PrologRuleEvaluator {
return VALID_LABEL_MATCHER.retainFrom(name);
}
private List<SubmitRecord> invalidResult(Term rule, Term record, String reason) {
private SubmitRecord invalidResult(Term rule, Term record, String reason) {
return ruleError(
String.format(
"Submit rule %s for change %s of %s output invalid result: %s%s",
@@ -310,15 +299,15 @@ public class PrologRuleEvaluator {
(reason == null ? "" : ". Reason: " + reason)));
}
private List<SubmitRecord> invalidResult(Term rule, Term record) {
private SubmitRecord invalidResult(Term rule, Term record) {
return invalidResult(rule, record, null);
}
private List<SubmitRecord> ruleError(String err) {
private SubmitRecord ruleError(String err) {
return ruleError(err, null);
}
private List<SubmitRecord> ruleError(String err, Exception e) {
private SubmitRecord ruleError(String err, Exception e) {
if (opts.logErrors()) {
logger.atSevere().withCause(e).log(err);
return defaultRuleError();

13
java/com/google/gerrit/server/rules/SubmitRule.java
View File

@@ -16,13 +16,13 @@ package com.google.gerrit.server.rules;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import com.google.gerrit.server.query.change.ChangeData;
import java.util.Collection;
import java.util.Optional;
/**
* Allows plugins to decide whether a change is ready to be submitted or not.
*
* <p>For a given {@link ChangeData}, each plugin is called and returns a {@link Collection} of
* {@link SubmitRecord}. This collection can be empty, or contain one or several values.
* <p>For a given {@link ChangeData}, each plugin is called and returns a {@link Optional} of {@link
* SubmitRecord}.
*
* <p>A Change can only be submitted if all the plugins give their consent.
*
@@ -38,6 +38,9 @@ import java.util.Collection;
*/
@ExtensionPoint
public interface SubmitRule {
/** Returns a {@link Collection} of {@link SubmitRecord} status for the change. */
Collection<SubmitRecord> evaluate(ChangeData changeData);
/**
* Returns a {@link Optional} of {@link SubmitRecord} status for the change. {@code
* Optional#empty()} if the SubmitRule was a no-op.
*/
Optional<SubmitRecord> evaluate(ChangeData changeData);
}

6
javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java
View File

@@ -30,7 +30,7 @@ import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Module;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Optional;
import org.junit.Test;
public class ChangeSubmitRequirementIT extends AbstractDaemonTest {
@@ -66,12 +66,12 @@ public class ChangeSubmitRequirementIT extends AbstractDaemonTest {
private static class CustomSubmitRule implements SubmitRule {
@Override
public Collection<SubmitRecord> evaluate(ChangeData changeData) {
public Optional<SubmitRecord> evaluate(ChangeData changeData) {
SubmitRecord record = new SubmitRecord();
record.labels = new ArrayList<>();
record.status = SubmitRecord.Status.NOT_READY;
record.requirements = ImmutableList.of(req);
return ImmutableList.of(record);
return Optional.of(record);
}
}
}

8
javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
View File

@@ -262,8 +262,8 @@ public class SubmitTypeRuleIT extends AbstractDaemonTest {
TestSubmitRuleInput in = new TestSubmitRuleInput();
in.rule = "invalid prolog rule";
// We have no rules.pl by default. The fact that the default rules are showing up here is a bug.
List<TestSubmitRuleInfo> response = gApi.changes().id(changeId).current().testSubmitRule(in);
assertThat(response).containsExactly(invalidPrologRuleInfo());
TestSubmitRuleInfo response = gApi.changes().id(changeId).current().testSubmitRule(in);
assertThat(response).isEqualTo(invalidPrologRuleInfo());
}
@Test
@@ -274,8 +274,8 @@ public class SubmitTypeRuleIT extends AbstractDaemonTest {
TestSubmitRuleInput in = new TestSubmitRuleInput();
in.rule = "invalid prolog rule";
List<TestSubmitRuleInfo> response = gApi.changes().id(changeId).current().testSubmitRule(in);
assertThat(response).containsExactly(invalidPrologRuleInfo());
TestSubmitRuleInfo response = gApi.changes().id(changeId).current().testSubmitRule(in);
assertThat(response).isEqualTo(invalidPrologRuleInfo());
}
private static TestSubmitRuleInfo invalidPrologRuleInfo() {

6
javatests/com/google/gerrit/acceptance/rest/TraceIT.java
View File

@@ -52,8 +52,8 @@ import com.google.gerrit.server.validators.ProjectCreationValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.SortedMap;
import java.util.SortedSet;
import org.apache.http.message.BasicHeader;
@@ -679,7 +679,7 @@ public class TraceIT extends AbstractDaemonTest {
boolean failOnce;
@Override
public Collection<SubmitRecord> evaluate(ChangeData changeData) {
public Optional<SubmitRecord> evaluate(ChangeData changeData) {
if (failOnce) {
failOnce = false;
throw new IllegalStateException("forced failure from test");
@@ -691,7 +691,7 @@ public class TraceIT extends AbstractDaemonTest {
SubmitRecord submitRecord = new SubmitRecord();
submitRecord.status = SubmitRecord.Status.OK;
return ImmutableList.of(submitRecord);
return Optional.of(submitRecord);
}
}

17
javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.server.rules;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
@@ -24,8 +25,8 @@ import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitRequirement;
import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
import com.google.inject.Inject;
import java.util.Collection;
import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.junit.Test;
@@ -41,10 +42,10 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange();
approve(r.getChangeId());
Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
Optional<SubmitRecord> submitRecord = rule.evaluate(r.getChange());
assertThat(submitRecords).hasSize(1);
SubmitRecord result = submitRecords.iterator().next();
assertThat(submitRecord).isPresent();
SubmitRecord result = submitRecord.get();
assertThat(result.status).isEqualTo(SubmitRecord.Status.NOT_READY);
assertThat(result.labels).isNotEmpty();
assertThat(result.requirements)
@@ -67,8 +68,8 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
// Approve as admin
approve(r.getChangeId());
Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
assertThat(submitRecords).isEmpty();
Optional<SubmitRecord> submitRecord = rule.evaluate(r.getChange());
assertThat(submitRecord).isEmpty();
}
@Test
@@ -78,8 +79,8 @@ public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange();
approve(r.getChangeId());
Collection<SubmitRecord> submitRecords = rule.evaluate(r.getChange());
assertThat(submitRecords).isEmpty();
Optional<SubmitRecord> submitRecord = rule.evaluate(r.getChange());
assertThat(submitRecord).isEmpty();
}
private void enableRule(String labelName, boolean newState) throws Exception {

27
javatests/com/google/gerrit/acceptance/server/rules/PrologRuleEvaluatorIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.server.rules;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -30,7 +31,6 @@ import com.googlecode.prolog_cafe.lang.IntegerTerm;
import com.googlecode.prolog_cafe.lang.StructureTerm;
import com.googlecode.prolog_cafe.lang.Term;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
@@ -46,9 +46,9 @@ public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
StructureTerm labels = new StructureTerm("label", verifiedLabel);
List<Term> terms = ImmutableList.of(makeTerm("ok", labels));
Collection<SubmitRecord> records = evaluator.resultsToSubmitRecord(null, terms);
SubmitRecord record = evaluator.resultsToSubmitRecord(null, terms);
assertThat(records).hasSize(1);
assertThat(record.status).isEqualTo(SubmitRecord.Status.OK);
}
/**
@@ -113,23 +113,16 @@ public class PrologRuleEvaluatorIT extends AbstractDaemonTest {
terms.add(makeTerm("not_ready", makeLabels(label3)));
// When
List<SubmitRecord> records = evaluator.resultsToSubmitRecord(null, terms);
SubmitRecord record = evaluator.resultsToSubmitRecord(null, terms);
// assert that
SubmitRecord record1Expected = new SubmitRecord();
record1Expected.status = SubmitRecord.Status.OK;
record1Expected.labels = new ArrayList<>();
record1Expected.labels.add(submitRecordLabel2);
SubmitRecord expectedRecord = new SubmitRecord();
expectedRecord.status = SubmitRecord.Status.OK;
expectedRecord.labels = new ArrayList<>();
expectedRecord.labels.add(submitRecordLabel2);
expectedRecord.labels.add(submitRecordLabel3);
SubmitRecord record2Expected = new SubmitRecord();
record2Expected.status = SubmitRecord.Status.OK;
record2Expected.labels = new ArrayList<>();
record2Expected.labels.add(submitRecordLabel3);
assertThat(records).hasSize(2);
assertThat(records.get(0)).isEqualTo(record1Expected);
assertThat(records.get(1)).isEqualTo(record2Expected);
assertThat(record).isEqualTo(expectedRecord);
}
private static Term makeTerm(String status, StructureTerm labels) {