opendev/gerrit
Code Issues Proposed changes

Execute the diff tests both for intraline and non-intraline

Browse Source 
As PolyGerrit always retrieves the diffs with intraline set to true, we
should make sure to have a good test coverage for that case.

Change-Id: I5e3990670a301660166c3f194feff5a09c23014b
This commit is contained in:
Alice Kober-Sotzek
2017-07-05 13:30:23 +02:00
parent 3928de5cb7
commit 704ba94fae
3 changed files with 68 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

158
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.revision;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.extensions.common.DiffInfoSubject.assertThat;
import static com.google.gerrit.extensions.common.FileInfoSubject.assertThat;
import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
@@ -26,11 +27,13 @@ import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.common.ChangeType;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.testutil.ConfigSuite;
import java.awt.image.BufferedImage;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
@@ -44,6 +47,7 @@ import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import javax.imageio.ImageIO;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -51,6 +55,10 @@ import org.junit.Before;
import org.junit.Test;
public class RevisionDiffIT extends AbstractDaemonTest {
// @RunWith(Parameterized.class) can't be used as AbstractDaemonTest is annotated with another
// runner. Using different configs is a workaround to achieve the same.
private static final String TEST_PARAMETER_MARKER = "test_only_parameter";
private static final String CURRENT = "current";
private static final String FILE_NAME = "some_file.txt";
private static final String FILE_NAME2 = "another_file.txt";
private static final String FILE_CONTENT =
@@ -59,12 +67,22 @@ public class RevisionDiffIT extends AbstractDaemonTest {
.collect(Collectors.joining());
private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n";
private boolean intraline;
private ObjectId commit1;
private String changeId;
private String initialPatchSetId;
@ConfigSuite.Config
public static Config intralineConfig() {
Config config = new Config();
config.setBoolean(TEST_PARAMETER_MARKER, null, "intraline", true);
return config;
}
@Before
public void setUp() throws Exception {
intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false);
ObjectId headCommit = testRepo.getRepository().resolve("HEAD");
commit1 =
addCommit(headCommit, ImmutableMap.of(FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2));
@@ -76,6 +94,9 @@ public class RevisionDiffIT extends AbstractDaemonTest {
@Test
public void diff() throws Exception {
// The assertions assume that intraline is false.
assume().that(intraline).isFalse();
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
@@ -91,7 +112,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
DiffInfo diff = gApi.changes().id(changeId).current().file(FILE_NAME).diff();
DiffInfo diff = getDiffRequest(changeId, CURRENT, FILE_NAME).get();
assertThat(diff.metaA.lines).isEqualTo(100);
assertThat(diff.metaB).isNull();
}
@@ -150,21 +171,21 @@ public class RevisionDiffIT extends AbstractDaemonTest {
DiffInfo diff;
// automerge
diff = gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).file("foo").diff();
diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "foo").get();
assertThat(diff.metaA.lines).isEqualTo(5);
assertThat(diff.metaB.lines).isEqualTo(1);
diff = gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).file("bar").diff();
diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "bar").get();
assertThat(diff.metaA.lines).isEqualTo(5);
assertThat(diff.metaB.lines).isEqualTo(1);
// parent 1
diff = gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).file("bar").diff(1);
diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "bar").withParent(1).get();
assertThat(diff.metaA.lines).isEqualTo(1);
assertThat(diff.metaB.lines).isEqualTo(1);
// parent 2
diff = gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).file("foo").diff(2);
diff = getDiffRequest(r.getChangeId(), r.getCommit().name(), "foo").withParent(2).get();
assertThat(diff.metaA.lines).isEqualTo(1);
assertThat(diff.metaB.lines).isEqualTo(1);
}
@@ -280,7 +301,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
assertThat(diffInfo).content().element(0).isDueToRebase();
@@ -314,7 +335,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(49);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 50");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line fifty");
@@ -349,7 +370,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
assertThat(diffInfo).content().element(0).isNotDueToRebase();
@@ -391,7 +412,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification2);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(previousPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(41);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 40");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line forty");
@@ -421,7 +442,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().isNull();
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line zero");
assertThat(diffInfo).content().element(0).isNotDueToRebase();
@@ -464,7 +485,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification2);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(previousPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(37);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 40");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line forty");
@@ -494,7 +515,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
assertThat(diffInfo).content().element(0).linesOfB().isNull();
assertThat(diffInfo).content().element(0).isNotDueToRebase();
@@ -525,7 +546,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(39);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 40");
assertThat(diffInfo)
@@ -557,7 +578,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(38);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 39", "Line 40");
assertThat(diffInfo)
@@ -589,7 +610,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(40);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 41", "Line 42");
assertThat(diffInfo)
@@ -619,7 +640,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(38);
assertThat(diffInfo)
.content()
@@ -657,7 +678,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(40);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 41", "Line 42");
assertThat(diffInfo)
@@ -690,7 +711,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
assertThat(diffInfo).content().element(0).isDueToRebase();
@@ -738,80 +759,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
.replace("Line 60\n", "Line sixty\n"));
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(previousPatchSetId);
assertThat(diffInfo).content().element(0).commonLines().hasSize(1);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 2");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line two");
assertThat(diffInfo).content().element(1).isDueToRebase();
assertThat(diffInfo).content().element(2).commonLines().hasSize(4);
assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line seven");
assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line 7");
assertThat(diffInfo).content().element(3).isNotDueToRebase();
assertThat(diffInfo).content().element(4).commonLines().hasSize(1);
assertThat(diffInfo).content().element(5).linesOfA().containsExactly("Line 9");
assertThat(diffInfo).content().element(5).linesOfB().containsExactly("Line nine");
assertThat(diffInfo).content().element(5).isNotDueToRebase();
assertThat(diffInfo).content().element(6).commonLines().hasSize(8);
assertThat(diffInfo).content().element(7).linesOfA().containsExactly("Line 18", "Line 19");
assertThat(diffInfo)
.content()
.element(7)
.linesOfB()
.containsExactly("Line eighteen", "Line nineteen");
assertThat(diffInfo).content().element(7).isDueToRebase();
assertThat(diffInfo).content().element(8).commonLines().hasSize(29);
assertThat(diffInfo).content().element(9).linesOfA().containsExactly("Line 50");
assertThat(diffInfo).content().element(9).linesOfB().containsExactly("Line fifty");
assertThat(diffInfo).content().element(9).isDueToRebase();
assertThat(diffInfo).content().element(10).commonLines().hasSize(9);
assertThat(diffInfo).content().element(11).linesOfA().containsExactly("Line 60");
assertThat(diffInfo).content().element(11).linesOfB().containsExactly("Line sixty");
assertThat(diffInfo).content().element(11).isNotDueToRebase();
assertThat(diffInfo).content().element(12).commonLines().hasSize(40);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(3);
assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(3);
}
@Test
public void multipleRebaseEditsMixedWithRegularEditsCanBeIdentified_WithIntraline()
throws Exception {
addModifiedPatchSet(
changeId,
FILE_NAME,
fileContent -> fileContent.replace("Line 7\n", "Line seven\n").replace("Line 24\n", ""));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
ObjectId commit2 =
addCommit(
commit1,
FILE_NAME,
FILE_CONTENT
.replace("Line 2\n", "Line two\n")
.replace("Line 18\nLine 19\n", "Line eighteen\nLine nineteen\n")
.replace("Line 50\n", "Line fifty\n"));
rebaseChangeOn(changeId, commit2);
addModifiedPatchSet(
changeId,
FILE_NAME,
fileContent ->
fileContent
.replace("Line seven\n", "Line 7\n")
.replace("Line 9\n", "Line nine\n")
.replace("Line 60\n", "Line sixty\n"));
DiffInfo diffInfo =
gApi.changes()
.id(changeId)
.current()
.file(FILE_NAME)
.diffRequest()
.withBase(previousPatchSetId)
.withIntraline(true)
.get();
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(1);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 2");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line two");
@@ -862,7 +810,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
rebaseChangeOn(changeId, commit2);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(previousPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
assertThat(diffInfo).changeType().isEqualTo(ChangeType.DELETED);
assertThat(diffInfo).content().element(0).linesOfA().hasSize(100);
assertThat(diffInfo).content().element(0).linesOfB().isNull();
@@ -884,7 +832,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
changeId, newFilePath, fileContent -> fileContent.replace("1st line\n", "First line\n"));
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(newFilePath).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, newFilePath).withBase(initialPatchSetId).get();
assertThat(diffInfo).changeType().isEqualTo(ChangeType.ADDED);
assertThat(diffInfo).content().element(0).linesOfA().isNull();
assertThat(diffInfo).content().element(0).linesOfB().hasSize(3);
@@ -909,7 +857,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, renamedFilePath, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(renamedFilePath).diff(initialPatchSetId);
getDiffRequest(changeId, CURRENT, renamedFilePath).withBase(initialPatchSetId).get();
assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
assertThat(diffInfo).content().element(0).isDueToRebase();
@@ -947,7 +895,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, renamedFilePath, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(renamedFilePath).diff(previousPatchSetId);
getDiffRequest(changeId, CURRENT, renamedFilePath).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(4);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 5");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line five");
@@ -991,7 +939,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
DiffInfo diffInfo =
gApi.changes().id(changeId).current().file(FILE_NAME).diff(previousPatchSetId);
getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(4);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line five");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line 5");
@@ -1062,11 +1010,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
String currentPatchSetId = gApi.changes().id(changeId).get().currentRevision;
DiffInfo diffInfo =
gApi.changes()
.id(changeId)
.revision(initialPatchSetId)
.file(FILE_NAME)
.diff(currentPatchSetId);
getDiffRequest(changeId, initialPatchSetId, FILE_NAME).withBase(currentPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(4);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line five");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line 5");
@@ -1212,4 +1156,14 @@ public class RevisionDiffIT extends AbstractDaemonTest {
ImageIO.write(bufferedImage, "png", byteArrayOutputStream);
return byteArrayOutputStream.toByteArray();
}
private FileApi.DiffRequest getDiffRequest(String changeId, String revisionId, String fileName)
throws Exception {
return gApi.changes()
.id(changeId)
.revision(revisionId)
.file(fileName)
.diffRequest()
.withIntraline(intraline);
}
}

11
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FileApi.java
View File

@@ -19,6 +19,7 @@ import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
import java.util.OptionalInt;
public interface FileApi {
BinaryResult content() throws RestApiException;
@@ -43,6 +44,7 @@ public interface FileApi {
private Integer context;
private Boolean intraline;
private Whitespace whitespace;
private OptionalInt parent = OptionalInt.empty();
public abstract DiffInfo get() throws RestApiException;
@@ -66,6 +68,11 @@ public interface FileApi {
return this;
}
public DiffRequest withParent(int parent) {
this.parent = OptionalInt.of(parent);
return this;
}
public String getBase() {
return base;
}
@@ -81,6 +88,10 @@ public interface FileApi {
public Whitespace getWhitespace() {
return whitespace;
}
public OptionalInt getParent() {
return parent;
}
}
/**

1
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/FileApiImpl.java
View File

@@ -101,6 +101,7 @@ class FileApiImpl implements FileApi {
if (r.getWhitespace() != null) {
getDiff.setWhitespace(r.getWhitespace());
}
r.getParent().ifPresent(getDiff::setParent);
try {
return getDiff.apply(file).value();
} catch (Exception e) {