Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1431: The method TestPullRequest#diff sometimes returns wrong information #1360

Closed
wants to merge 2 commits into from
Closed

1431: The method TestPullRequest#diff sometimes returns wrong information #1360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19b5f65
SKARA-1431
lgxbslgx Aug 20, 2022
52a4d4f
Use the method 'metgeBase'.
lgxbslgx Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions bots/csr/src/test/java/org/openjdk/skara/bots/csr/CSRBotTests.java
View file
Open in desktop
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
24 changes: 16 additions & 8 deletions test/src/main/java/org/openjdk/skara/test/TestPullRequest.java
View file
Open in desktop
Expand Up @@ -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;

Expand Down Expand Up @@ -242,17 +243,24 @@ 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 = sourceHash;
var targetBranch = new Branch(targetRef());
while (!"0".repeat(40).equals(baseHash.hex())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the common commit is more than 40 commits away, return "no diff"? :-)

This is only used in testing, right? If so, this you could get away with it, but I think a comment describing the limitations would be needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are misunderstanding the code. "0".repeat(40) just generates a string with 40 zeros, basically a null hash.

Looping to find the common ancestor is one way of doing it, but I would recommend checking out git merge-base. There is a method for it already on GitRepository.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yes, I misunderstood the code as looping backwards for a maximum of 40. Don't know how I got to that conclusion. My brain is apparently still partially stuck in vacation mode. I don't think I'm used to methods being called on a string literal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looping to find the common ancestor is one way of doing it, but I would recommend checking out git merge-base. There is a method for it already on GitRepository.

Thanks for reminding me. I change the code to use the method mergeBase just now.

if (targetLocalRepository.contains(targetBranch, baseHash)) {
break;
}
baseHash = targetRepository.commit(baseHash).get().parents().get(0);
}
return targetLocalRepository.diff(baseHash, sourceHash);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
Expand Down