opendev/gerrit
Code Issues Proposed changes

Only port unresolved comment threads

Browse Source 
Porting all comments from previous patchsets to the requested patchset
could be a large amount of comments especially if there was a lot of
activity (conversations + code adjustments) on the change in the past.
In addition, some of the comment threads might not even apply to the
later patchsets anymore as they were already addressed. To increase
the overview for users, we decide to port only unresolved comment
threads. We filter the comments already in the backend as we can thus
achieve the best performance (less comments to port -> less diffs to
compute, which can be a big amount otherwise for higher patchset
numbers).

We apply that filtering approach both to published and draft comments.
However, we don't factor in the unresolved state of draft comments which
are a reply to published comments when deciding whether to port the
published comment thread. We could have done this but it would have
added additional complexity. Furthermore, this would have introduced
a dependency of published comments on draft comments which we typically
don't have for endpoints of published comments. There's also always
the risk that Gerrit's state changes between the calls to the two
different endpoints for published and draft comments (e.g. draft
comment which changed the unresolved state is discarded), in which case
the published comment endpoint would return the wrong result.

For the frontend, this behavior means that it might get a ported draft
comment for which the published comment thread was not ported (or the
other way around). In such a case, the frontend should simply re-use the
original comment thread (which it loaded anyway as the frontend loads
all comments on a change) and put it to the file/range indicated by the
ported draft comment. The frontend needs such an approach anyway as we
don't port robot comments to which a published/draft comment can also
be a reply.

There is one corner case not implemented right now: If a robot comment
is in the middle of a comment thread, we might derive the wrong
unresolved state for the thread as we internally consider the thread
as two threads (-> one before the robot comment, one afterwards).
In theory, comment threads involving robot comments in the middle
are possible with the current APIs. However, we don't know of anybody
using such a bot at the moment. If somebody does, they might see
other issues as well as this corner case is not explicitly supported in
other places of Gerrit either. If we wanted to fix this corner case,
we'd need to load robot comments too and then filter them out before
porting the comments (or returning them). We didn't want to add this
additional complexity at the moment but can do so whenever we need it.

The same situation shouldn't happen for draft comments as drafts will
be considered to be at the end of a comment thread. Even if a draft
comment was added prior to another reply on the thread, its date will
be adjusted when published and hence it will move to the end of the
thread. In addition, published comments can't be a reply to a draft
comment.

Filtering out all resolved comment threads might be too aggressive in
some situations. Reviewers might especially want to see comment threads
which were resolved for the target patchset. The current fields on
comments don't allow us to identify this situation, though, as the
patchset field of a comment always points to the patchset on which the
root of the comment thread was left. We also shouldn't change this,
especially as it might break other tools. We'd need another solution
which we can add when we see that the current, simpler approach isn't
sufficient enough.

Change-Id: I4af5946ce34b9f4998d4922c6b51c585bc687c66
This commit is contained in:
Alice Kober-Sotzek
2020-09-11 10:12:10 +02:00
parent 159e70fd80
commit 6b048edeff
3 changed files with 159 additions and 359 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
java/com/google/gerrit/server/restapi/change/CommentPorter.java
View File

@@ -29,6 +29,8 @@ import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.change.CommentThread;
import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.DiffMappings;
import com.google.gerrit.server.patch.GitPositionTransformer;
@@ -43,6 +45,7 @@ import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -102,9 +105,19 @@ public class CommentPorter {
private ImmutableList<HumanComment> filterToRelevant(
List<HumanComment> allComments, PatchSet targetPatchset) {
return allComments.stream()
ImmutableList<HumanComment> previousPatchsetsComments =
allComments.stream()
.filter(comment -> comment.key.patchSetId < targetPatchset.number())
.collect(toImmutableList());
ImmutableSet<CommentThread<HumanComment>> commentThreads =
CommentThreads.forComments(previousPatchsetsComments).getThreads();
return commentThreads.stream()
.filter(CommentThread::unresolved)
.map(CommentThread::comments)
.flatMap(Collection::stream)
.collect(toImmutableList());
}
private ImmutableList<HumanComment> port(

493
javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
View File

@@ -27,6 +27,7 @@ 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.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation;
import com.google.gerrit.acceptance.testsuite.change.TestPatchset;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
@@ -41,7 +42,6 @@ import java.time.LocalDateTime;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import org.junit.Ignore;
import org.junit.Test;
public class PortedCommentsIT extends AbstractDaemonTest {
@@ -58,9 +58,9 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
changeOps.change(changeId).patchset(patchset2Id).newComment().create();
changeOps.change(changeId).patchset(patchset3Id).newComment().create();
String comment1Uuid = newComment(patchset1Id).create();
newComment(patchset2Id).create();
newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -76,8 +76,8 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset4Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String comment3Uuid = changeOps.change(changeId).patchset(patchset3Id).newComment().create();
String comment1Uuid = newComment(patchset1Id).create();
String comment3Uuid = newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset4Id));
@@ -93,8 +93,8 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String comment2Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String comment1Uuid = newComment(patchset1Id).create();
String comment2Uuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -110,21 +110,9 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rootCommentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String child1CommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.parentUuid(rootCommentUuid)
.create();
String child2CommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.parentUuid(child1CommentUuid)
.create();
String rootCommentUuid = newComment(patchset1Id).create();
String child1CommentUuid = newComment(patchset1Id).parentUuid(rootCommentUuid).create();
String child2CommentUuid = newComment(patchset1Id).parentUuid(child1CommentUuid).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -134,17 +122,14 @@ public class PortedCommentsIT extends AbstractDaemonTest {
}
@Test
// TODO(aliceks): Filter out unresolved comment threads.
@Ignore
public void onlyUnresolvedCommentsArePorted() throws Exception {
public void onlyUnresolvedPublishedCommentsArePorted() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
String comment2Uuid =
changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
newComment(patchset1Id).resolved().create();
String comment2Uuid = newComment(patchset1Id).unresolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -152,33 +137,34 @@ public class PortedCommentsIT extends AbstractDaemonTest {
}
@Test
// TODO(aliceks): Filter out unresolved comment threads.
@Ignore
public void onlyUnresolvedDraftCommentsArePorted() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
newDraftComment(patchset1Id).author(accountId).resolved().create();
String comment2Uuid = newDraftComment(patchset1Id).author(accountId).unresolved().create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment2Uuid);
}
@Test
public void unresolvedStateOfLastCommentInThreadMatters() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rootComment1Uuid =
changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
String rootComment1Uuid = newComment(patchset1Id).resolved().create();
String childComment1Uuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.parentUuid(rootComment1Uuid)
.unresolved()
.create();
String rootComment2Uuid =
changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.parentUuid(rootComment2Uuid)
.resolved()
.create();
newComment(patchset1Id).parentUuid(rootComment1Uuid).unresolved().create();
String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
newComment(patchset1Id).parentUuid(rootComment2Uuid).resolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -196,28 +182,15 @@ public class PortedCommentsIT extends AbstractDaemonTest {
// Add comments. Comments should be more than 1 second apart as NoteDb only supports second
// precision.
LocalDateTime now = LocalDateTime.now();
String rootCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.resolved()
.createdOn(now)
.create();
String rootCommentUuid = newComment(patchset1Id).resolved().createdOn(now).create();
String childComment1Uuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.resolved()
.createdOn(now.plusSeconds(5))
.create();
String childComment2Uuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.unresolved()
.createdOn(now.plusSeconds(10))
@@ -230,6 +203,30 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.containsExactly(rootCommentUuid, childComment1Uuid, childComment2Uuid);
}
@Test
public void unresolvedStateOfDraftCommentsIsIgnoredForPublishedComments() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rootComment1Uuid = newComment(patchset1Id).resolved().create();
newDraftComment(patchset1Id)
.author(accountId)
.parentUuid(rootComment1Uuid)
.unresolved()
.create();
String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
newDraftComment(patchset1Id).author(accountId).parentUuid(rootComment2Uuid).resolved().create();
// Draft comments are only visible to their author.
requestScopeOps.setApiUser(accountId);
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(rootComment2Uuid);
}
@Test
public void draftCommentsAreNotPortedViaApiForPublishedComments() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
@@ -238,7 +235,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
changeOps.change(changeId).patchset(patchset1Id).newDraftComment().author(accountId).create();
newDraftComment(patchset1Id).author(accountId).create();
// Draft comments are only visible to their author.
requestScopeOps.setApiUser(accountId);
@@ -255,7 +252,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -271,7 +268,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -288,7 +285,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().author(otherUserId).create();
newComment(patchset1Id).author(otherUserId).create();
List<CommentInfo> portedComments = flatten(getPortedDraftCommentsOfUser(patchset2Id, userId));
@@ -303,10 +300,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rangeCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.message("Range comment")
.fromLine(1)
.charOffset(2)
@@ -315,30 +309,11 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.ofFile("myFile")
.create();
String lineCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.message("Line comment")
.onLine(1)
.ofFile("myFile")
.create();
newComment(patchset1Id).message("Line comment").onLine(1).ofFile("myFile").create();
String fileCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.message("File comment")
.onFileLevelOf("myFile")
.create();
newComment(patchset1Id).message("File comment").onFileLevelOf("myFile").create();
String patchsetLevelCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.message("Patchset-level comment")
.onPatchsetLevel()
.create();
newComment(patchset1Id).message("Patchset-level comment").onPatchsetLevel().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -355,8 +330,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String commentUuid =
changeOps.change(changeId).patchset(patchset1Id).newComment().onParentCommit().create();
String commentUuid = newComment(patchset1Id).onParentCommit().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -370,7 +344,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String commentUuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -384,7 +358,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -399,13 +373,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newDraftComment()
.author(authorId)
.create();
String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -424,8 +392,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
String commentUuid = newComment(patchset1.patchsetId()).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -439,13 +406,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1.patchsetId())
.newComment()
.message("My comment text")
.create();
String commentUuid = newComment(patchset1.patchsetId()).message("My comment text").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -459,15 +420,9 @@ public class PortedCommentsIT extends AbstractDaemonTest {
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rootCommentUuid =
changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
String rootCommentUuid = newComment(patchset1.patchsetId()).create();
String childCommentUuid =
changeOps
.change(changeId)
.patchset(patchset1.patchsetId())
.newComment()
.parentUuid(rootCommentUuid)
.create();
newComment(patchset1.patchsetId()).parentUuid(rootCommentUuid).create();
CommentInfo portedComment = getPortedComment(patchset2Id, childCommentUuid);
@@ -482,8 +437,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps.change(changeId).patchset(patchset1Id).newComment().author(authorId).create();
String commentUuid = newComment(patchset1Id).author(authorId).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -498,13 +452,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newDraftComment()
.author(authorId)
.create();
String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -521,13 +469,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1.patchsetId())
.newComment()
.tag("My comment tag")
.create();
String commentUuid = newComment(patchset1.patchsetId()).tag("My comment tag").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -541,7 +483,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -555,7 +497,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -572,13 +514,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onFileLevelOf("myFile")
.create();
String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -602,10 +538,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -636,10 +569,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -665,10 +595,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -703,10 +630,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -746,10 +670,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -789,10 +710,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -821,10 +739,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(3)
@@ -855,10 +770,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(2)
@@ -889,10 +801,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -923,10 +832,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -955,10 +861,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(3)
@@ -988,10 +891,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1020,10 +920,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1060,10 +957,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1087,10 +981,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1119,14 +1010,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1147,14 +1031,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 2\nLine 3\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1170,14 +1047,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1201,14 +1071,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset3Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset3Id);
@@ -1238,14 +1101,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1272,14 +1128,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine two\nLine three\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(2)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1302,14 +1151,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(2)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1330,14 +1172,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1358,14 +1193,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(4)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1386,14 +1214,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(4)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1409,14 +1230,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onLine(3)
.ofFile("myFile")
.create();
String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1439,13 +1253,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onFileLevelOf("myFile")
.create();
String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1461,13 +1269,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onFileLevelOf("myFile")
.create();
String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1492,7 +1294,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().onFileLevelOf("myFile").create();
newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1510,13 +1312,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
.onFileLevelOf("myFile")
.create();
String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1539,7 +1335,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1555,7 +1351,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1576,10 +1372,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(changeId)
.patchset(patchset1Id)
.newComment()
newComment(patchset1Id)
// The /COMMIT_MSG file has a header of 6 lines, so the summary line is in line 7.
// Place comment on 'Text 2' which is line 10.
.onLine(10)
@@ -1616,14 +1409,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
changeOps.change(childChangeId).newPatchset().parent().patchset(parentPatchset2Id).create();
// Add comment.
String commentUuid =
changeOps
.change(childChangeId)
.patchset(childPatchset1Id)
.newComment()
.onParentCommit()
.onLine(1)
.ofFile("myFile")
.create();
newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1665,14 +1451,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(childChangeId)
.patchset(childPatchset1Id)
.newComment()
.onParentCommit()
.onLine(1)
.ofFile("file1")
.create();
newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1714,14 +1493,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(childChangeId)
.patchset(childPatchset1Id)
.newComment()
.onSecondParentCommit()
.onLine(1)
.ofFile("file2")
.create();
newComment(childPatchset1Id).onSecondParentCommit().onLine(1).ofFile("file2").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1769,14 +1541,7 @@ public class PortedCommentsIT extends AbstractDaemonTest {
.create();
// Add comment.
String commentUuid =
changeOps
.change(childChangeId)
.patchset(childPatchset1Id)
.newComment()
.onAutoMergeCommit()
.onLine(1)
.ofFile("file1")
.create();
newComment(childPatchset1Id).onAutoMergeCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1787,6 +1552,22 @@ public class PortedCommentsIT extends AbstractDaemonTest {
assertThat(portedComment).line().isGreaterThan(2);
}
private TestCommentCreation.Builder newComment(PatchSet.Id patchsetId) {
// Create unresolved comments by default as only those are ported. Tests get override the
// unresolved state by explicitly setting it.
return changeOps.change(patchsetId.changeId()).patchset(patchsetId).newComment().unresolved();
}
private TestCommentCreation.Builder newDraftComment(PatchSet.Id patchsetId) {
// Create unresolved comments by default as only those are ported. Tests get override the
// unresolved state by explicitly setting it.
return changeOps
.change(patchsetId.changeId())
.patchset(patchsetId)
.newDraftComment()
.unresolved();
}
private CommentInfo getPortedComment(PatchSet.Id patchsetId, String commentUuid)
throws RestApiException {
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchsetId);

8
javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
View File

@@ -57,6 +57,8 @@ public class CommentPorterTest {
@Mock private PatchListCache patchListCache;
private int uuidCounter = 0;
@Test
public void commentsAreNotDroppedWhenDiffNotAvailable() throws Exception {
Project.NameKey project = Project.nameKey("myProject");
@@ -206,7 +208,7 @@ public class CommentPorterTest {
private HumanComment createComment(PatchSet.Id patchsetId, String filePath) {
return new HumanComment(
new Comment.Key("commentUuid", filePath, patchsetId.get()),
new Comment.Key(getUniqueUuid(), filePath, patchsetId.get()),
Account.id(100),
new Timestamp(1234),
(short) 1,
@@ -215,6 +217,10 @@ public class CommentPorterTest {
true);
}
private String getUniqueUuid() {
return "commentUuid" + uuidCounter++;
}
private Correspondence<HumanComment, String> hasFilePath() {
return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
}