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

Jump to bottom

453: Merge commit not included in notification email #710

Closed
wants to merge 1 commit into from
Closed

453: Merge commit not included in notification email #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 25 additions & 15 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@
import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

class MailingListNotifier implements Notifier, RepositoryListener {
private final MailingList list;
Expand Down Expand Up @@ -137,17 +138,14 @@ private String tagToSubject(HostedRepository repository, Hash hash, Tag tag) {

private List<Commit> filterPrCommits(HostedRepository repository, Repository localRepository, List<Commit> commits, Branch branch) throws NonRetriableException {
var ret = new ArrayList<Commit>();
var mergedHashes = new HashSet<Hash>();
var mergedCommits = new HashSet<Hash>();

for (var commit : commits) {
if (mergedHashes.contains(commit.hash())) {
log.info("Commit " + commit.hash() + " belongs to a merge PR - skipping");
continue;
}

var candidates = repository.findPullRequestsWithComment(null, "Pushed as commit " + commit.hash() + ".");
if (candidates.size() != 1) {
log.warning("Commit " + commit.hash() + " matches " + candidates.size() + " pull requests - expected 1");
if (candidates.size() > 1) {
log.warning("Commit " + commit.hash() + " matches " + candidates.size() + " pull requests - expected 1");
}
ret.add(commit);
continue;
}
Expand All @@ -161,17 +159,29 @@ private List<Commit> filterPrCommits(HostedRepository repository, Repository loc
}

// For a merge PR, many other of these commits could belong here as well
try {
localRepository.fetch(repository.url(), candidate.fetchRef());
var baseHash = PullRequestUtils.baseHash(candidate, localRepository);
var prCommits = localRepository.commitMetadata(baseHash, candidate.headHash());
prCommits.forEach(prCommit -> mergedHashes.add(prCommit.hash()));
} catch (IOException e) {
log.warning("Could not fetch commits from " + prLink + " - cannot see if the belong to the PR");
if (commit.parents().size() > 1) {
if (!PullRequestUtils.isMerge(candidate)) {
log.warning("Merge commit from non-merge PR?");
ret.add(commit);
continue;
}

// For a merge PR, the first parent is always the target branch, so skip that one
for (int i = 1; i < commit.parents().size(); ++i) {
try {
localRepository.commitMetadata(commit.parents().get(0), commit.parents().get(i))
.forEach(c -> mergedCommits.add(c.hash()));
} catch (IOException e) {
log.warning("Unable to check if commits between " + commit.parents().get(0) + " and "
+ commit.parents().get(i) + " were brought in through merging in " + prLink);
}
}
}
}

return ret;
return ret.stream()
.filter(c -> !mergedCommits.contains(c.hash()))
.collect(Collectors.toList());
}

private void sendCombinedCommits(HostedRepository repository, List<Commit> commits, Branch branch) throws NonRetriableException {
Expand Down
175 changes: 175 additions & 0 deletions ...ify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@
import org.openjdk.skara.test.*;

import java.io.IOException;
import java.nio.file.Files;
import java.time.Duration;
import java.util.*;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -181,6 +182,84 @@ void testMailingListMultiple(TestInfo testInfo) throws IOException {
}
}

@Test
void testMailingListMerge(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var repo = credentials.getHostedRepository();
var repoFolder = tempFolder.path().resolve("repo");
var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
credentials.commitLock(localRepo);
localRepo.pushAll(repo.url());

var listAddress = EmailAddress.parse(listServer.createList("test"));
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var tagStorage = createTagStorage(repo);
var branchStorage = createBranchStorage(repo);
var prStateStorage = createPullRequestStateStorage(repo);
var storageFolder = tempFolder.path().resolve("storage");

var sender = EmailAddress.from("duke", "duke@duke.duke");
var notifyBot = NotifyBot.newBuilder()
.repository(repo)
.storagePath(storageFolder)
.branches(Pattern.compile("master"))
.tagStorageBuilder(tagStorage)
.branchStorageBuilder(branchStorage)
.prStateStorageBuilder(prStateStorage)
.build();
var updater = MailingListNotifier.newBuilder()
.list(mailmanList)
.recipient(listAddress)
.sender(sender)
.reportNewTags(false)
.reportNewBranches(false)
.reportNewBuilds(false)
.build();
updater.attachTo(notifyBot);

// No mail should be sent on the first run as there is no history
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

var editHash1 = CheckableRepository.appendAndCommit(localRepo, "Another line", "23456789: More fixes",
"first_author", "first@author.example.com");
localRepo.push(editHash1, repo.url(), "master");
var editHash2 = CheckableRepository.appendAndCommit(localRepo, "Yet another line", "3456789A: Even more fixes",
"another_author", "another@author.example.com");
localRepo.checkout(editHash1, true);
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Something else");
localRepo.add(unrelated);
localRepo.commit("Unrelated", "unrelated_author", "unrelated@author.example.com");
localRepo.merge(editHash2);
var mergeHash = localRepo.commit("Merge", "merge_author", "merge@author.example.com");
localRepo.push(mergeHash, repo.url(), "master");

TestBotRunner.runPeriodicItems(notifyBot);
listServer.processIncoming();

var conversations = mailmanList.conversations(Duration.ofDays(1));
var email = conversations.get(0).first();
assertEquals(listAddress, email.sender());
assertEquals(EmailAddress.from("merge_author", "merge@author.example.com"), email.author());
assertEquals(email.recipients(), List.of(listAddress));
assertTrue(email.subject().contains(": 4 new changesets"));
assertFalse(email.subject().contains("master"));
assertTrue(email.body().contains("Changeset: " + editHash1.abbreviate()));
assertTrue(email.body().contains("23456789: More fixes"));
assertTrue(email.body().contains("Changeset: " + editHash2.abbreviate()));
assertTrue(email.body().contains("3456789A: Even more fixes"));
assertFalse(email.body().contains(masterHash.abbreviate()));
assertTrue(email.hasHeader("X-Git-URL"));
assertEquals(repo.webUrl().toString(), email.headerValue("X-Git-URL"));
assertTrue(email.hasHeader("X-Git-Changeset"));
assertEquals(editHash1.hex(), email.headerValue("X-Git-Changeset"));
}
}

@Test
void testMailingListSponsored(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
Expand Down Expand Up @@ -520,6 +599,102 @@ void testMailingListPR(TestInfo testInfo) throws IOException {
}
}

@Test
void testMailingListMergePR(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var repo = credentials.getHostedRepository();
var repoFolder = tempFolder.path().resolve("repo");
var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
credentials.commitLock(localRepo);
localRepo.pushAll(repo.url());

var listAddress = EmailAddress.parse(listServer.createList("test"));
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var tagStorage = createTagStorage(repo);
var branchStorage = createBranchStorage(repo);
var prStateStorage = createPullRequestStateStorage(repo);
var storageFolder = tempFolder.path().resolve("storage");

var sender = EmailAddress.from("duke", "duke@duke.duke");
var notifyBot = NotifyBot.newBuilder()
.repository(repo)
.storagePath(storageFolder)
.branches(Pattern.compile("master"))
.tagStorageBuilder(tagStorage)
.branchStorageBuilder(branchStorage)
.prStateStorageBuilder(prStateStorage)
.build();
var updater = MailingListNotifier.newBuilder()
.list(mailmanList)
.recipient(listAddress)
.sender(sender)
.reportNewTags(false)
.reportNewBranches(false)
.reportNewBuilds(false)
.mode(MailingListNotifier.Mode.PR)
.build();
updater.attachTo(notifyBot);

// No mail should be sent on the first run as there is no history
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

var editHash1 = CheckableRepository.appendAndCommit(localRepo, "Another line", "23456789: More fixes",
"first_author", "first@author.example.com");
localRepo.push(editHash1, repo.url(), "edit");
CheckableRepository.appendAndCommit(localRepo, "And another line", "12345678: And more fixes",
"second_author", "second@author.example.com");
var editHash2 = CheckableRepository.appendAndCommit(localRepo, "Yet another line", "3456789A: Even more fixes",
"another_author", "another@author.example.com");
localRepo.checkout(editHash1, true);
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Something else");
localRepo.add(unrelated);
localRepo.commit("Unrelated", "unrelated_author", "unrelated@author.example.com");
localRepo.merge(editHash2);
var mergeHash = localRepo.commit("Merge", "merge_author", "merge@author.example.com");
localRepo.push(mergeHash, repo.url(), "edit");

var pr = credentials.createPullRequest(repo, "master", "edit", "Merge test");

// PR hasn't been integrated yet, so there should be no mail
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

// Simulate an RFR email
var rfr = Email.create("[repo/branch] RFR: Merge test", "PR:\n" + pr.webUrl().toString())
.author(EmailAddress.from("duke", "duke@duke.duke"))
.recipient(listAddress)
.build();
mailmanList.post(rfr);
listServer.processIncoming();

// And an integration
pr.addComment("Pushed as commit " + mergeHash.hex() + ".");
localRepo.push(mergeHash, repo.url(), "master");

TestBotRunner.runPeriodicItems(notifyBot);
listServer.processIncoming();

var conversations = mailmanList.conversations(Duration.ofDays(1));
conversations.sort(Comparator.comparing(conversation -> conversation.first().subject()));
assertEquals(2, conversations.size());

var prConversation = conversations.get(0);
var pushConversation = conversations.get(1);
assertEquals(1, prConversation.allMessages().size());

var pushEmail = pushConversation.first();
assertEquals(listAddress, pushEmail.sender());
assertEquals(EmailAddress.from("unrelated_author", "unrelated@author.example.com"), pushEmail.author());
assertEquals(pushEmail.recipients(), List.of(listAddress));
assertTrue(pushEmail.subject().contains("2 new changesets"));
}
}

@Test
void testMailingListPROnce(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
Expand Down