diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java index e15a3e120..d8703a714 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java @@ -37,8 +37,10 @@ private ArchiveItem(ArchiveItem parent, String id, ZonedDateTime created, ZonedD this.footer = footer; } - static ArchiveItem from(PullRequest pr, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, Hash base, Hash head) { - return new ArchiveItem(null, "fc", pr.createdAt(), pr.updatedAt(), pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()), + static ArchiveItem from(PullRequest pr, Repository localRepo, URI issueTracker, String issuePrefix, + WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, + ZonedDateTime created, ZonedDateTime updated, Hash base, Hash head) { + return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()), () -> "RFR: " + pr.title(), () -> "", () -> ArchiveMessages.composeConversation(pr, base, head), @@ -49,8 +51,10 @@ static ArchiveItem from(PullRequest pr, Repository localRepo, URI issueTracker, }); } - static ArchiveItem from(PullRequest pr, Repository localRepo, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, Hash lastBase, Hash lastHead, Hash base, Hash head, int index, ArchiveItem parent) { - return new ArchiveItem(parent,"ha" + head.hex(), ZonedDateTime.now(), ZonedDateTime.now(), pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()), + static ArchiveItem from(PullRequest pr, Repository localRepo, WebrevStorage.WebrevGenerator webrevGenerator, + WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated, + Hash lastBase, Hash lastHead, Hash base, Hash head, int index, ArchiveItem parent) { + 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: [Rev %02d] RFR: %s", index, pr.title()), () -> "", () -> ArchiveMessages.composeRevision(pr, localRepo, base, head, lastBase, lastHead), diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java index 5a843b55e..9d19c1ed5 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java @@ -66,12 +66,13 @@ private List generateArchiveItems(List sentEmails, Repositor if (email.hasHeader("PR-Base-Hash")) { var curBase = new Hash(email.headerValue("PR-Base-Hash")); var curHead = new Hash(email.headerValue("PR-Head-Hash")); + var created = email.date(); if (generated.isEmpty()) { - var first = ArchiveItem.from(pr, localRepo, issueTracker, issuePrefix, webrevGenerator, webrevNotification, curBase, curHead); + var first = ArchiveItem.from(pr, localRepo, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), curBase, curHead); generated.add(first); } else { - var revision = ArchiveItem.from(pr, localRepo, webrevGenerator, webrevNotification, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0)); + var revision = ArchiveItem.from(pr, localRepo, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0)); generated.add(revision); } @@ -83,10 +84,10 @@ private List generateArchiveItems(List 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, issueTracker, issuePrefix, webrevGenerator, webrevNotification, base, head); + var first = ArchiveItem.from(pr, localRepo, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head); generated.add(first); } else { - var revision = ArchiveItem.from(pr, localRepo, webrevGenerator, webrevNotification, lastBase, lastHead, base, head, ++revisionIndex, generated.get(0)); + var revision = ArchiveItem.from(pr, localRepo, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0)); generated.add(revision); } } diff --git a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java index 5096b232e..263644a81 100644 --- a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java +++ b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java @@ -1526,6 +1526,72 @@ void cooldown(TestInfo testInfo) throws IOException { } } + @Test + void cooldownNewRevision(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(); + var archiveFolder = new TemporaryDirectory(); + var listServer = new TestMailmanServer(); + var webrevServer = new TestWebrevServer()) { + var bot = credentials.getHostedRepository(); + var author = credentials.getHostedRepository(); + var archive = 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 mlBotBuilder = MailingListBridgeBot.newBuilder() + .from(from) + .repo(bot) + .ignoredUsers(Set.of(bot.forge().currentUser().userName())) + .archive(archive) + .censusRepo(censusBuilder.build()) + .list(listAddress) + .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()); + + // Populate the projects repository + var reviewFile = Path.of("reviewfile.txt"); + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), reviewFile); + 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, "Line 1\nLine 2\nLine 3\nLine 4"); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request"); + pr.setBody("This is now ready"); + + var mlBot = mlBotBuilder.build(); + var mlBotWithCooldown = mlBotBuilder.cooldown(Duration.ofDays(1)).build(); + + TestBotRunner.runPeriodicItems(mlBot); + listServer.processIncoming(); + + // Commit another revision + var updatedHash = CheckableRepository.appendAndCommit(localRepo, "More stuff"); + localRepo.push(updatedHash, author.url(), "edit"); + + // Bot with cooldown configured should not create a new webrev + TestBotRunner.runPeriodicItems(mlBotWithCooldown); + assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1))); + + // But without, it should + TestBotRunner.runPeriodicItems(mlBot); + listServer.processIncoming(); + + // Check the archive + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "webrev.01")); + } + } + @Test void retryAfterCooldown(TestInfo testInfo) throws IOException, InterruptedException { try (var credentials = new HostCredentials(testInfo); @@ -1604,4 +1670,84 @@ void retryAfterCooldown(TestInfo testInfo) throws IOException, InterruptedExcept assertTrue(archiveContains(archiveFolder.path(), "Looks good - " + counter + " -")); } } + + @Test + void retryNewRevisionAfterCooldown(TestInfo testInfo) throws IOException, InterruptedException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(); + var archiveFolder = new TemporaryDirectory(); + var listServer = new TestMailmanServer(); + var webrevServer = new TestWebrevServer()) { + var bot = credentials.getHostedRepository(); + var author = credentials.getHostedRepository(); + var archive = 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 cooldown = Duration.ofMillis(500); + var mlBotBuilder = MailingListBridgeBot.newBuilder() + .from(from) + .repo(bot) + .ignoredUsers(Set.of(bot.forge().currentUser().userName())) + .archive(archive) + .censusRepo(censusBuilder.build()) + .list(listAddress) + .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()); + + // Populate the projects repository + var reviewFile = Path.of("reviewfile.txt"); + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType(), reviewFile); + 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, "Line 1\nLine 2\nLine 3\nLine 4"); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request"); + pr.setBody("This is now ready"); + + var mlBot = mlBotBuilder.cooldown(cooldown).build(); + Thread.sleep(cooldown.toMillis()); + TestBotRunner.runPeriodicItems(mlBot); + listServer.processIncoming(); + + // Make a new revision and run the check within the cooldown period + int counter; + for (counter = 1; counter < 10; ++counter) { + var start = Instant.now(); + var revHash = CheckableRepository.appendAndCommit(localRepo, "more stuff", "Update number - " + counter + " -"); + localRepo.push(revHash, author.url(), "edit"); + + // The bot should not bridge the new revision due to cooldown + TestBotRunner.runPeriodicItems(mlBot); + var elapsed = Duration.between(start, Instant.now()); + if (elapsed.compareTo(cooldown) < 0) { + break; + } else { + log.info("Didn't do the test in time - retrying (elapsed: " + elapsed + " required: " + cooldown + ")"); + listServer.processIncoming(); + cooldown = cooldown.multipliedBy(2); + mlBot = mlBotBuilder.cooldown(cooldown).build(); + } + } + assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1))); + + // But after the cooldown period has passed, it should + Thread.sleep(cooldown.toMillis()); + TestBotRunner.runPeriodicItems(mlBot); + listServer.processIncoming(); + + // Check the archive + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "Update number - " + counter + " -")); + } + } }