From 4b43eb80257d86af171f9a0c514279ced8712a12 Mon Sep 17 00:00:00 2001 From: Dmitrii Filippov Date: Fri, 24 Jan 2020 15:52:32 +0100 Subject: [PATCH] Add edge cases tests and fixes for getFixPreview * Add tests for edge cases * Throw ResourceNotFoundException if a fix references non-existing files Change-Id: I6abf4c5a1236602e523ad0c3e703be452ce8e982 --- .../server/patch/PatchScriptBuilder.java | 6 +- .../patch/PatchScriptFactoryForAutoFix.java | 5 +- .../server/restapi/change/GetFixPreview.java | 2 +- .../api/revision/RobotCommentsIT.java | 116 ++++++++++++++++-- 4 files changed, 115 insertions(+), 14 deletions(-) diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index 92df79451f..d88b626860 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.ChangeType; 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 @@ class PatchScriptBuilder { PatchScript toPatchScript( Repository git, ObjectId baseId, String fileName, List 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 cf0719009b..572aa9bc38 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.entities.PatchSet; 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 @@ public class PatchScriptFactoryForAutoFix implements Callable { @Override public PatchScript call() throws LargeObjectException, AuthException, InvalidChangeOperationException, IOException, - PermissionBackendException { + PermissionBackendException, ResourceNotFoundException { try { permissionBackend.currentUser().change(notes).check(ChangePermission.READ); @@ -108,7 +109,7 @@ public class PatchScriptFactoryForAutoFix implements Callable { 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 06667562fd..b39424c8a6 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 @@ public class GetFixPreview implements RestReadView { String fileName, ImmutableList 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 c35ded6cca..9cabe4b22b 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.common.collect.Iterables; 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.Map; 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 @@ public class RobotCommentsIT extends AbstractDaemonTest { 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(); @@ -995,20 +1000,76 @@ public class RobotCommentsIT extends AbstractDaemonTest { } @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"; + + FixSuggestionInfo commitMsgSuggestionInfo = createFixSuggestionInfo(commitMsgReplacement); + RobotCommentInput commitMsgRobotCommentInput = + TestCommentHelper.createRobotCommentInput(Patch.COMMIT_MSG, commitMsgSuggestionInfo); + testCommentHelper.addRobotComment(changeId, commitMsgRobotCommentInput); + + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + Map 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); - testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); List robotCommentInfos = getRobotComments(); List fixIds = getFixIds(robotCommentInfos); String fixId = Iterables.getOnlyElement(fixIds); assertThrows( - BadRequestException.class, + ResourceNotFoundException.class, () -> gApi.changes().id(changeId).current().getFixPreview(fixId)); } @@ -1122,6 +1183,41 @@ public class RobotCommentsIT extends AbstractDaemonTest { 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 robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + Map 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();