opendev/gerrit
Code Issues Proposed changes

Add tests for Prolog's handling of SubmitRecords

Browse Source 
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
This commit is contained in:
Maxime Guerreiro
2018-03-09 18:00:17 +01:00
parent 227eb5776f
commit ead26b44df
4 changed files with 212 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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<SubmitRecord> resultsToSubmitRecord(Term submitRule, List<Term> results) {
public List<SubmitRecord> resultsToSubmitRecord(Term submitRule, List<Term> results) {
List<SubmitRecord> out = new ArrayList<>(results.size());
for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) {
Term submitRecord = results.get(resultIdx);

3
javatests/com/google/gerrit/acceptance/server/project/BUILD
View File

@@ -4,4 +4,7 @@ acceptance_tests(
srcs = glob(["*IT.java"]),
group = "server_project",
labels = ["server"],
deps = [
"@prolog_runtime//jar",
],
)

156
javatests/com/google/gerrit/acceptance/server/project/SubmitRulesEvaluatorIT.java Normal file
View File

@@ -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<Term> terms = ImmutableList.of(makeTerm("ok", labels));
Collection<SubmitRecord> 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.
*
* <p>Let's consider this rules.pl file (equivalent to the code in this test)
*
* <pre>{@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)).
* }</pre>
*
* The first submit_rule always fails because the Verified label is rejected.
*
* <p>The second submit_rule is always valid, and provides two labels: OK and Code-Review.
*
* <p>The third submit_rule always fails because the Any-Label-Name label is rejected.
*
* <p>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<Term> 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<SubmitRecord> 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);
}
}

52
javatests/com/google/gerrit/common/data/SubmitRecordTest.java Normal file
View File

@@ -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<SubmitRecord> submitRecords = new ArrayList<>();
submitRecords.add(NOT_READY_RECORD);
submitRecords.add(OK_RECORD);
assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isTrue();
}
@Test
public void emptyResultIfInvalid() {
Collection<SubmitRecord> submitRecords = new ArrayList<>();
submitRecords.add(NOT_READY_RECORD);
assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isFalse();
}
}