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.

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

270: Remove ignored reviews from the summary #437

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
@@ -44,6 +44,7 @@ class CheckRun {
private final List<Review> activeReviews;
private final Set<String> labels;
private final CensusInstance censusInstance;
private final boolean ignoreStaleReviews;

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
private final String progressMarker = "<!-- Anything below this marker will be automatically updated, please do not edit manually! -->";
@@ -53,7 +54,7 @@ class CheckRun {

private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List<Comment> comments,
List<Review> allReviews, List<Review> activeReviews, Set<String> labels,
CensusInstance censusInstance) {
CensusInstance censusInstance, boolean ignoreStaleReviews) {
this.workItem = workItem;
this.pr = pr;
this.prInstance = prInstance;
@@ -63,11 +64,13 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prI
this.labels = new HashSet<>(labels);
this.newLabels = new HashSet<>(labels);
this.censusInstance = censusInstance;
this.ignoreStaleReviews = ignoreStaleReviews;
}

static void execute(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List<Comment> comments,
List<Review> allReviews, List<Review> activeReviews, Set<String> labels, CensusInstance censusInstance) {
var run = new CheckRun(workItem, pr, prInstance, comments, allReviews, activeReviews, labels, censusInstance);
List<Review> allReviews, List<Review> activeReviews, Set<String> labels, CensusInstance censusInstance,
boolean ignoreStaleReviews) {
var run = new CheckRun(workItem, pr, prInstance, comments, allReviews, activeReviews, labels, censusInstance, ignoreStaleReviews);
run.checkStatus();
}

@@ -264,7 +267,11 @@ private Optional<String> getReviewersList(List<Review> reviews) {
.map(review -> {
var entry = " * " + formatReviewer(review.reviewer());
if (!review.hash().equals(pr.headHash())) {
entry += " **Note!** Review applies to " + review.hash();
if (ignoreStaleReviews) {
entry += " 🔄 Re-review required (review applies to " + review.hash() + ")";
} else {
entry += " ⚠️ Review applies to " + review.hash();
}
}
return entry;
})
@@ -328,7 +335,7 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
}

getReviewersList(reviews).ifPresent(reviewers -> {
progressBody.append("\n\n## Approvers\n");
progressBody.append("\n\n## Reviewers\n");
progressBody.append(reviewers);
});

@@ -153,7 +153,7 @@ public void run(Path scratchPath) {
new HostedRepositoryPool(seedPath),
pr,
bot.ignoreStaleReviews());
CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census);
CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census, bot.ignoreStaleReviews());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
@@ -191,16 +191,16 @@ void multipleReviews(TestInfo testInfo) throws IOException {

// Let the status bot inspect the PR
TestBotRunner.runPeriodicItems(checkBot);
assertFalse(authorPr.body().contains("Approvers"));
assertFalse(authorPr.body().contains("Reviewers"));

// Approve it
var reviewerPr = reviewer.pullRequest(authorPr.id());
reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");
reviewerPr.addReview(Review.Verdict.APPROVED, "Reviewers");
TestBotRunner.runPeriodicItems(checkBot);

// Refresh the PR and check that it has been approved
authorPr = author.pullRequest(authorPr.id());
assertTrue(authorPr.body().contains("Approvers"));
assertTrue(authorPr.body().contains("Reviewers"));

// Update the file after approval
editHash = CheckableRepository.appendAndCommit(localRepo, "Now I've gone and changed it");
@@ -219,15 +219,15 @@ void multipleReviews(TestInfo testInfo) throws IOException {
// Check that the review is flagged as stale
TestBotRunner.runPeriodicItems(checkBot);
authorPr = author.pullRequest(authorPr.id());
assertTrue(authorPr.body().contains("Note"));
assertTrue(authorPr.body().contains("Review applies to"));

// Now we can approve it again
reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");
TestBotRunner.runPeriodicItems(checkBot);

// Refresh the PR and check that it has been approved (once) and is no longer stale
authorPr = author.pullRequest(authorPr.id());
assertTrue(authorPr.body().contains("Approvers"));
assertTrue(authorPr.body().contains("Reviewers"));
assertEquals(1, authorPr.body().split("Generated Reviewer", -1).length - 1);
assertTrue(authorPr.reviews().size() >= 1);
assertFalse(authorPr.body().contains("Note"));
@@ -239,7 +239,7 @@ void multipleReviews(TestInfo testInfo) throws IOException {

// Refresh the PR and check that it still only approved once (but two reviews) and is no longer stale
authorPr = author.pullRequest(authorPr.id());
assertTrue(authorPr.body().contains("Approvers"));
assertTrue(authorPr.body().contains("Reviewers"));
assertEquals(1, authorPr.body().split("Generated Reviewer", -1).length - 1);
assertTrue(authorPr.reviews().size() >= 2);
assertFalse(authorPr.body().contains("Note"));
@@ -270,15 +270,15 @@ void selfReview(TestInfo testInfo) throws IOException {

// Let the status bot inspect the PR
TestBotRunner.runPeriodicItems(checkBot);
assertFalse(authorPr.body().contains("Approvers"));
assertFalse(authorPr.body().contains("Reviewers"));

// Approve it
authorPr.addReview(Review.Verdict.APPROVED, "Approved");
TestBotRunner.runPeriodicItems(checkBot);

// Refresh the PR and check that it has been approved
authorPr = author.pullRequest(authorPr.id());
assertTrue(authorPr.body().contains("Approvers"));
assertTrue(authorPr.body().contains("Reviewers"));

// Verify that the check failed
var checks = authorPr.checks(editHash);
@@ -1161,6 +1161,7 @@ void ignoreStale(TestInfo testInfo) throws IOException {

// The PR should be flagged as ready
assertTrue(pr.labels().contains("ready"));
assertFalse(pr.body().contains("Re-review required"));

// Add another commit
editHash = CheckableRepository.replaceAndCommit(localRepo, "Another line");
@@ -1182,6 +1183,7 @@ void ignoreStale(TestInfo testInfo) throws IOException {
// The PR should no longer be ready, as the review is stale
assertFalse(pr.labels().contains("ready"));
assertTrue(pr.labels().contains("rfr"));
assertTrue(pr.body().contains("Re-review required"));
}
}