Skip to content
Permalink
Browse files
169: If there are no changes after squashing, jcheck should fail
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Nov 25, 2019
1 parent bdaf913 commit 6ef64684a0e98f9dd569b1bae181e9b6812e90b6
@@ -23,9 +23,9 @@
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.Commit;
import org.openjdk.skara.vcs.openjdk.Issue;

import java.io.*;
@@ -468,20 +468,26 @@ private void checkStatus() {
log.info("Starting to run jcheck on PR head");
pr.createCheck(checkBuilder.build());
var localHash = prInstance.commit(censusInstance.namespace(), censusDomain, null);

// Try to rebase
boolean rebasePossible = true;
var ignored = new PrintWriter(new StringWriter());
var rebasedHash = prInstance.rebase(localHash, ignored);
if (rebasedHash.isEmpty()) {
rebasePossible = false;
} else {
localHash = rebasedHash.get();
}
PullRequestCheckIssueVisitor visitor = prInstance.createVisitor(localHash, censusInstance);
List<String> additionalErrors;
if (!localHash.equals(prInstance.baseHash())) {
// Try to rebase
var ignored = new PrintWriter(new StringWriter());
var rebasedHash = prInstance.rebase(localHash, ignored);
if (rebasedHash.isEmpty()) {
rebasePossible = false;
} else {
localHash = rebasedHash.get();
}

// Determine current status
var visitor = prInstance.executeChecks(localHash, censusInstance);
var additionalErrors = botSpecificChecks();
// Determine current status
prInstance.executeChecks(localHash, censusInstance, visitor);
additionalErrors = botSpecificChecks();
}
else {
additionalErrors = List.of("This PR contains no changes");
}
updateCheckBuilder(checkBuilder, visitor, additionalErrors);
updateReadyForReview(visitor, additionalErrors);

@@ -91,7 +91,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
}
}

var issues = prInstance.executeChecks(localHash, censusInstance);
var issues = prInstance.createVisitor(localHash, censusInstance);
prInstance.executeChecks(localHash, censusInstance, issues);
if (!issues.getMessages().isEmpty()) {
reply.print("Your merge request cannot be fulfilled at this time, as ");
reply.println("your changes failed the final jcheck:");
@@ -99,6 +99,10 @@ private String commitMessage(List<Review> activeReviews, Namespace namespace, bo
private Hash commitSquashed(List<Review> activeReviews, Namespace namespace, String censusDomain, String sponsorId) throws IOException {
localRepo.checkout(baseHash, true);
localRepo.squash(headHash);
if (localRepo.isClean()) {
// There are no changes remaining after squashing
return baseHash;
}

Author committer;
Author author;
@@ -205,16 +209,17 @@ Set<Path> changedFiles() throws IOException {
return ret;
}

PullRequestCheckIssueVisitor executeChecks(Hash localHash, CensusInstance censusInstance) throws Exception {
PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance censusInstance) throws IOException {
var checks = JCheck.checks(localRepo(), censusInstance.census(), localHash);
var visitor = new PullRequestCheckIssueVisitor(checks);
return new PullRequestCheckIssueVisitor(checks);
}

void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor) throws Exception {
try (var issues = JCheck.check(localRepo(), censusInstance.census(), CommitMessageParsers.v1, "HEAD~1..HEAD",
localHash, new HashMap<>(), new HashSet<>())) {
for (var issue : issues) {
issue.accept(visitor);
}
}

return visitor;
}
}
@@ -87,7 +87,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
}
}

var issues = prInstance.executeChecks(localHash, censusInstance);
var issues = prInstance.createVisitor(localHash, censusInstance);
prInstance.executeChecks(localHash, censusInstance, issues);
if (!issues.getMessages().isEmpty()) {
reply.print("Your merge request cannot be fulfilled at this time, as ");
reply.println("your changes failed the final jcheck:");
@@ -1097,4 +1097,38 @@ void retryOnException(TestInfo testInfo) throws IOException {
assertEquals(CheckStatus.SUCCESS, check.status());
}
}

@Test
void noCommit(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 checkBot = new PullRequestBot(author, censusBuilder.build(), "master");

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.url(), "master", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "master");
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

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

// Verify that the check failed
var checks = pr.checks(editHash);
assertEquals(1, checks.size());
var check = checks.get("jcheck");
assertEquals(CheckStatus.FAILURE, check.status());
}
}
}
@@ -30,6 +30,7 @@
import org.junit.jupiter.api.*;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
import java.util.Set;
import java.util.stream.Collectors;
@@ -482,14 +483,16 @@ void autoRebase(TestInfo testInfo) throws IOException {
@Test
void retryOnFailure(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var tempFolder = new TemporaryDirectory();
var censusFolder = new TemporaryDirectory()) {

var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var censusBuilder = credentials.getCensusBuilder()
.addCommitter(author.forge().currentUser().id())
.addReviewer(integrator.forge().currentUser().id());
var mergeBot = new PullRequestBot(integrator, censusBuilder.build(), "master");
var censusRepo = censusBuilder.build();
var mergeBot = new PullRequestBot(integrator, censusRepo, "master");

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
@@ -509,15 +512,20 @@ void retryOnFailure(TestInfo testInfo) throws IOException {
// Let the bot check it
TestBotRunner.runPeriodicItems(mergeBot);

// Pre-push to cause a failure
localRepo.push(editHash, author.url(), "master");
// Break the census to cause an exception
var localCensus = Repository.materialize(censusFolder.path(), censusRepo.url(), "+master:current_census");
var currentCensusHash = localCensus.resolve("current_census").orElseThrow();
Files.writeString(censusFolder.path().resolve("contributors.xml"), "This is not xml", StandardCharsets.UTF_8);
localCensus.add(censusFolder.path().resolve("contributors.xml"));
var badCensusHash = localCensus.commit("Bad census update", "duke", "duke@openjdk.org");
localCensus.push(badCensusHash, censusRepo.url(), "master", true);

// Attempt a merge (without triggering another check)
pr.addComment("/integrate");
assertThrows(RuntimeException.class, () -> TestBotRunner.runPeriodicItems(mergeBot, wi -> wi instanceof CheckWorkItem));

// Restore the master branch
localRepo.push(masterHash, author.url(), "master", true);
// Restore the census
localCensus.push(currentCensusHash, censusRepo.url(), "master", true);

// The bot should now retry
TestBotRunner.runPeriodicItems(mergeBot, wi -> wi instanceof CheckWorkItem);

0 comments on commit 6ef6468

Please sign in to comment.