From 106961964e431a693b1460292e5247f3032c0c4a Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 13 Aug 2020 19:00:38 +0200 Subject: [PATCH] Add draft comment creation to test API of changes The API for draft comments mimics the one for published comments. Which type of comment should be created is decided at the start of the fluent API by either using newDraftComment() or newComment(). The tests for draft comments are mostly copied from the ones for published comments albeit with some additions. Even though a lot of the code is shared between draft and published comments, having so many tests proved to be necessary as otherwise we wouldn't have noticed the different code required to correctly set a tag for draft comments. Change-Id: I413143a2fc5502efbf37406342ad42b7f6d95490 --- .../change/PerPatchsetOperations.java | 26 +- .../change/PerPatchsetOperationsImpl.java | 19 +- .../testsuite/change/TestCommentCreation.java | 19 +- .../change/PatchsetOperationsImplTest.java | 349 ++++++++++++++++++ 4 files changed, 403 insertions(+), 10 deletions(-) diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperations.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperations.java index 755e0c0914..33d2d43461 100644 --- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperations.java +++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperations.java @@ -27,8 +27,8 @@ public interface PerPatchsetOperations { TestPatchset get(); /** - * Starts the fluent chain to create a new comment. The returned builder can be used to specify - * the attributes of the new comment. To create the comment for real, {@link + * Starts the fluent chain to create a new, published comment. The returned builder can be used to + * specify the attributes of the comment. To create the comment for real, {@link * TestCommentCreation.Builder#create()} must be called. * *

Example: @@ -37,6 +37,7 @@ public interface PerPatchsetOperations { * String createdCommentUuid = changeOperations * .change(changeId) * .currentPatchset() + * .newComment() * .onLine(2) * .ofFile("file1") * .create(); @@ -45,4 +46,25 @@ public interface PerPatchsetOperations { * @return builder to create a new comment */ TestCommentCreation.Builder newComment(); + + /** + * Starts the fluent chain to create a new draft comment. The returned builder can be used to + * specify the attributes of the draft comment. To create the draft comment for real, {@link + * TestCommentCreation.Builder#create()} must be called. + * + *

Example: + * + * 

+   * String createdDraftCommentUuid = changeOperations
+   *     .change(changeId)
+   *     .currentPatchset()
+   *     .newDraftComment()
+   *     .onLine(2)
+   *     .ofFile("file1")
+   *     .create();
+   *
+ * + * @return builder to create a new comment + */ + TestCommentCreation.Builder newDraftComment(); } diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java index 2abe3c185f..d39f1e1824 100644 --- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java @@ -18,6 +18,7 @@ import static com.google.gerrit.server.CommentsUtil.setCommentCommitId; import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation.CommentSide; import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.Comment.Status; import com.google.gerrit.entities.HumanComment; import com.google.gerrit.entities.Patch; import com.google.gerrit.entities.PatchSet; @@ -25,7 +26,6 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.client.Comment; import com.google.gerrit.extensions.client.Comment.Range; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser.GenericFactory; @@ -93,7 +93,12 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations { @Override public TestCommentCreation.Builder newComment() { - return TestCommentCreation.builder(this::createComment); + return TestCommentCreation.builder(this::createComment, Status.PUBLISHED); + } + + @Override + public TestCommentCreation.Builder newDraftComment() { + return TestCommentCreation.builder(this::createComment, Status.DRAFT); } private String createComment(TestCommentCreation commentCreation) @@ -133,15 +138,16 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations { public boolean updateChange(ChangeContext context) throws Exception { HumanComment comment = toNewComment(context, commentCreation); ChangeUpdate changeUpdate = context.getUpdate(patchsetId); - changeUpdate.putComment(HumanComment.Status.PUBLISHED, comment); - // Only the tag set on the ChangeUpdate matters. The tag field of HumanComment is ignored. + changeUpdate.putComment(commentCreation.status(), comment); + // For published comments, only the tag set on the ChangeUpdate (and not on the HumanComment) + // matters. commentCreation.tag().ifPresent(changeUpdate::setTag); createdCommentUuid = comment.key.uuid; return true; } private HumanComment toNewComment(ChangeContext context, TestCommentCreation commentCreation) - throws UnprocessableEntityException, PatchListNotAvailableException { + throws PatchListNotAvailableException { String message = commentCreation.message().orElse("The text of a test comment."); String filePath = commentCreation.file().orElse(Patch.PATCHSET_LEVEL); @@ -159,6 +165,9 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations { message, unresolved, parentUuid); + // For draft comments, only the tag set on the HumanComment (and not on the ChangeUpdate) + // matters. + commentCreation.tag().ifPresent(tag -> newComment.tag = tag); commentCreation.line().ifPresent(line -> newComment.setLineNbrAndRange(line, null)); // Specification of range trumps explicit line specification. diff --git a/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java b/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java index 76f2f8d6cc..d1d15675e3 100644 --- a/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java +++ b/java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java @@ -19,6 +19,7 @@ import com.google.gerrit.acceptance.testsuite.ThrowingFunction; import com.google.gerrit.acceptance.testsuite.change.TestRange.Position; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.Patch; import java.util.Optional; import java.util.function.Function; @@ -46,11 +47,15 @@ public abstract class TestCommentCreation { public abstract Optional author(); + abstract Comment.Status status(); + abstract ThrowingFunction commentCreator(); public static TestCommentCreation.Builder builder( - ThrowingFunction commentCreator) { - return new AutoValue_TestCommentCreation.Builder().commentCreator(commentCreator); + ThrowingFunction commentCreator, Comment.Status commentStatus) { + return new AutoValue_TestCommentCreation.Builder() + .commentCreator(commentCreator) + .status(commentStatus); } @AutoValue.Builder @@ -156,7 +161,8 @@ public abstract class TestCommentCreation { /** * UUID of another comment to which this comment is a reply. This comment must have similar - * attributes (e.g. file, line, side) as the parent comment. + * attributes (e.g. file, line, side) as the parent comment. The parent comment must be a + * published comment. */ public abstract Builder parentUuid(String parentUuid); @@ -166,6 +172,13 @@ public abstract class TestCommentCreation { /** Author of the comment. Must be an existing user account. */ public abstract Builder author(Account.Id accountId); + /** + * Status of the comment. Hidden in the API surface. Use {@link + * PerPatchsetOperations#newComment()} or {@link PerPatchsetOperations#newDraftComment()} + * depending on which type of comment you want to create. + */ + abstract Builder status(Comment.Status value); + abstract TestCommentCreation.Builder commentCreator( ThrowingFunction commentCreator); diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java index 206db6e771..cf687aa2da 100644 --- a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java +++ b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java @@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.ass import com.google.common.truth.Correspondence; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Patch; @@ -36,6 +37,7 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest { @Inject private ChangeOperations changeOperations; @Inject private AccountOperations accountOperations; + @Inject private RequestScopeOperations requestScopeOperations; @Test public void commentCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception { @@ -310,10 +312,345 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest { assertThat(comment).author().id().isEqualTo(accountId.get()); } + @Test + public void draftCommentCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations.change(changeId).currentPatchset().newDraftComment().create(); + + List comments = getDraftCommentsFromServer(changeId); + assertThatList(comments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid); + } + + @Test + public void draftCommentCanBeCreatedOnOlderPatchset() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + PatchSet.Id previousPatchsetId = + changeOperations.change(changeId).currentPatchset().get().patchsetId(); + changeOperations.change(changeId).newPatchset().create(); + + String commentUuid = + changeOperations.change(changeId).patchset(previousPatchsetId).newDraftComment().create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).patchSet().isEqualTo(previousPatchsetId.get()); + } + + @Test + public void draftCommentIsCreatedWithSpecifiedMessage() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .message("Test comment message") + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).message().isEqualTo("Test comment message"); + } + + @Test + public void draftCommentCanBeCreatedWithEmptyMessage() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations.change(changeId).currentPatchset().newDraftComment().noMessage().create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).message().isNull(); + } + + @Test + public void draftPatchsetLevelCommentCanBeCreated() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onPatchsetLevel() + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).path().isEqualTo(Patch.PATCHSET_LEVEL); + } + + @Test + public void draftFileCommentCanBeCreated() throws Exception { + Change.Id changeId = changeOperations.newChange().file("file1").content("Line 1").create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onFileLevelOf("file1") + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).path().isEqualTo("file1"); + assertThat(comment).line().isNull(); + assertThat(comment).range().isNull(); + } + + @Test + public void draftLineCommentCanBeCreated() throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .file("file1") + .content("Line 1\nLine 2\nLine 3\nLine 4\n") + .create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onLine(3) + .ofFile("file1") + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).line().isEqualTo(3); + assertThat(comment).range().isNull(); + } + + @Test + public void draftRangeCommentCanBeCreated() throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .file("file1") + .content("Line 1\nLine 2\nLine 3\nLine 4\n") + .create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .fromLine(2) + .charOffset(4) + .toLine(3) + .charOffset(5) + .ofFile("file1") + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).range().startLine().isEqualTo(2); + assertThat(comment).range().startCharacter().isEqualTo(4); + assertThat(comment).range().endLine().isEqualTo(3); + assertThat(comment).range().endCharacter().isEqualTo(5); + // Line is automatically filled from specified range. It's the end line. + assertThat(comment).line().isEqualTo(3); + } + + @Test + public void draftCommentCanBeCreatedOnPatchsetCommit() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onPatchsetCommit() + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + // Null is often used instead of Side.REVISION as Side.REVISION is the default. + assertThat(comment).side().isAnyOf(Side.REVISION, null); + assertThat(comment).parent().isNull(); + } + + @Test + public void draftCommentCanBeCreatedOnParentCommit() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onParentCommit() + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).side().isEqualTo(Side.PARENT); + assertThat(comment).parent().isEqualTo(1); + } + + @Test + public void draftCommentCanBeCreatedOnSecondParentCommit() throws Exception { + // Second parents only exist for merge commits. The test API currently doesn't support the + // creation of changes with merge commits yet, though. As there's no explicit validation keeping + // us from adding comments on the non-existing second parent of a regular commit, just use the + // latter. That's still better than not having this test at all. + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onSecondParentCommit() + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).side().isEqualTo(Side.PARENT); + assertThat(comment).parent().isEqualTo(2); + } + + @Test + public void draftCommentCanBeCreatedOnAutoMergeCommit() throws Exception { + // Second parents only exist for merge commits. The test API currently doesn't support the + // creation of changes with merge commits yet, though. As there's no explicit validation keeping + // us from adding comments on the non-existing second parent of a regular commit, just use the + // latter. That's still better than not having this test at all. + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .onAutoMergeCommit() + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).side().isEqualTo(Side.PARENT); + assertThat(comment).parent().isNull(); + } + + @Test + public void draftCommentCanBeCreatedAsResolved() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations.change(changeId).currentPatchset().newDraftComment().resolved().create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).unresolved().isFalse(); + } + + @Test + public void draftCommentCanBeCreatedAsUnresolved() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations.change(changeId).currentPatchset().newDraftComment().unresolved().create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).unresolved().isTrue(); + } + + @Test + public void draftReplyToDraftCommentCanBeCreated() { + Change.Id changeId = changeOperations.newChange().create(); + String parentCommentUuid = + changeOperations.change(changeId).currentPatchset().newDraftComment().create(); + + // Gerrit's other APIs shouldn't support the creation of a draft reply to a draft comment but + // there's currently no reason to not support such a comment via the test API if a test really + // wants to create such a comment. + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .parentUuid(parentCommentUuid) + .create(); + } + + @Test + public void draftReplyToPublishedCommentCanBeCreated() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + String parentCommentUuid = + changeOperations.change(changeId).currentPatchset().newComment().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .parentUuid(parentCommentUuid) + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).inReplyTo().isEqualTo(parentCommentUuid); + } + + @Test + public void tagCanBeAttachedToADraftComment() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .tag("my special tag") + .create(); + + CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid); + assertThat(comment).tag().isEqualTo("my special tag"); + } + + @Test + public void draftCommentIsCreatedWithSpecifiedAuthor() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + Account.Id accountId = accountOperations.newAccount().create(); + + String commentUuid = + changeOperations + .change(changeId) + .currentPatchset() + .newDraftComment() + .author(accountId) + .create(); + + // A user can only retrieve their own draft comments. + requestScopeOperations.setApiUser(accountId); + List comments = getDraftCommentsFromServer(changeId); + // Draft comments never have the author field set. As a user can only retrieve their own draft + // comments, we implicitly know that the author was correctly set when we find the created + // comment in the draft comments of that user. + assertThatList(comments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid); + } + + @Test + public void noDraftCommentsAreCreatedOnCreationOfPublishedComment() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + changeOperations.change(changeId).currentPatchset().newComment().create(); + + List comments = getDraftCommentsFromServer(changeId); + assertThatList(comments).isEmpty(); + } + + @Test + public void noPublishedCommentsAreCreatedOnCreationOfDraftComment() throws Exception { + Change.Id changeId = changeOperations.newChange().create(); + + changeOperations.change(changeId).currentPatchset().newDraftComment().create(); + + List comments = getCommentsFromServer(changeId); + assertThatList(comments).isEmpty(); + } + private List getCommentsFromServer(Change.Id changeId) throws RestApiException { return gApi.changes().id(changeId.get()).commentsAsList(); } + private List getDraftCommentsFromServer(Change.Id changeId) throws RestApiException { + return gApi.changes().id(changeId.get()).draftsAsList(); + } + private CommentInfo getCommentFromServer(Change.Id changeId, String uuid) throws RestApiException { return gApi.changes().id(changeId.get()).commentsAsList().stream() @@ -325,6 +662,18 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest { String.format("Comment %s not found on change %d", uuid, changeId.get()))); } + private CommentInfo getDraftCommentFromServer(Change.Id changeId, String uuid) + throws RestApiException { + return gApi.changes().id(changeId.get()).draftsAsList().stream() + .filter(comment -> comment.id.equals(uuid)) + .findAny() + .orElseThrow( + () -> + new IllegalStateException( + String.format( + "Draft comment %s not found on change %d", uuid, changeId.get()))); + } + private Correspondence hasUuid() { return NullAwareCorrespondence.transforming(comment -> comment.id, "hasUuid"); }