From 05ccb2308232ca8835c07bbe1e169d099bca723f Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 10 Dec 2018 13:00:37 -0800 Subject: [PATCH] CommentJsonMigratorTest: Check change equivalence more explicitly ChangeBundle is pretty tightly coupled to the ReviewDb migration, and contains a lot of fuzzy-matching cases that aren't really relevant for direct comparison of NoteDb changes. We want to delete it sooner rather than later. There are two important properties being tested here: * The draft and published Comment entities are exactly equal before and after migration. * Migration doesn't affect anything else about the NoteDb commits, specifically the commit messages. Change the implementation of assertNoDifferences to test these directly instead of relying on ChangeBundle. This new implementation subsumes the other assertLogEqualExceptTrees calls in the test method bodies. Change-Id: I5955674ff30eaa6664217693d6810454dc0df62c --- .../notedb/CommentJsonMigratorTest.java | 91 ++++++++++++++++--- 1 file changed, 80 insertions(+), 11 deletions(-) diff --git a/javatests/com/google/gerrit/server/notedb/CommentJsonMigratorTest.java b/javatests/com/google/gerrit/server/notedb/CommentJsonMigratorTest.java index b9027bc175..b63adabf74 100644 --- a/javatests/com/google/gerrit/server/notedb/CommentJsonMigratorTest.java +++ b/javatests/com/google/gerrit/server/notedb/CommentJsonMigratorTest.java @@ -38,6 +38,7 @@ import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.testing.TestChanges; import com.google.inject.Inject; import java.io.ByteArrayOutputStream; +import java.util.Collection; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.jgit.junit.TestRepository; @@ -185,7 +186,6 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { // Comments at each commit all have JSON format. ImmutableList newLog = log(project, RefNames.changeMetaRef(c.getId())); - assertLogEqualExceptTrees(newLog, oldLog); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(0))).isEmpty(); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(1))).isEmpty(); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(2))) @@ -297,7 +297,6 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { // Comments at each commit all have JSON format. ImmutableList newOwnerLog = log(allUsers, RefNames.refsDraftComments(c.getId(), changeOwner.getAccountId())); - assertLogEqualExceptTrees(newOwnerLog, oldOwnerLog); assertThat(getLegacyFormatMapForDraftComments(notes, newOwnerLog.get(0))) .containsExactly(ownerCommentPs1.key, false); assertThat(getLegacyFormatMapForDraftComments(notes, newOwnerLog.get(1))) @@ -305,7 +304,6 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { ImmutableList newOtherLog = log(allUsers, RefNames.refsDraftComments(c.getId(), otherUser.getAccountId())); - assertLogEqualExceptTrees(newOtherLog, oldOtherLog); assertThat(getLegacyFormatMapForDraftComments(notes, newOtherLog.get(0))) .containsExactly(otherCommentPs1.key, false); } @@ -392,7 +390,6 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { // Comments at each commit all have JSON format. ImmutableList newLog = log(project, RefNames.changeMetaRef(c.getId())); - assertLogEqualExceptTrees(newLog, oldLog); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(0))).isEmpty(); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(1))).isEmpty(); assertThat(getLegacyFormatMapForPublishedComments(notes, newLog.get(2))).isEmpty(); @@ -491,17 +488,35 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { } private ImmutableList log(Project.NameKey project, String refName) throws Exception { - try (Repository repo = repoManager.openRepository(project); - RevWalk rw = new RevWalk(repo)) { + try (Repository repo = repoManager.openRepository(project)) { + return log(repo, refName); + } + } + + private ImmutableList log(Repository repo, String refName) throws Exception { + try (RevWalk rw = new RevWalk(repo)) { rw.sort(RevSort.TOPO); rw.sort(RevSort.REVERSE); Ref ref = repo.exactRef(refName); - checkArgument(ref != null, "missing ref: %s", refName); + if (ref == null) { + return ImmutableList.of(); + } rw.markStart(rw.parseCommit(ref.getObjectId())); return ImmutableList.copyOf(rw); } } + private ImmutableListMultimap logAll( + Project.NameKey project, Collection refs) throws Exception { + ImmutableListMultimap.Builder logs = ImmutableListMultimap.builder(); + try (Repository repo = repoManager.openRepository(project)) { + for (Ref r : refs) { + logs.putAll(r.getName(), log(repo, r.getName())); + } + } + return logs.build(); + } + private static void assertLogEqualExceptTrees( ImmutableList actualLog, ImmutableList expectedLog) { assertThat(actualLog).hasSize(expectedLog.size()); @@ -522,9 +537,63 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest { } private void assertNoDifferences(ChangeNotes actual, ChangeNotes expected) throws Exception { - assertThat( - ChangeBundle.fromNotes(commentsUtil, actual) - .differencesFrom(ChangeBundle.fromNotes(commentsUtil, expected))) - .isEmpty(); + checkArgument( + actual.getChangeId().equals(expected.getChangeId()), + "must be same change: %s != %s", + actual.getChangeId(), + expected.getChangeId()); + + // Parsed comment representations are equal. + // TODO(dborowitz): Comparing collections directly would be much easier, but Comment doesn't + // have a proper equals; switch to that when the issues with + // https://gerrit-review.googlesource.com/c/gerrit/+/207013 are resolved. + assertCommentsEqual( + commentsUtil.draftByChange(null, actual), commentsUtil.draftByChange(null, expected)); + assertCommentsEqual( + commentsUtil.publishedByChange(null, actual), + commentsUtil.publishedByChange(null, expected)); + + // Change metadata is equal. + assertLogEqualExceptTrees( + log(project, actual.getRefName()), log(project, expected.getRefName())); + + // Logs of all draft refs are equal. + ImmutableListMultimap actualDraftLogs = + logAll(allUsersName, commentsUtil.getDraftRefs(actual.getChangeId())); + ImmutableListMultimap expectedDraftLogs = + logAll(allUsersName, commentsUtil.getDraftRefs(expected.getChangeId())); + assertThat(actualDraftLogs.keySet()) + .named("draft ref names") + .containsExactlyElementsIn(expectedDraftLogs.keySet()); + for (String refName : actualDraftLogs.keySet()) { + assertLogEqualExceptTrees(actualDraftLogs.get(refName), actualDraftLogs.get(refName)); + } + } + + private static void assertCommentsEqual(List actualList, List expectedList) { + ImmutableMap actualMap = byKey(actualList); + ImmutableMap expectedMap = byKey(expectedList); + assertThat(actualMap.keySet()).isEqualTo(expectedMap.keySet()); + for (Comment.Key key : actualMap.keySet()) { + Comment actual = actualMap.get(key); + Comment expected = expectedMap.get(key); + assertThat(actual.key).isEqualTo(expected.key); + assertThat(actual.lineNbr).isEqualTo(expected.lineNbr); + assertThat(actual.author).isEqualTo(expected.author); + assertThat(actual.getRealAuthor()).isEqualTo(expected.getRealAuthor()); + assertThat(actual.writtenOn).isEqualTo(expected.writtenOn); + assertThat(actual.side).isEqualTo(expected.side); + assertThat(actual.message).isEqualTo(expected.message); + assertThat(actual.parentUuid).isEqualTo(expected.parentUuid); + assertThat(actual.range).isEqualTo(expected.range); + assertThat(actual.tag).isEqualTo(expected.tag); + assertThat(actual.revId).isEqualTo(expected.revId); + assertThat(actual.serverId).isEqualTo(expected.serverId); + assertThat(actual.unresolved).isEqualTo(expected.unresolved); + } + } + + private static ImmutableMap byKey(List comments) { + return comments.stream().collect(toImmutableMap(c -> c.key, c -> c)); } }