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.

Sign up for GitHub

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

test-bot: make it configurable to check committer status #761

Closed
wants to merge 1 commit into from
Closed

test-bot: make it configurable to check committer status #761

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bots/tester/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ dependencies {
implementation project(':issuetracker')
implementation project(':json')
implementation project(':vcs')
implementation project(':jcheck')

testImplementation project(':test')
}
2 changes: 2 additions & 0 deletions bots/tester/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
requires org.openjdk.skara.bot;
requires org.openjdk.skara.vcs;
requires org.openjdk.skara.ci;
requires org.openjdk.skara.census;
requires org.openjdk.skara.jcheck;

requires java.logging;

Expand Down
45 changes: 42 additions & 3 deletions bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,17 @@
import org.openjdk.skara.ci.Job;
import org.openjdk.skara.bot.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.census.Census;
import org.openjdk.skara.jcheck.JCheckConfiguration;
import org.openjdk.skara.vcs.Repository;
import org.openjdk.skara.host.HostUser;

import java.io.*;
import java.nio.file.*;
import java.util.*;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import java.util.function.Predicate;

public class TestBot implements Bot {
private static class Observation {
Expand All @@ -55,6 +60,9 @@ private static class Observation {
private final HostedRepository repo;
private final PullRequestUpdateCache cache;
private final Map<String, Observation> states;
private final HostedRepository censusRemote;
private final Path censusDir;
private final boolean checkCommitterStatus;

TestBot(ContinuousIntegration ci,
String approversGroupId,
Expand All @@ -63,7 +71,10 @@ private static class Observation {
List<String> defaultJobs,
String name,
Path storage,
HostedRepository repo) {
HostedRepository repo,
HostedRepository censusRemote,
Path censusDir,
boolean checkCommitterStatus) {
this.ci = ci;
this.approversGroupId = approversGroupId;
this.allowlist = allowlist;
Expand All @@ -74,10 +85,36 @@ private static class Observation {
this.repo = repo;
this.cache = new PullRequestUpdateCache();
this.states = new HashMap<>();
this.censusRemote = censusRemote;
this.censusDir = censusDir;
this.checkCommitterStatus = checkCommitterStatus;
}

@Override
public List<WorkItem> getPeriodicItems() {
Predicate<HostUser> isCommitter = null;
if (checkCommitterStatus) {
try {
var censusRepo = Repository.materialize(censusDir, censusRemote.url(), "master");
var census = Census.parse(censusDir);
var namespace = census.namespace(repo.namespace());
var jcheckConf = repo.fileContents(".jcheck/conf", "master");
var jcheck = JCheckConfiguration.parse(jcheckConf.lines().collect(Collectors.toList()));
var project = census.project(jcheck.general().project());
isCommitter = u -> {
var contributor = namespace.get(u.id());
if (contributor == null) {
return false;
}
return project.isCommitter(contributor.username(), census.version().format());
};
} catch (IOException e) {
throw new UncheckedIOException(e);
}
} else {
isCommitter = u -> true;
}

var ret = new ArrayList<WorkItem>();

var host = repo.webUrl().getHost();
Expand All @@ -91,7 +128,8 @@ public List<WorkItem> getPeriodicItems() {
defaultJobs,
name,
storage,
pr));
pr,
isCommitter));
} else {
// is there a job running for this PR?
var desc = pr.repository().name() + "#" + pr.id();
Expand Down Expand Up @@ -130,7 +168,8 @@ public List<WorkItem> getPeriodicItems() {
defaultJobs,
name,
storage,
pr));
pr,
isCommitter));
}
}
}
Expand Down
12 changes: 11 additions & 1 deletion bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBotFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ public List<Bot> create(BotConfiguration configuration) {
var ret = new ArrayList<Bot>();
var specific = configuration.specific();

var censusDir = storage.resolve("census.git");
try {
Files.createDirectories(censusDir);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
var censusRemote = configuration.repository(specific.get("census").asString());
var checkCommitterStatus = specific.contains("role") &&
specific.get("role").asString().toLowerCase().equals("committer");

var approvers = specific.get("approvers").asString();
var allowlist = specific.get("allowlist").stream().map(JSONValue::asString).collect(Collectors.toSet());
var availableJobs = specific.get("availableJobs").stream().map(JSONValue::asString).collect(Collectors.toList());
Expand All @@ -59,7 +69,7 @@ public List<Bot> create(BotConfiguration configuration) {
var ci = configuration.continuousIntegration(specific.get("ci").asString());
for (var repo : specific.get("repositories").asArray()) {
var hostedRepo = configuration.repository(repo.asString());
ret.add(new TestBot(ci, approvers, allowlist, availableJobs, defaultJobs, name, storage, hostedRepo));
ret.add(new TestBot(ci, approvers, allowlist, availableJobs, defaultJobs, name, storage, hostedRepo, censusRemote, censusDir, checkCommitterStatus));
}

return ret;
Expand Down
6 changes: 4 additions & 2 deletions bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestWorkItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,10 @@ public class TestWorkItem implements WorkItem {
private final Path storage;
private final HostedRepository repository;
private final PullRequest pr;
private final Predicate<HostUser> isCommitter;

TestWorkItem(ContinuousIntegration ci, String approversGroupId, Set<String> allowlist, List<String> availableJobs,
List<String> defaultJobs, String name, Path storage, PullRequest pr) {
List<String> defaultJobs, String name, Path storage, PullRequest pr, Predicate<HostUser> isCommitter) {
this.ci = ci;
this.approversGroupId = approversGroupId;
this.allowlist = allowlist;
Expand All @@ -60,6 +61,7 @@ public class TestWorkItem implements WorkItem {
this.storage = storage;
this.repository = pr.repository();
this.pr = pr;
this.isCommitter = isCommitter;
}

@Override
Expand Down Expand Up @@ -252,7 +254,7 @@ private String display(Job job) {

private boolean validate(HostUser u) {
var forge = pr.repository().forge();
return forge.isMemberOf(approversGroupId, u) || allowlist.contains(u.id());
return isCommitter.test(u) && (forge.isMemberOf(approversGroupId, u) || allowlist.contains(u.id()));
}

@Override
Expand Down
3 changes: 2 additions & 1 deletion bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ void noTestCommentShouldDoNothing(TestInfo testInfo) throws IOException {

var storage = tmp.path().resolve("storage");
var ci = new InMemoryContinuousIntegration();
var bot = new TestBot(ci, "0", Set.of(), List.of(), List.of(), "", storage, upstreamHostedRepo);
var bot = new TestBot(ci, "0", Set.of(), List.of(), List.of(), "",
storage, upstreamHostedRepo, null, null, false);
var runner = new TestBotRunner();

runner.runPeriodicItems(bot);
Expand Down
121 changes: 108 additions & 13 deletions bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestWorkItemTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ void noTestCommentsShouldDoNothing() throws IOException {
pr.author = duke;
pr.comments = List.of();

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);
item.run(scratch);

var comments = pr.comments();
Expand Down Expand Up @@ -99,7 +100,8 @@ void topLevelTestApproveShouldDoNothing() throws IOException {
var testApproveComment = new Comment("0", "/test approve", duke, now, now);
pr.comments = List.of(testApproveComment);

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);
item.run(scratch);

var comments = pr.comments();
Expand Down Expand Up @@ -135,7 +137,8 @@ void topLevelTestCancelShouldDoNothing() throws IOException {
var testApproveComment = new Comment("0", "/test cancel", duke, now, now);
pr.comments = List.of(testApproveComment);

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);
item.run(scratch);

var comments = pr.comments();
Expand Down Expand Up @@ -173,7 +176,8 @@ void testCommentWithMadeUpJobShouldBeError() throws IOException {
var comment = new Comment("0", "/test foobar", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);
item.run(scratch);

var comments = pr.comments();
Expand Down Expand Up @@ -220,7 +224,8 @@ void testCommentFromUnapprovedUserShouldBePending() throws IOException {
var comment = new Comment("0", "/test foobar", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

// Non-existing test group should result in error
item.run(scratch);
Expand Down Expand Up @@ -301,7 +306,8 @@ void cancelAtestCommentShouldBeCancel() throws IOException {
var cancelComment = new Comment("1", "/test cancel", duke, now, now);
pr.comments = new ArrayList<>(List.of(testComment, cancelComment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -342,7 +348,8 @@ void cancellingAPendingTestCommentShouldWork() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -426,7 +433,8 @@ void cancellingApprovedPendingRequestShouldBeCancelled() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -513,7 +521,8 @@ void approvedPendingRequestShouldBeStarted() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -621,7 +630,8 @@ void cancellingApprovedPendingRequestShouldBeCancel() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -746,7 +756,8 @@ void errorWhenCreatingTestJobShouldResultInError() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -834,7 +845,8 @@ void finishedJobShouldResultInFinishedComment() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> true);

item.run(scratch);

Expand Down Expand Up @@ -987,7 +999,8 @@ void userOnApprovelistDoesNotNeedApproval() throws IOException {
var comment = new Comment("0", "/test tier1", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of("0"), available, defaultJobs, name, storage, pr);
var item = new TestWorkItem(ci, approvers, Set.of("0"), available, defaultJobs, name, storage, pr,
u -> true);

var expectedJobId = "null-1337-17-0";
var expectedJob = new InMemoryJob();
Expand Down Expand Up @@ -1023,4 +1036,86 @@ void userOnApprovelistDoesNotNeedApproval() throws IOException {
.contains("0 jobs completed, 1 job running, 7 jobs not yet started"));
}
}

@Test
void testCommentFromNonCommitterShouldRequireApproval() throws IOException {
try (var tmp = new TemporaryDirectory()) {
var ci = new InMemoryContinuousIntegration();
var approvers = "0";
var available = List.of("tier1", "tier2", "tier3");
var defaultJobs = List.of("tier1");
var name = "test";
var storage = tmp.path().resolve("storage");
var scratch = tmp.path().resolve("storage");

var bot = new HostUser(1, "bot", "openjdk [bot]");
var host = new InMemoryHost();
host.currentUserDetails = bot;

var repo = new InMemoryHostedRepository();
repo.host = host;

var pr = new InMemoryPullRequest();
pr.repository = repo;

var duke = new HostUser(0, "duke", "Duke");
host.groups = Map.of(approvers, Set.of(duke));
pr.author = duke;
pr.headHash = new Hash("01234567890123456789012345789012345789");

var now = ZonedDateTime.now();
var comment = new Comment("0", "/test foobar", duke, now, now);
pr.comments = new ArrayList<>(List.of(comment));

var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr,
u -> false);

// Non-existing test group should result in error
item.run(scratch);

var comments = pr.comments();
assertEquals(2, comments.size());
assertEquals(comment, comments.get(0));

var secondComment = comments.get(1);
assertEquals(bot, secondComment.author());

var lines = secondComment.body().split("\n");
assertEquals(2, lines.length);
assertEquals("<!-- TEST ERROR -->", lines[0]);
assertEquals("@duke the test group foobar does not exist", lines[1]);

// Trying to test again should be fine
var thirdComment = new Comment("2", "/test tier1", duke, now, now);
pr.comments.add(thirdComment);
item.run(scratch);

comments = pr.comments();
assertEquals(4, comments.size());
assertEquals(comment, comments.get(0));
assertEquals(secondComment, comments.get(1));
assertEquals(thirdComment, comments.get(2));

var fourthComment = comments.get(3);
assertEquals(bot, fourthComment.author());

lines = fourthComment.body().split("\n");
assertEquals("<!-- TEST PENDING -->", lines[0]);
assertEquals("<!-- 01234567890123456789012345789012345789 -->", lines[1]);
assertEquals("<!-- tier1 -->", lines[2]);
assertEquals("@duke you need to get approval to run the tests in tier1 for commits up until 01234567",
lines[3]);

// Nothing should change if we run it yet again
item.run(scratch);

comments = pr.comments();
assertEquals(4, comments.size());
assertEquals(comment, comments.get(0));
assertEquals(secondComment, comments.get(1));
assertEquals(thirdComment, comments.get(2));
assertEquals(fourthComment, comments.get(3));
}
}

}