Make consistent ordering for lists from PatchLineCommentsUtil

Change-Id: Ia6ae44f387444a29b259ce02166aade808bf70c0

gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java

@@ -99,22 +99,22 @@ public class PatchLineCommentsUtil {
   public List<PatchLineComment> publishedByChange(ReviewDb db,
       ChangeNotes notes) throws OrmException {
     if (!migration.readComments()) {
-      return byCommentStatus(db.patchComments().byChange(notes.getChangeId()),
+      return sort(byCommentStatus(
-          Status.PUBLISHED);
+          db.patchComments().byChange(notes.getChangeId()), Status.PUBLISHED));
     }
 
     notes.load();
     List<PatchLineComment> comments = Lists.newArrayList();
     comments.addAll(notes.getBaseComments().values());
     comments.addAll(notes.getPatchSetComments().values());
-    return comments;
+    return sort(comments);
   }
 
   public List<PatchLineComment> draftByChange(ReviewDb db,
       ChangeNotes notes) throws OrmException {
     if (!migration.readComments()) {
-      return byCommentStatus(db.patchComments().byChange(notes.getChangeId()),
+      return sort(byCommentStatus(
-          Status.DRAFT);
+          db.patchComments().byChange(notes.getChangeId()), Status.DRAFT));
     }
 
     List<PatchLineComment> comments = Lists.newArrayList();
@@ -125,7 +125,7 @@ public class PatchLineCommentsUtil {
         comments.addAll(draftByChangeAuthor(db, notes, account));
       }
     }
-    return comments;
+    return sort(comments);
   }
 
   private static List<PatchLineComment> byCommentStatus(
@@ -144,7 +144,7 @@ public class PatchLineCommentsUtil {
   public List<PatchLineComment> byPatchSet(ReviewDb db,
       ChangeNotes notes, PatchSet.Id psId) throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().byPatchSet(psId).toList();
+      return sort(db.patchComments().byPatchSet(psId).toList());
     }
     List<PatchLineComment> comments = Lists.newArrayList();
     comments.addAll(publishedByPatchSet(db, notes, psId));
@@ -156,13 +156,14 @@ public class PatchLineCommentsUtil {
         comments.addAll(draftByPatchSetAuthor(db, psId, account, notes));
       }
     }
-    return comments;
+    return sort(comments);
   }
 
   public List<PatchLineComment> publishedByChangeFile(ReviewDb db,
       ChangeNotes notes, Change.Id changeId, String file) throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().publishedByChangeFile(changeId, file).toList();
+      return sort(
+          db.patchComments().publishedByChangeFile(changeId, file).toList());
     }
     notes.load();
     List<PatchLineComment> comments = Lists.newArrayList();
@@ -170,70 +171,69 @@ public class PatchLineCommentsUtil {
     addCommentsOnFile(comments, notes.getBaseComments().values(), file);
     addCommentsOnFile(comments, notes.getPatchSetComments().values(),
         file);
-    Collections.sort(comments, ChangeNotes.PatchLineCommentComparator);
+    return sort(comments);
-    return comments;
   }
 
   public List<PatchLineComment> publishedByPatchSet(ReviewDb db,
       ChangeNotes notes, PatchSet.Id psId) throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().publishedByPatchSet(psId).toList();
+      return sort(
+          db.patchComments().publishedByPatchSet(psId).toList());
     }
     notes.load();
     List<PatchLineComment> comments = new ArrayList<PatchLineComment>();
     comments.addAll(notes.getPatchSetComments().get(psId));
     comments.addAll(notes.getBaseComments().get(psId));
-    return comments;
+    return sort(comments);
   }
 
   public List<PatchLineComment> draftByPatchSetAuthor(ReviewDb db,
       PatchSet.Id psId, Account.Id author, ChangeNotes notes)
       throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().draftByPatchSetAuthor(psId, author).toList();
+      return sort(
+          db.patchComments().draftByPatchSetAuthor(psId, author).toList());
     }
 
     List<PatchLineComment> comments = Lists.newArrayList();
-
     comments.addAll(notes.getDraftBaseComments(author).row(psId).values());
     comments.addAll(notes.getDraftPsComments(author).row(psId).values());
-    Collections.sort(comments, ChangeNotes.PatchLineCommentComparator);
+    return sort(comments);
-    return comments;
   }
 
   public List<PatchLineComment> draftByChangeFileAuthor(ReviewDb db,
       ChangeNotes notes, String file, Account.Id author)
       throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments()
+      return sort(
-          .draftByChangeFileAuthor(notes.getChangeId(), file, author)
+          db.patchComments()
-          .toList();
+            .draftByChangeFileAuthor(notes.getChangeId(), file, author)
+            .toList());
     }
     List<PatchLineComment> comments = Lists.newArrayList();
     addCommentsOnFile(comments, notes.getDraftBaseComments(author).values(),
         file);
     addCommentsOnFile(comments, notes.getDraftPsComments(author).values(),
         file);
-    Collections.sort(comments, ChangeNotes.PatchLineCommentComparator);
+    return sort(comments);
-    return comments;
   }
 
   public List<PatchLineComment> draftByChangeAuthor(ReviewDb db,
       ChangeNotes notes, Account.Id author)
       throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().byChange(notes.getChangeId()).toList();
+      return sort(db.patchComments().byChange(notes.getChangeId()).toList());
     }
     List<PatchLineComment> comments = Lists.newArrayList();
     comments.addAll(notes.getDraftBaseComments(author).values());
     comments.addAll(notes.getDraftPsComments(author).values());
-    return comments;
+    return sort(comments);
   }
 
   public List<PatchLineComment> draftByAuthor(ReviewDb db,
       Account.Id author) throws OrmException {
     if (!migration.readComments()) {
-      return db.patchComments().draftByAuthor(author).toList();
+      return sort(db.patchComments().draftByAuthor(author).toList());
     }
 
     Set<String> refNames =
@@ -251,7 +251,7 @@ public class PatchLineCommentsUtil {
       comments.addAll(draftNotes.getDraftBaseComments().values());
       comments.addAll(draftNotes.getDraftPsComments().values());
     }
-    return comments;
+    return sort(comments);
   }
 
   public void insertComments(ReviewDb db, ChangeUpdate update,
@@ -343,4 +343,9 @@ public class PatchLineCommentsUtil {
       }
     });
   }
+
+  private static List<PatchLineComment> sort(List<PatchLineComment> comments) {
+    Collections.sort(comments, ChangeNotes.PatchLineCommentComparator);
+    return comments;
+  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java

@@ -21,7 +21,6 @@ import static org.easymock.EasyMock.expectLastCall;
 import static org.easymock.EasyMock.replay;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Objects;
@@ -349,12 +348,8 @@ public class CommentsTest  {
         Lists.newArrayList(plc4, plc5);
     assertEquals(publishedExpected.size(), publishedActual.size());
     assertEquals(draftExpected.size(), draftActual.size());
-    for (PatchLineComment c : draftExpected) {
+    assertEquals(publishedExpected, publishedActual);
-      assertTrue(draftActual.contains(c));
+    assertEquals(draftExpected, draftActual);
-    }
-    for (PatchLineComment c : publishedExpected) {
-      assertTrue(publishedActual.contains(c));
-    }
   }
 
   private static IAnswer<ResultSet<PatchLineComment>> results(