From 6aecd8efdef42b6592b01e6a51c0b084e03fba6a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 16 May 2014 15:52:23 +0200 Subject: [PATCH] Add basic test for PatchListCache that checks the returned patches The tests cover the current behaviour. At the moment when two patch sets are compared the returned file list may be incorrect and contain too many files. This is the case if the old patch set contains modifications that were reverted in the new patch set or if there was a rebase between the two patch sets that touched files that are not contained in the new patch set. As commented in the tests they check the current behaviour which is incorrect. The problems will be fixed in a follow-up change that then adapts these tests to check the correct behaviour. Change-Id: I1ed59dc25a40a8de4b28f824187e37983fac1072 Signed-off-by: Edwin Kempin --- .../server/change/PatchListCacheIT.java | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java new file mode 100644 index 0000000000..4088cd1284 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -0,0 +1,242 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.server.change; + +import static com.google.gerrit.acceptance.GitUtil.add; +import static com.google.gerrit.acceptance.GitUtil.amendCommit; +import static com.google.gerrit.acceptance.GitUtil.createCommit; +import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static com.google.gerrit.acceptance.GitUtil.rm; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GitUtil.Commit; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; +import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.Patch.ChangeType; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListEntry; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; + +import org.eclipse.jgit.api.ResetCommand.ResetType; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +public class PatchListCacheIT extends AbstractDaemonTest { + private static String SUBJECT_1 = "subject 1"; + private static String SUBJECT_2 = "subject 2"; + private static String SUBJECT_3 = "subject 3"; + private static String FILE_A = "a.txt"; + private static String FILE_B = "b.txt"; + private static String FILE_C = "c.txt"; + private static String FILE_D = "d.txt"; + + @Inject + private PatchListCache patchListCache; + + @Test + public void listPatchesAgainstBase() throws GitAPIException, IOException, + PatchListNotAvailableException, OrmException, RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1, 1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + + // Compare Change 1,1 with Base (+FILE_A, -FILE_D) + List entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + + // Change 1,2 (+FILE_A, +FILE_B, -FILE_D) + add(git, FILE_B, "2"); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + entries = getCurrentPatches(c.getChangeId()); + + // Compare Change 1,2 with Base (+FILE_A, +FILE_B, -FILE_D) + assertEquals(4, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertAdded(FILE_B, entries.get(2)); + assertDeleted(FILE_D, entries.get(3)); + } + + @Test + public void listPatchesAgainstBaseWithRebase() throws GitAPIException, + IOException, PatchListNotAvailableException, OrmException, + RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + List entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + + // Change 2,1 (+FILE_B) + git.reset().setMode(ResetType.HARD).setRef("HEAD~1").call(); + add(git, FILE_B, "2"); + createCommit(git, admin.getIdent(), SUBJECT_3); + pushHead(git, "refs/for/master", false); + + // Change 1,2 (+FILE_A, -FILE_D)) + git.cherryPick().include(c.getCommit()).call(); + pushHead(git, "refs/for/master", false); + + // Compare Change 1,2 with Base (+FILE_A, -FILE_D)) + entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + } + + @Test + public void listPatchesAgainstOtherPatchSet() throws GitAPIException, + IOException, PatchListNotAvailableException, OrmException, + RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, +FILE_C, -FILE_D) + add(git, FILE_A, "1"); + add(git, FILE_C, "3"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + ObjectId a = getCurrentRevisionId(c.getChangeId()); + + // Change 1,2 (+FILE_A, +FILE_B, -FILE_D) + add(git, FILE_B, "2"); + rm(git, FILE_C); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + ObjectId b = getCurrentRevisionId(c.getChangeId()); + + // Compare Change 1,1 with Change 1,2 + // expected: +FILE_B + // actual: +FILE_B, -FILE_C + // -FILE_C is wrongly returned, it is not contained in Change 1,2 + // but was only added in Change 1,1 + List entries = getPatches(a, b); + assertEquals(3, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_B, entries.get(1)); + assertDeleted(FILE_C, entries.get(2)); + } + + @Test + public void listPatchesAgainstOtherPatchSetWithRebase() + throws GitAPIException, IOException, PatchListNotAvailableException, + OrmException, RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + ObjectId a = getCurrentRevisionId(c.getChangeId()); + + // Change 2,1 (+FILE_B) + git.reset().setMode(ResetType.HARD).setRef("HEAD~1").call(); + add(git, FILE_B, "2"); + createCommit(git, admin.getIdent(), SUBJECT_3); + pushHead(git, "refs/for/master", false); + + // Change 1,2 (+FILE_A, +FILE_C, -FILE_D) + git.cherryPick().include(c.getCommit()).call(); + add(git, FILE_C, "2"); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + ObjectId b = getCurrentRevisionId(c.getChangeId()); + + // Compare Change 1,1 with Change 1,2 + // expected: +FILE_C + // actual: +FILE_B, +FILE_C + // +FILE_B is wrongly returned, it is neither contained in Change 1,1 + // nor in Change 1,2, but was only changed due to the rebase + // on Change 2,1 + List entries = getPatches(a, b); + assertEquals(3, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_B, entries.get(1)); + assertAdded(FILE_C, entries.get(2)); + } + + private static void assertAdded(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.ADDED, e.getChangeType()); + } + + private static void assertModified(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.MODIFIED, e.getChangeType()); + } + + private static void assertDeleted(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.DELETED, e.getChangeType()); + } + + private static void assertName(String expectedNewName, PatchListEntry e) { + assertEquals(expectedNewName, e.getNewName()); + assertNull(e.getOldName()); + } + + private List getCurrentPatches(String changeId) + throws PatchListNotAvailableException, OrmException, RestApiException { + return patchListCache.get(getKey(null, getCurrentRevisionId(changeId))).getPatches(); + } + + private List getPatches(ObjectId revisionIdA, ObjectId revisionIdB) + throws PatchListNotAvailableException, OrmException { + return patchListCache.get(getKey(revisionIdA, revisionIdB)).getPatches(); + } + + private PatchListKey getKey(ObjectId revisionIdA, ObjectId revisionIdB) throws OrmException { + return new PatchListKey(project, revisionIdA, revisionIdB, Whitespace.IGNORE_NONE); + } + + private ObjectId getCurrentRevisionId(String changeId) throws RestApiException { + return ObjectId.fromString(gApi.changes().id(changeId).get().currentRevision); + } +}