commit 5e544811c954d0d8c267fca4325402871964b21e Author: Alice Kober-Sotzek Date: Fri Mar 13 21:54:16 2020 +0100 Clean up some tests around fix replacement interpretation FixCalculatorVariousTest contains the low level tests for how replacements are translated into edits and the new file content. Some of the tests in FixReplacementInterpreterTest also covered this. Remove test cases from FixReplacementInterpreterTest which were already covered by FixCalculatorVariousTest and move missing test cases over. In addition, add a new test to FixReplacementInterpreterTest which covers the high level behavior of creating tree modifications. Change-Id: I5ff83c2726d77bd19190bc45da8c87c053754b43 diff --git a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java index 56d3b6c..e5ef6af 100644 --- a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java +++ b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.fixes; import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThatList; -import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.util.Comparator.comparing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -24,7 +23,6 @@ import com.google.common.collect.ImmutableList; import com.google.gerrit.entities.Comment.Range; import com.google.gerrit.entities.FixReplacement; import com.google.gerrit.extensions.restapi.BinaryResult; -import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.server.change.FileContentUtil; import com.google.gerrit.server.edit.tree.TreeModification; import com.google.gerrit.server.project.ProjectState; @@ -57,6 +55,25 @@ public class FixReplacementInterpreterTest { } @Test + public void replacementIsTranslatedToTreeModification() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath1, new Range(1, 1, 3, 2), "Modified content"); + mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); + + List treeModifications = toTreeModifications(fixReplacement); + assertThatList(treeModifications) + .onlyElement() + .asChangeFileContentModification() + .filePaths() + .containsExactly(filePath1); + assertThatList(treeModifications) + .onlyElement() + .asChangeFileContentModification() + .newContent() + .isEqualTo("FModified contentird line\n"); + } + + @Test public void treeModificationsTargetCorrectFiles() throws Exception { FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(1, 6, 3, 2), "Modified content"); @@ -93,137 +110,6 @@ public class FixReplacementInterpreterTest { } @Test - public void replacementsCanDeleteALine() throws Exception { - FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(2, 0, 3, 0), ""); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nThird line\n"); - } - - @Test - public void replacementsCanAddALine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(2, 0, 2, 0), "A new line\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nA new line\nSecond line\nThird line\n"); - } - - @Test - public void replacementsMaySpanMultipleLines() throws Exception { - FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(1, 6, 3, 1), "and t"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First and third line\n"); - } - - @Test - public void replacementsMayOccurOnSameLine() throws Exception { - FixReplacement fixReplacement1 = new FixReplacement(filePath1, new Range(2, 0, 2, 6), "A"); - FixReplacement fixReplacement2 = - new FixReplacement(filePath1, new Range(2, 7, 2, 11), "modification"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = - toTreeModifications(fixReplacement1, fixReplacement2); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nA modification\nThird line\n"); - } - - @Test() - public void startAfterEndOfLineMarkThrowsAnException() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(1, 11, 2, 6), "A modification"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test() - public void endAfterEndOfLineMarkThrowsAnException() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(2, 0, 2, 12), "A modification"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test - public void replacementsMayTouch() throws Exception { - FixReplacement fixReplacement1 = - new FixReplacement(filePath1, new Range(1, 6, 2, 7), "modified "); - FixReplacement fixReplacement2 = - new FixReplacement(filePath1, new Range(2, 7, 3, 5), "content"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = - toTreeModifications(fixReplacement1, fixReplacement2); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First modified content line\n"); - } - - @Test - public void replacementsCanAddContentAtEndOfFile() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(4, 0, 4, 0), "New content"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nSecond line\nThird line\nNew content"); - } - - @Test - public void replacementsCanChangeLastLine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(3, 0, 4, 0), "New content\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nSecond line\nNew content\n"); - } - - @Test - public void replacementsCanChangeLastLineWithoutEOLMark() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(3, 0, 3, 10), "New content\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nSecond line\nNew content\n"); - } - - @Test public void replacementsCanModifySeveralFilesInAnyOrder() throws Exception { FixReplacement fixReplacement1 = new FixReplacement(filePath1, new Range(1, 1, 3, 2), "Modified content"); @@ -249,84 +135,6 @@ public class FixReplacementInterpreterTest { .isEqualTo("1st line\nFirst modification\nSecond modification\n"); } - @Test - public void lineSeparatorCanBeChanged() throws Exception { - FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(2, 11, 3, 0), "\r"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - List treeModifications = toTreeModifications(fixReplacement); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo("First line\nSecond line\rThird line\n"); - } - - @Test - public void replacementsDoNotNeedToBeOrderedAccordingToRange() throws Exception { - FixReplacement fixReplacement1 = - new FixReplacement(filePath1, new Range(1, 0, 2, 0), "1st modification\n"); - FixReplacement fixReplacement2 = - new FixReplacement(filePath1, new Range(3, 0, 4, 0), "2nd modification\n"); - FixReplacement fixReplacement3 = - new FixReplacement(filePath1, new Range(4, 0, 5, 0), "3rd modification\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\nFourth line\nFifth line\n"); - - List treeModifications = - toTreeModifications(fixReplacement2, fixReplacement1, fixReplacement3); - assertThatList(treeModifications) - .onlyElement() - .asChangeFileContentModification() - .newContent() - .isEqualTo( - "1st modification\nSecond line\n2nd modification\n3rd modification\nFifth line\n"); - } - - @Test - public void replacementsMustNotReferToNotExistingLine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(5, 0, 5, 0), "A new line\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test - public void replacementsMustNotReferToZeroLine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(0, 0, 0, 0), "A new line\n"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test - public void replacementsMustNotReferToNotExistingOffsetOfIntermediateLine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(1, 0, 1, 11), "modified"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test - public void replacementsMustNotReferToNotExistingOffsetOfLastLine() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(3, 0, 3, 11), "modified"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - - @Test - public void replacementsMustNotReferToNegativeOffset() throws Exception { - FixReplacement fixReplacement = - new FixReplacement(filePath1, new Range(1, -1, 1, 5), "modified"); - mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - - assertThrows(ResourceConflictException.class, () -> toTreeModifications(fixReplacement)); - } - private void mockFileContent(String filePath, String fileContent) throws Exception { when(fileContentUtil.getContent(repository, projectState, patchSetCommitId, filePath)) .thenReturn(BinaryResult.create(fileContent)); diff --git a/javatests/com/google/gerrit/server/fixes/fixCalculator/FixCalculatorVariousTest.java b/javatests/com/google/gerrit/server/fixes/fixCalculator/FixCalculatorVariousTest.java index 861af3e..fa5c47f 100644 --- a/javatests/com/google/gerrit/server/fixes/fixCalculator/FixCalculatorVariousTest.java +++ b/javatests/com/google/gerrit/server/fixes/fixCalculator/FixCalculatorVariousTest.java @@ -34,7 +34,7 @@ public class FixCalculatorVariousTest { "First line\nSecond line\nThird line\nFourth line\nFifth line\n"; private static final Text multilineContent = new Text(multilineContentString.getBytes(UTF_8)); - public static FixResult calculateFixSingleReplacement( + static FixResult calculateFixSingleReplacement( String content, int startLine, int startChar, int endLine, int endChar, String replacement) throws ResourceConflictException { FixReplacement fixReplacement = @@ -52,13 +52,66 @@ public class FixCalculatorVariousTest { } @Test - public void insertAtTheEndOfSingleLineContentHasEOLMarkInvalidPosition() throws Exception { + public void lineNumberMustExist() { + assertThrows( + ResourceConflictException.class, + () -> calculateFixSingleReplacement("First line\nSecond line", 4, 0, 4, 0, "Abc")); + } + + @Test + public void startOffsetMustNotBeNegative() { + assertThrows( + ResourceConflictException.class, + () -> calculateFixSingleReplacement("First line\nSecond line", 0, -1, 0, 0, "Abc")); + } + + @Test + public void endOffsetMustNotBeNegative() { + assertThrows( + ResourceConflictException.class, + () -> calculateFixSingleReplacement("First line\nSecond line", 0, 0, 0, -1, "Abc")); + } + + @Test + public void insertAtTheEndOfSingleLineContentHasEOLMarkInvalidPosition() { assertThrows( ResourceConflictException.class, () -> calculateFixSingleReplacement("First line\n", 1, 11, 1, 11, "Abc")); } @Test + public void startAfterEndOfLineMarkOfIntermediateLineThrowsAnException() { + assertThrows( + ResourceConflictException.class, + () -> + calculateFixSingleReplacement( + "First line\nSecond line\nThird line\n", 1, 11, 2, 6, "Abc")); + } + + @Test + public void startAfterEndOfLineMarkOfLastLineThrowsAnException() { + assertThrows( + ResourceConflictException.class, + () -> calculateFixSingleReplacement("First line\n", 1, 11, 2, 0, "Abc")); + } + + @Test + public void endAfterEndOfLineMarkOfIntermediateLineThrowsAnException() { + assertThrows( + ResourceConflictException.class, + () -> + calculateFixSingleReplacement( + "First line\nSecond line\nThird line\n", 2, 0, 2, 12, "Abc")); + } + + @Test + public void endAfterEndOfLineMarkOfLastLineThrowsAnException() { + assertThrows( + ResourceConflictException.class, + () -> calculateFixSingleReplacement("First line\nSecond line\n", 2, 0, 2, 12, "Abc")); + } + + @Test public void severalChangesInTheSameLineNonSorted() throws Exception { FixReplacement replace = new FixReplacement("path", new Range(2, 1, 2, 3), "ABC"); FixReplacement insert = new FixReplacement("path", new Range(2, 5, 2, 5), "DEFG"); @@ -146,7 +199,18 @@ public class FixCalculatorVariousTest { assertThat(result) .text() .isEqualTo( - "FiAB\nC\nDEFG\nQ\nrd line\nFourth lneQWERTY\nSixth line\nSevXY line\nEighth KLMNO\nASDFline\nNinth line\nTenine\n"); + "FiAB\n" + + "C\n" + + "DEFG\n" + + "Q\n" + + "rd line\n" + + "Fourth lneQWERTY\n" + + "Sixth line\n" + + "SevXY line\n" + + "Eighth KLMNO\n" + + "ASDFline\n" + + "Ninth line\n" + + "Tenine\n"); assertThat(result).edits().hasSize(3); assertThat(result).edits().element(0).isReplace(0, 5, 0, 6); assertThat(result) @@ -165,4 +229,23 @@ public class FixCalculatorVariousTest { assertThat(result).edits().element(2).isReplace(9, 1, 11, 1); assertThat(result).edits().element(2).internalEdits().onlyElement().isDelete(3, 4, 3); } + + @Test + public void changesMayTouch() throws Exception { + FixReplacement firstReplace = new FixReplacement("path", new Range(1, 6, 2, 7), "modified "); + FixReplacement consecutiveReplace = + new FixReplacement("path", new Range(2, 7, 3, 5), "content"); + FixResult result = + FixCalculator.calculateFix( + multilineContent, ImmutableList.of(firstReplace, consecutiveReplace)); + assertThat(result).text().isEqualTo("First modified content line\nFourth line\nFifth line\n"); + assertThat(result).edits().hasSize(1); + Edit edit = result.edits.get(0); + assertThat(edit).isReplace(0, 3, 0, 1); + // The current code creates two inline edits even though only one would be necessary. It + // shouldn't make a visual difference to the user and hence we can ignore this. + assertThat(edit).internalEdits().hasSize(2); + assertThat(edit).internalEdits().element(0).isReplace(6, 12, 6, 9); + assertThat(edit).internalEdits().element(1).isReplace(18, 10, 15, 7); + } }