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

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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter
Filter file types
Beta Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -40,6 +40,7 @@ dependencies {
implementation project(':issuetracker')
implementation project(':json')
implementation project(':vcs')
implementation project(':jcheck')

testImplementation project(':test')
}
@@ -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;

@@ -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 {
@@ -55,6 +60,9 @@
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,
@@ -63,7 +71,10 @@
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;
@@ -74,10 +85,36 @@
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();
@@ -91,7 +128,8 @@
defaultJobs,
name,
storage,
pr));
pr,
isCommitter));
} else {
// is there a job running for this PR?
var desc = pr.repository().name() + "#" + pr.id();
@@ -130,7 +168,8 @@
defaultJobs,
name,
storage,
pr));
pr,
isCommitter));
}
}
}
@@ -51,6 +51,16 @@ public String name() {
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());
@@ -59,7 +69,7 @@ public String name() {
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;
@@ -48,9 +48,10 @@
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;
@@ -60,6 +61,7 @@
this.storage = storage;
this.repository = pr.repository();
this.pr = pr;
this.isCommitter = isCommitter;
}

@Override
@@ -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
@@ -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);
@@ -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();
@@ -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();
@@ -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();
@@ -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();
@@ -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);
@@ -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);

@@ -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);

@@ -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);

@@ -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);

@@ -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);

@@ -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);

@@ -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);

@@ -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();
@@ -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));
}
}

}
ProTip! Use n and p to navigate between commits in a pull request.