Skip to content

Commit

Permalink
2231: JCheck ran twice unnecessarily
Browse files Browse the repository at this point in the history
Reviewed-by: erikj
  • Loading branch information
zhaosongzs committed Apr 12, 2024
1 parent 863ac20 commit 894690e
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 17 deletions.
25 changes: 8 additions & 17 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class CheckRun {
private static final String BACKPORT_CSR_MARKER = "<!-- PullRequestBot backport csr comment -->";
private static final Set<String> PRIMARY_TYPES = Set.of("Bug", "New Feature", "Enhancement", "Task", "Sub-task");
protected static final String CSR_PROCESS_LINK = "https://wiki.openjdk.org/display/csr/Main";
private static final Path JCHECK_CONF_PATH = Path.of(".jcheck", "conf");
private final Set<String> newLabels;
private final boolean reviewCleanBackport;
private final Approval approval;
Expand Down Expand Up @@ -1244,14 +1245,9 @@ private void checkStatus() {
localRepo.lookup(pr.headHash()).ifPresent(this::updateMergeClean);
}

var commits = localRepo.commitMetadata(localRepo.mergeBase(targetHash, pr.headHash()), pr.headHash(), true);
isJCheckConfUpdatedInMergePR = commits.stream().anyMatch(c -> {
try {
return isFileUpdated(Path.of(".jcheck", "conf"), c.hash());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
});
var mergeBaseHash = localRepo.mergeBase(targetHash, pr.headHash());
var commits = localRepo.commitMetadata(mergeBaseHash, pr.headHash(), true);
isJCheckConfUpdatedInMergePR = isFileUpdated(JCHECK_CONF_PATH, mergeBaseHash, pr.headHash());

// JCheck all commits in "Merge PR"
if (workItem.bot.jcheckMerge()) {
Expand Down Expand Up @@ -1318,7 +1314,7 @@ private void checkStatus() {
// from the resulting commit. Not needed if we are overriding the JCheck configuration since
// then we won't use the one in the repo anyway.
if (!hasOverridingJCheckConf &&
(isFileUpdated(Path.of(".jcheck", "conf"), localHash) || isJCheckConfUpdatedInMergePR)) {
(isFileUpdated(JCHECK_CONF_PATH, localRepo.mergeBase(pr.headHash(), targetHash), pr.headHash()) || isJCheckConfUpdatedInMergePR)) {
jcheckType = "source jcheck";
var localJCheckConf = checkablePullRequest.parseJCheckConfiguration(localHash);
var localVisitor = checkablePullRequest.createVisitor(localJCheckConf);
Expand All @@ -1334,7 +1330,7 @@ private void checkStatus() {
needUpdateAdditionalProgresses = true;
}

var confFile = localRepo.lines(Path.of(".jcheck", "conf"), localHash);
var confFile = localRepo.lines(JCHECK_CONF_PATH, localHash);
JdkVersion version = null;
if (confFile.isPresent()) {
var configuration = JCheckConfiguration.parse(confFile.get());
Expand Down Expand Up @@ -1476,13 +1472,8 @@ && approvalNeeded() && approval.approvalComment() && readyToPostApprovalNeededCo
}
}

private boolean isFileUpdated(Path filename, Hash hash) throws IOException {
return !localRepo.files(hash, filename).isEmpty() &&
localRepo.commits(hash.hex(), 1).stream()
.anyMatch(commit -> commit.parentDiffs().stream()
.anyMatch(diff -> diff.patches().stream()
.anyMatch(patch -> (patch.source().path().isPresent() && patch.source().path().get().equals(filename))
|| ((patch.target().path().isPresent() && patch.target().path().get().equals(filename))))));
private boolean isFileUpdated(Path filename, Hash from, Hash to) throws IOException {
return !localRepo.diff(from, to, List.of(filename)).patches().isEmpty();
}

private void updateCSRLabel(JdkVersion version, Map<String, IssueTrackerIssue> csrIssueTrackerIssueMap) {
Expand Down
60 changes: 60 additions & 0 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -2845,6 +2845,66 @@ void testRunJcheckTwice(TestInfo testInfo) throws IOException {
}
}

@Test
void testNotRunJcheckTwice(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();

var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var seedFolder = tempFolder.path().resolve("seed");
var checkBot = PullRequestBot.newBuilder()
.repo(author)
.censusRepo(censusBuilder.build())
.censusLink("https://census.com/{{contributor}}-profile")
.seedStorage(seedFolder)
.build();

// Populate the projects repository
// set the .jcheck/conf without whitespace check
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), Path.of("appendable.txt"), Set.of("author", "reviewers"), "0.1");
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.authenticatedUrl(), "master", true);

// Make a change with a corresponding PR, add a line with whitespace issue
var editHash = CheckableRepository.appendAndCommit(localRepo, "An additional line\r\n");
localRepo.push(editHash, author.authenticatedUrl(), "refs/heads/edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Check the status
TestBotRunner.runPeriodicItems(checkBot);

// Verify that the check succeeded
var checks = pr.checks(editHash);
assertEquals(1, checks.size());
var check = checks.get("jcheck");
assertEquals(CheckStatus.SUCCESS, check.status());
// pr body should not have the process for whitespace
assertFalse(pr.store().body().contains("whitespace"));

localRepo.checkout(masterHash);
// Add whitespace check to .jcheck/conf
var checkConf = tempFolder.path().resolve(".jcheck/conf");
writeToCheckConf(checkConf);
localRepo.add(checkConf);
var updateHash = localRepo.commit("enable whitespace issue check", "testauthor", "ta@none.none");
localRepo.push(updateHash, author.authenticatedUrl(), "master", true);
CheckableRepository.appendAndCommit(localRepo, "An additional line1\r\n");
CheckableRepository.appendAndCommit(localRepo, "An additional line2\r\n");
updateHash = CheckableRepository.appendAndCommit(localRepo, "An additional line3\r\n");
localRepo.push(updateHash, author.authenticatedUrl(), "master", true);

TestBotRunner.runPeriodicItems(checkBot);

// pr body should not have the integrationBlocker for whitespace and reviewer check
assertFalse(pr.store().body().contains("Whitespace errors (failed with updated jcheck configuration in pull request)"));
assertFalse(pr.store().body().contains("Too few reviewers with at least role reviewer found (have 0, need at least 1) (failed with updated jcheck configuration in pull request)"));
}
}

@Test
void testRunJcheckTwiceWithBadConfiguration(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
Expand Down

1 comment on commit 894690e

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.