From ead26b44dfacc4e7fcf82d3244ded387cb07065c Mon Sep 17 00:00:00 2001 From: Maxime Guerreiro Date: Fri, 9 Mar 2018 18:00:17 +0100 Subject: [PATCH] Add tests for Prolog's handling of SubmitRecords Prolog handles the results of the defined `submit_rule` in a specific way: once an OK record is found, processing of the other SubmitRecords is stopped. This leads to weird situations, for instance if we have a *not_ready* record _before_ the *ok* one. Add a test to ensure the behavior isn't changed inadvertently and to document it with simple and reproducible test cases. Change-Id: I06d054a47c47e05d3e453e4520474cc2d26c98e4 --- .../server/project/SubmitRuleEvaluator.java | 2 +- .../gerrit/acceptance/server/project/BUILD | 3 + .../project/SubmitRulesEvaluatorIT.java | 156 ++++++++++++++++++ .../gerrit/common/data/SubmitRecordTest.java | 52 ++++++ 4 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 javatests/com/google/gerrit/acceptance/server/project/SubmitRulesEvaluatorIT.java create mode 100644 javatests/com/google/gerrit/common/data/SubmitRecordTest.java diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index c1da015d40..7f7ba652af 100644 --- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java @@ -252,7 +252,7 @@ public class SubmitRuleEvaluator { * output. Later after the loop the out collection is reversed to restore it to the original * ordering. */ - private List resultsToSubmitRecord(Term submitRule, List results) { + public List resultsToSubmitRecord(Term submitRule, List results) { List out = new ArrayList<>(results.size()); for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) { Term submitRecord = results.get(resultIdx); diff --git a/javatests/com/google/gerrit/acceptance/server/project/BUILD b/javatests/com/google/gerrit/acceptance/server/project/BUILD index efa1cdb48f..b834f2ef48 100644 --- a/javatests/com/google/gerrit/acceptance/server/project/BUILD +++ b/javatests/com/google/gerrit/acceptance/server/project/BUILD @@ -4,4 +4,7 @@ acceptance_tests( srcs = glob(["*IT.java"]), group = "server_project", labels = ["server"], + deps = [ + "@prolog_runtime//jar", + ], ) diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRulesEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRulesEvaluatorIT.java new file mode 100644 index 0000000000..e7f5b2dddb --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRulesEvaluatorIT.java @@ -0,0 +1,156 @@ +// Copyright (C) 2018 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.acceptance.server.project; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.common.data.SubmitRecord; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.project.SubmitRuleEvaluator; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.testing.TestChanges; +import com.google.inject.Inject; +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.junit.Test; + +public class SubmitRulesEvaluatorIT extends AbstractDaemonTest { + @Inject private SubmitRuleEvaluator.Factory evaluatorFactory; + + @Test + public void convertsPrologToSubmitRecord() { + SubmitRuleEvaluator evaluator = makeEvaluator(); + + StructureTerm verifiedLabel = makeLabel("Verified", "may"); + StructureTerm labels = new StructureTerm("label", verifiedLabel); + + List terms = ImmutableList.of(makeTerm("ok", labels)); + Collection records = evaluator.resultsToSubmitRecord(null, terms); + + assertThat(records).hasSize(1); + } + + /** + * The Prolog behavior is everything but intuitive. Several submit_rules can be defined, and each + * will provide a different SubmitRecord answer when queried. The current implementation stops + * parsing the Prolog terms into SubmitRecord objects once it finds an OK record. This might lead + * to tangling results, as reproduced by this test. + * + *

Let's consider this rules.pl file (equivalent to the code in this test) + * + * 

{@code
+   * submit_rule(submit(R)) :-
+   *     gerrit:uploader(U),
+   *     R = label('Verified', reject(U)).
+   *
+   * submit_rule(submit(CR, V)) :-
+   *     gerrit:uploader(U),
+   *     V = label('Code-Review', ok(U)).
+   *
+   * submit_rule(submit(R)) :-
+   *     gerrit:uploader(U),
+   *     R = label('Any-Label-Name', reject(U)).
+   * }
+ * + * The first submit_rule always fails because the Verified label is rejected. + * + *

The second submit_rule is always valid, and provides two labels: OK and Code-Review. + * + *

The third submit_rule always fails because the Any-Label-Name label is rejected. + * + *

In this case, the last two SubmitRecords are used, the first one is discarded. + */ + @Test + public void abortsEarlyWithOkayRecord() { + SubmitRuleEvaluator evaluator = makeEvaluator(); + + SubmitRecord.Label submitRecordLabel1 = new SubmitRecord.Label(); + submitRecordLabel1.label = "Verified"; + submitRecordLabel1.status = SubmitRecord.Label.Status.REJECT; + submitRecordLabel1.appliedBy = admin.id; + + SubmitRecord.Label submitRecordLabel2 = new SubmitRecord.Label(); + submitRecordLabel2.label = "Code-Review"; + submitRecordLabel2.status = SubmitRecord.Label.Status.OK; + submitRecordLabel2.appliedBy = admin.id; + + SubmitRecord.Label submitRecordLabel3 = new SubmitRecord.Label(); + submitRecordLabel3.label = "Any-Label-Name"; + submitRecordLabel3.status = SubmitRecord.Label.Status.REJECT; + submitRecordLabel3.appliedBy = user.id; + + List terms = new ArrayList<>(); + + StructureTerm label1 = makeLabel(submitRecordLabel1.label, "reject", admin); + + StructureTerm label2 = makeLabel(submitRecordLabel2.label, "ok", admin); + + StructureTerm label3 = makeLabel(submitRecordLabel3.label, "reject", user); + + terms.add(makeTerm("not_ready", makeLabels(label1))); + terms.add(makeTerm("ok", makeLabels(label2))); + terms.add(makeTerm("not_ready", makeLabels(label3))); + + // When + List records = evaluator.resultsToSubmitRecord(null, terms); + + // assert that + SubmitRecord record1Expected = new SubmitRecord(); + record1Expected.status = SubmitRecord.Status.OK; + record1Expected.labels = new ArrayList<>(); + record1Expected.labels.add(submitRecordLabel2); + + SubmitRecord record2Expected = new SubmitRecord(); + record2Expected.status = SubmitRecord.Status.NOT_READY; + record2Expected.labels = new ArrayList<>(); + record2Expected.labels.add(submitRecordLabel3); + + assertThat(records).hasSize(2); + + assertThat(records.get(0)).isEqualTo(record1Expected); + assertThat(records.get(1)).isEqualTo(record2Expected); + } + + private static Term makeTerm(String status, StructureTerm labels) { + return new StructureTerm(status, labels); + } + + private static StructureTerm makeLabel(String name, String status) { + return new StructureTerm("label", new StructureTerm(name), new StructureTerm(status)); + } + + private static StructureTerm makeLabel(String name, String status, TestAccount account) { + StructureTerm user = new StructureTerm("user", new IntegerTerm(account.id.get())); + return new StructureTerm("label", new StructureTerm(name), new StructureTerm(status, user)); + } + + private static StructureTerm makeLabels(StructureTerm... labels) { + return new StructureTerm("label", labels); + } + + private SubmitRuleEvaluator makeEvaluator() { + ChangeData cd = ChangeData.createForTest(project, new Change.Id(1), 1); + cd.setChange(TestChanges.newChange(project, admin.id)); + + return evaluatorFactory.create(cd); + } +} diff --git a/javatests/com/google/gerrit/common/data/SubmitRecordTest.java b/javatests/com/google/gerrit/common/data/SubmitRecordTest.java new file mode 100644 index 0000000000..f989285253 --- /dev/null +++ b/javatests/com/google/gerrit/common/data/SubmitRecordTest.java @@ -0,0 +1,52 @@ +// Copyright (C) 2018 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.common.data; + +import static com.google.common.truth.Truth.assertThat; + +import java.util.ArrayList; +import java.util.Collection; +import org.junit.Test; + +public class SubmitRecordTest { + + private static SubmitRecord OK_RECORD; + private static SubmitRecord NOT_READY_RECORD; + + static { + OK_RECORD = new SubmitRecord(); + OK_RECORD.status = SubmitRecord.Status.OK; + + NOT_READY_RECORD = new SubmitRecord(); + NOT_READY_RECORD.status = SubmitRecord.Status.NOT_READY; + } + + @Test + public void findOkRecord() { + Collection submitRecords = new ArrayList<>(); + submitRecords.add(NOT_READY_RECORD); + submitRecords.add(OK_RECORD); + + assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isTrue(); + } + + @Test + public void emptyResultIfInvalid() { + Collection submitRecords = new ArrayList<>(); + submitRecords.add(NOT_READY_RECORD); + + assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isFalse(); + } +}