Merge "Add edge cases tests and fixes for getFixPreview"
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index 92df794..d88b626 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -28,6 +28,7 @@
import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.server.fixes.FixCalculator;
import com.google.gerrit.server.mime.FileTypeRegistry;
import com.google.gerrit.server.patch.DiffContentCalculator.DiffCalculatorResult;
@@ -116,9 +117,12 @@
PatchScript toPatchScript(
Repository git, ObjectId baseId, String fileName, List<FixReplacement> fixReplacements)
- throws IOException, ResourceConflictException {
+ throws IOException, ResourceConflictException, ResourceNotFoundException {
SidesResolver sidesResolver = new SidesResolver(git, ComparisonType.againstOtherPatchSet());
PatchSide a = resolveSideA(git, sidesResolver, fileName, baseId);
+ if (a.mode == FileMode.MISSING) {
+ throw new ResourceNotFoundException(String.format("File %s not found", fileName));
+ }
FixCalculator.FixResult fixResult = FixCalculator.calculateFix(a.src, fixReplacements);
PatchSide b =
new PatchSide(
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java b/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
index 2e39607..2f5458c 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java
@@ -25,6 +25,7 @@
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.server.git.LargeObjectException;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -93,7 +94,7 @@
@Override
public PatchScript call()
throws LargeObjectException, AuthException, InvalidChangeOperationException, IOException,
- PermissionBackendException {
+ PermissionBackendException, ResourceNotFoundException {
try {
permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
@@ -108,7 +109,7 @@
return createPatchScript();
}
- private PatchScript createPatchScript() throws LargeObjectException {
+ private PatchScript createPatchScript() throws LargeObjectException, ResourceNotFoundException {
checkState(patchSet.id().get() != 0, "edit not supported for left side");
PatchScriptBuilder b = newBuilder();
try {
diff --git a/java/com/google/gerrit/server/restapi/change/GetFixPreview.java b/java/com/google/gerrit/server/restapi/change/GetFixPreview.java
index 0666756..b39424c 100644
--- a/java/com/google/gerrit/server/restapi/change/GetFixPreview.java
+++ b/java/com/google/gerrit/server/restapi/change/GetFixPreview.java
@@ -109,7 +109,7 @@
String fileName,
ImmutableList<FixReplacement> fixReplacements)
throws PermissionBackendException, AuthException, LargeObjectException,
- InvalidChangeOperationException, IOException {
+ InvalidChangeOperationException, IOException, ResourceNotFoundException {
PatchScriptFactoryForAutoFix psf =
patchScriptFactoryFactory.create(
git, notes, fileName, patchSet, fixReplacements, DiffPreferencesInfo.defaults());
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
index 6b92c16..56d164b 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -28,6 +28,8 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -53,20 +55,22 @@
import java.util.Objects;
import java.util.Optional;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
public class RobotCommentsIT extends AbstractDaemonTest {
@Inject private TestCommentHelper testCommentHelper;
private static final String PLAIN_TEXT_CONTENT_TYPE = "text/plain";
+ private static final String GERRIT_COMMIT_MESSAGE_TYPE = "text/x-gerrit-commit-message";
private static final String FILE_NAME = "file_to_fix.txt";
private static final String FILE_NAME2 = "another_file_to_fix.txt";
+ private static final String FILE_NAME3 = "file_without_newline_at_end.txt";
private static final String FILE_CONTENT =
"First line\nSecond line\nThird line\nFourth line\nFifth line\nSixth line"
+ "\nSeventh line\nEighth line\nNinth line\nTenth line\n";
private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n";
+ private static final String FILE_CONTENT3 = "1st line\n2nd line";
private String changeId;
private String commitId;
@@ -81,7 +85,8 @@
admin.newIdent(),
testRepo,
"Provide files which can be used for fixes",
- ImmutableMap.of(FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2));
+ ImmutableMap.of(
+ FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2, FILE_NAME3, FILE_CONTENT3));
PushOneCommit.Result changeResult = push.to("refs/for/master");
changeId = changeResult.getChangeId();
commitId = changeResult.getCommit().getName();
@@ -994,20 +999,76 @@
}
@Test
- @Ignore
- public void getFixPreviewForNonExistingFile() throws Exception {
- // Not implemented yet.
- fixReplacementInfo.path = "a_non_existent_file.txt";
- fixReplacementInfo.range = createRange(1, 0, 2, 0);
- fixReplacementInfo.replacement = "Modified content\n";
+ public void getFixPreviewForCommitMsg() throws Exception {
+ updateCommitMessage(
+ changeId, "Commit title\n\nCommit message line 1\nLine 2\nLine 3\nLast line\n");
+ FixReplacementInfo commitMsgReplacement = new FixReplacementInfo();
+ commitMsgReplacement.path = Patch.COMMIT_MSG;
+ // The test assumes that the first 5 lines is a header.
+ // Line 10 has content "Line 2"
+ commitMsgReplacement.range = createRange(10, 0, 11, 0);
+ commitMsgReplacement.replacement = "New content\n";
- testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
+ FixSuggestionInfo commitMsgSuggestionInfo = createFixSuggestionInfo(commitMsgReplacement);
+ RobotCommentInput commitMsgRobotCommentInput =
+ TestCommentHelper.createRobotCommentInput(Patch.COMMIT_MSG, commitMsgSuggestionInfo);
+ testCommentHelper.addRobotComment(changeId, commitMsgRobotCommentInput);
+
+ List<RobotCommentInfo> robotCommentInfos = getRobotComments();
+
+ List<String> fixIds = getFixIds(robotCommentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ Map<String, DiffInfo> fixPreview = gApi.changes().id(changeId).current().getFixPreview(fixId);
+ assertThat(fixPreview).hasSize(1);
+ assertThat(fixPreview).containsKey(Patch.COMMIT_MSG);
+
+ DiffInfo diff = fixPreview.get(Patch.COMMIT_MSG);
+ assertThat(diff).metaA().name().isEqualTo(Patch.COMMIT_MSG);
+ assertThat(diff).metaA().contentType().isEqualTo(GERRIT_COMMIT_MESSAGE_TYPE);
+ assertThat(diff).metaB().name().isEqualTo(Patch.COMMIT_MSG);
+ assertThat(diff).metaB().contentType().isEqualTo(GERRIT_COMMIT_MESSAGE_TYPE);
+
+ assertThat(diff).content().element(0).commonLines().hasSize(9);
+ // Header has a dynamic content, do not check it
+ assertThat(diff).content().element(0).commonLines().element(6).isEqualTo("Commit title");
+ assertThat(diff).content().element(0).commonLines().element(7).isEqualTo("");
+ assertThat(diff)
+ .content()
+ .element(0)
+ .commonLines()
+ .element(8)
+ .isEqualTo("Commit message line 1");
+ assertThat(diff).content().element(1).linesOfA().containsExactly("Line 2");
+ assertThat(diff).content().element(1).linesOfB().containsExactly("New content");
+ assertThat(diff).content().element(2).commonLines().containsExactly("Line 3", "Last line", "");
+ }
+
+ private void updateCommitMessage(String changeId, String newCommitMessage) throws Exception {
+ gApi.changes().id(changeId).edit().create();
+ gApi.changes().id(changeId).edit().modifyCommitMessage(newCommitMessage);
+ PublishChangeEditInput publishInput = new PublishChangeEditInput();
+ gApi.changes().id(changeId).edit().publish(publishInput);
+ }
+
+ @Test
+ public void getFixPreviewForNonExistingFile() throws Exception {
+ FixReplacementInfo replacement = new FixReplacementInfo();
+ replacement.path = "a_non_existent_file.txt";
+ replacement.range = createRange(1, 0, 2, 0);
+ replacement.replacement = "Modified content\n";
+
+ FixSuggestionInfo fixSuggestion = createFixSuggestionInfo(replacement);
+ RobotCommentInput commentInput =
+ TestCommentHelper.createRobotCommentInput(FILE_NAME2, fixSuggestion);
+ testCommentHelper.addRobotComment(changeId, commentInput);
+
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
List<String> fixIds = getFixIds(robotCommentInfos);
String fixId = Iterables.getOnlyElement(fixIds);
assertThrows(
- BadRequestException.class,
+ ResourceNotFoundException.class,
() -> gApi.changes().id(changeId).current().getFixPreview(fixId));
}
@@ -1121,6 +1182,41 @@
assertThat(diff2).content().element(2).linesOfB().isNull();
}
+ @Test
+ public void getFixPreviewAddNewLineAtEnd() throws Exception {
+ FixReplacementInfo replacement = new FixReplacementInfo();
+ replacement.path = FILE_NAME3;
+ replacement.range = createRange(2, 8, 2, 8);
+ replacement.replacement = "\n";
+
+ FixSuggestionInfo fixSuggestion = createFixSuggestionInfo(replacement);
+ RobotCommentInput commentInput =
+ TestCommentHelper.createRobotCommentInput(FILE_NAME3, fixSuggestion);
+ testCommentHelper.addRobotComment(changeId, commentInput);
+
+ List<RobotCommentInfo> robotCommentInfos = getRobotComments();
+
+ List<String> fixIds = getFixIds(robotCommentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ Map<String, DiffInfo> fixPreview = gApi.changes().id(changeId).current().getFixPreview(fixId);
+
+ assertThat(fixPreview).hasSize(1);
+ assertThat(fixPreview).containsKey(FILE_NAME3);
+
+ DiffInfo diff = fixPreview.get(FILE_NAME3);
+ assertThat(diff).metaA().totalLineCount().isEqualTo(2);
+ // Original file doesn't have EOL marker at the end of file.
+ // Due to the additional EOL mark diff has one additional line
+ // This behavior is in line with ordinary get diff API.
+ assertThat(diff).metaB().totalLineCount().isEqualTo(3);
+
+ assertThat(diff).content().hasSize(2);
+ assertThat(diff).content().element(0).commonLines().containsExactly("1st line");
+ assertThat(diff).content().element(1).linesOfA().containsExactly("2nd line");
+ assertThat(diff).content().element(1).linesOfB().containsExactly("2nd line", "");
+ }
+
private static FixSuggestionInfo createFixSuggestionInfo(
FixReplacementInfo... fixReplacementInfos) {
FixSuggestionInfo newFixSuggestionInfo = new FixSuggestionInfo();
