1431: The method TestPullRequest#diff sometimes returns wrong informa…

…tion Reviewed-by: ihse, erikj
openjdk · Aug 23, 2022 · cd765e9 · cd765e9 · openjdk-notifier · Aug 23, 2022
1 parent fd16a1d
commit cd765e9
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 8 deletions.
diff --git a/bots/csr/src/test/java/org/openjdk/skara/bots/csr/CSRBotTests.java b/bots/csr/src/test/java/org/openjdk/skara/bots/csr/CSRBotTests.java
@@ -329,6 +329,9 @@ void testBackportCsr(TestInfo testInfo) throws IOException {
             // The bot shouldn't add the `csr` label.
             assertFalse(pr.labelNames().contains("csr"));
 
+            // Test the method `TestPullRequest#diff`.
+            assertEquals(1, pr.diff().patches().size());
+
             // Add `version=bla` to `.jcheck/conf`, set the version as a wrong value
             localRepo.checkout(localRepo.defaultBranch());
             defaultConf = Files.readString(localRepo.root().resolve(".jcheck/conf"), StandardCharsets.UTF_8);
@@ -342,6 +345,9 @@ void testBackportCsr(TestInfo testInfo) throws IOException {
             // The bot shouldn't add the `csr` label.
             assertFalse(pr.labelNames().contains("csr"));
 
+            // Test the method `TestPullRequest#diff`.
+            assertEquals(1, pr.diff().patches().size());
+
             // Set the `version` in `.jcheck/conf` as 17 which is an available version.
             localRepo.checkout(localRepo.defaultBranch());
             defaultConf = Files.readString(localRepo.root().resolve(".jcheck/conf"), StandardCharsets.UTF_8);

diff --git a/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java b/test/src/main/java/org/openjdk/skara/test/TestPullRequest.java
@@ -26,6 +26,7 @@
 import org.openjdk.skara.host.*;
 import org.openjdk.skara.issuetracker.IssueProject;
 import org.openjdk.skara.network.URIBuilder;
+import org.openjdk.skara.vcs.Branch;
 import org.openjdk.skara.vcs.Diff;
 import org.openjdk.skara.vcs.Hash;
 
@@ -242,17 +243,17 @@ public URI headUrl() {
     @Override
     public Diff diff() {
         try {
-            var targetHash = targetRepository.branchHash(targetRef());
             var targetLocalRepository = targetRepository.localRepository();
             var sourceLocalRepository = sourceRepository.localRepository();
-            if (targetLocalRepository.root().equals(sourceLocalRepository.root())) {
-                // same target and source repo, must contain both commits
-                return targetLocalRepository.diff(targetHash, headHash());
-            } else {
-                var uri = URI.create("file://" + sourceLocalRepository.root().toString());
-                var fetchHead = targetLocalRepository.fetch(uri, sourceRef);
-                return targetLocalRepository.diff(targetHash, fetchHead);
+            var sourceHash = headHash();
+            if (!targetLocalRepository.root().equals(sourceLocalRepository.root())) {
+                // The target and source repo are not same, fetch the source branch
+                var sourceUri = URI.create("file://" + sourceLocalRepository.root().toString());
+                sourceHash = targetLocalRepository.fetch(sourceUri, sourceRef);
             }
+            // Find the base hash of the source and target branches.
+            var baseHash = targetLocalRepository.mergeBase(sourceHash, targetRepository.branchHash(targetRef()));
+            return targetLocalRepository.diff(baseHash, sourceHash);
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }