Skip to content
Permalink
Browse files
321: No RFR mail is sent if a PR is integrated very quickly
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Mar 25, 2020
1 parent 84586ab commit 899d3b67633722b8f3249f6cf09916d5a13b60c9
Showing 8 changed files with 207 additions and 22 deletions.
@@ -49,9 +49,11 @@ private static Optional<Commit> mergeCommit(Repository localRepo, Hash head) {
static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAuthor hostUserToEmailAuthor,
URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator,
WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated,
Hash base, Hash head, String subjectPrefix) {
return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()),
() -> subjectPrefix + "RFR: " + pr.title(),
Hash base, Hash head, String subjectPrefix, String threadPrefix) {
return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(),
"PR-Base-Hash", base.hex(),
"PR-Thread-Prefix", threadPrefix),
() -> subjectPrefix + threadPrefix + ": " + pr.title(),
() -> "",
() -> ArchiveMessages.composeConversation(pr, localRepo, base, head),
() -> {
@@ -108,9 +110,9 @@ private static String hostUserToCommitterName(HostUserToEmailAuthor hostUserToEm
static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAuthor hostUserToEmailAuthor,
WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification,
ZonedDateTime created, ZonedDateTime updated, Hash lastBase, Hash lastHead, Hash base,
Hash head, int index, ArchiveItem parent, String subjectPrefix) {
Hash head, int index, ArchiveItem parent, String subjectPrefix, String threadPrefix) {
return new ArchiveItem(parent,"ha" + head.hex(), created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()),
() -> String.format("Re: %s[Rev %02d] RFR: %s", subjectPrefix, index, pr.title()),
() -> String.format("Re: %s[Rev %02d] %s: %s", subjectPrefix, index, threadPrefix, pr.title()),
() -> "",
() -> {
if (lastBase.equals(base)) {
@@ -26,7 +26,7 @@
import org.openjdk.skara.email.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.mailinglist.*;
import org.openjdk.skara.vcs.Repository;

@@ -243,9 +243,17 @@ public void run(Path scratchPath) {
// First determine if this PR should be inspected further or not
if (sentMails.isEmpty()) {
var labels = new HashSet<>(pr.labels());
for (var readyLabel : bot.readyLabels()) {
if (!labels.contains(readyLabel)) {
log.fine("PR is not yet ready - missing label '" + readyLabel + "'");

if (pr.state() == Issue.State.OPEN) {
for (var readyLabel : bot.readyLabels()) {
if (!labels.contains(readyLabel)) {
log.fine("PR is not yet ready - missing label '" + readyLabel + "'");
return;
}
}
} else {
if (!labels.contains("integrated")) {
log.fine("Closed PR was not integrated - will not initiate an RFR thread");
return;
}
}
@@ -197,8 +197,8 @@ public List<WorkItem> getPeriodicItems() {
log.info("Fetching all open pull requests");
prs = codeRepo.pullRequests();
} else {
log.info("Fetching only pull requests updated after " + lastPartialUpdate.minus(cooldown));
prs = codeRepo.pullRequests(lastPartialUpdate.minus(cooldown));
log.info("Fetching recently updated pull requests (open and closed)");
prs = codeRepo.pullRequests(ZonedDateTime.now().minus(Duration.ofDays(14)));
lastPartialUpdate = ZonedDateTime.now();
}

@@ -3,7 +3,7 @@
import org.openjdk.skara.email.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.vcs.*;

import java.net.URI;
@@ -60,6 +60,18 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
Hash lastBase = null;
Hash lastHead = null;
int revisionIndex = 0;
var threadPrefix = "RFR";

if (!sentEmails.isEmpty()) {
var first = sentEmails.get(0);
if (first.hasHeader("PR-Thread-Prefix")) {
threadPrefix = first.headerValue("PR-Thread-Prefix");
}
} else {
if (pr.state() != Issue.State.OPEN) {
threadPrefix = "FYI";
}
}

// Check existing generated mails to find which hashes have been previously reported
for (var email : sentEmails) {
@@ -69,10 +81,10 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
var created = email.date();

if (generated.isEmpty()) {
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), curBase, curHead, subjectPrefix);
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), curBase, curHead, subjectPrefix, threadPrefix);
generated.add(first);
} else {
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix);
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
generated.add(revision);
}

@@ -84,10 +96,10 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
// Check if we're at a revision not previously reported
if (!base.equals(lastBase) || !head.equals(lastHead)) {
if (generated.isEmpty()) {
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head, subjectPrefix);
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head, subjectPrefix, threadPrefix);
generated.add(first);
} else {
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0), subjectPrefix);
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
generated.add(revision);
}
}
@@ -78,10 +78,17 @@ private void generate(PullRequest pr, Repository localRepository, Path folder, D
var conf = JCheckConfiguration.from(localRepository, head);
var project = conf.general().jbs() != null ? conf.general().jbs() : conf.general().project();
var id = issue.get().id();
var issueTracker = IssueTracker.from("jira", URI.create("https://bugs.openjdk.java.net"));
var hostedIssue = issueTracker.project(project).issue(id);
if (hostedIssue.isPresent()) {
builder = builder.issue(hostedIssue.get().webUrl().toString());
IssueTracker issueTracker = null;
try {
issueTracker = IssueTracker.from("jira", URI.create("https://bugs.openjdk.java.net"));
} catch (RuntimeException e) {
log.warning("Failed to create Jira issue tracker");
}
if (issueTracker != null) {
var hostedIssue = issueTracker.project(project).issue(id);
if (hostedIssue.isPresent()) {
builder = builder.issue(hostedIssue.get().webUrl().toString());
}
}
}
}
@@ -24,6 +24,7 @@

import org.openjdk.skara.email.EmailAddress;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.issuetracker.Issue;
import org.openjdk.skara.network.URIBuilder;
import org.openjdk.skara.mailinglist.MailingListServerFactory;
import org.openjdk.skara.test.*;
@@ -269,6 +270,159 @@ void simpleArchive(TestInfo testInfo) throws IOException {
}
}

@Test
void archiveIntegrated(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var webrevFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var author = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var ignored = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = MailingListBridgeBot.newBuilder()
.from(from)
.repo(author)
.archive(archive)
.censusRepo(censusBuilder.build())
.list(listAddress)
.ignoredUsers(Set.of(ignored.forge().currentUser().userName()))
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.issueTracker(URIBuilder.base("http://issues.test/browse/").build())
.build();

// 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);
localRepo.push(masterHash, archive.url(), "webrev", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change",
"Change msg\n\nWith several lines");
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "1234: This is a pull request");
pr.setBody("This should not be ready");
pr.setState(Issue.State.CLOSED);

// Run an archive pass
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);

// A PR that isn't ready for review should not be archived
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertFalse(archiveContains(archiveFolder.path(), "This is a pull request"));

// Flag it as ready for review
pr.setBody("This has already been integrated");
pr.addLabel("integrated");

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);

// The archive should now contain an entry
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Subject: FYI: 1234: This is a pull request"));

var updatedHash = CheckableRepository.appendAndCommit(localRepo, "Another change");
localRepo.push(updatedHash, author.url(), "edit");

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);

// The archive should now contain another entry
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Subject: Re: \\[Rev 01\\] FYI: 1234: This is a pull request"));
}
}

@Test
void archiveIntegratedRetainPrefix(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var webrevFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var author = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var ignored = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = MailingListBridgeBot.newBuilder()
.from(from)
.repo(author)
.archive(archive)
.censusRepo(censusBuilder.build())
.list(listAddress)
.ignoredUsers(Set.of(ignored.forge().currentUser().userName()))
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.readyLabels(Set.of("rfr"))
.issueTracker(URIBuilder.base("http://issues.test/browse/").build())
.build();

// 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);
localRepo.push(masterHash, archive.url(), "webrev", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change",
"Change msg\n\nWith several lines");
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "1234: This is a pull request");
pr.setBody("This should be ready");

// Run an archive pass
TestBotRunner.runPeriodicItems(mlBot);
TestBotRunner.runPeriodicItems(mlBot);

// A PR that isn't ready for review should not be archived
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertFalse(archiveContains(archiveFolder.path(), "This is a pull request"));

// Flag it as ready for review
pr.addLabel("rfr");

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);

// The archive should now contain an entry
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Subject: RFR: 1234: This is a pull request"));

// Close it and then push another change
pr.setState(Issue.State.CLOSED);
var updatedHash = CheckableRepository.appendAndCommit(localRepo, "Another change");
localRepo.push(updatedHash, author.url(), "edit");

// Run another archive pass
TestBotRunner.runPeriodicItems(mlBot);

// The archive should now contain another entry - should retain the RFR thread prefix
Repository.materialize(archiveFolder.path(), archive.url(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Subject: Re: \\[Rev 01\\] RFR: 1234: This is a pull request"));
}
}

@Test
void reviewComment(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
@@ -161,7 +161,6 @@ List<TestPullRequest> getPullRequests(TestHostedRepository repository) {
return data.pullRequests.entrySet().stream()
.sorted(Comparator.comparing(Map.Entry::getKey))
.map(pr -> getPullRequest(repository, pr.getKey()))
.filter(pr -> pr.state().equals(Issue.State.OPEN))
.collect(Collectors.toList());
}

@@ -23,6 +23,7 @@
package org.openjdk.skara.test;

import org.openjdk.skara.forge.*;
import org.openjdk.skara.issuetracker.Issue;
import org.openjdk.skara.json.JSONValue;
import org.openjdk.skara.vcs.*;

@@ -70,7 +71,9 @@ public PullRequest pullRequest(String id) {

@Override
public List<PullRequest> pullRequests() {
return new ArrayList<>(host.getPullRequests(this));
return host.getPullRequests(this).stream()
.filter(pr -> pr.state().equals(Issue.State.OPEN))
.collect(Collectors.toList());
}

@Override

0 comments on commit 899d3b6

Please sign in to comment.