diff --git a/.jcheck/conf b/.jcheck/conf index 8222f65e9..c2354f7f9 100644 --- a/.jcheck/conf +++ b/.jcheck/conf @@ -38,4 +38,4 @@ domain=openjdk.org files=.*\.java$|.*\.yml$|.*\.gradle$|.*.\txt$ [checks "reviewers"] -minimum=1 +reviewers=1 diff --git a/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java b/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java index a64084a25..73aa36c93 100644 --- a/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java +++ b/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java @@ -118,8 +118,7 @@ private static INI convert(INI old) { config.add("message=Merge"); config.add("[checks \"reviewers\"]"); - config.add("minimum=1"); - config.add("role=contributor"); + config.add("contributor=1"); config.add("ignore=duke"); config.add("[checks \"committer\"]"); diff --git a/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersCheck.java b/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersCheck.java index 38a302585..cdf31d09e 100644 --- a/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersCheck.java +++ b/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersCheck.java @@ -29,6 +29,9 @@ import java.io.IOException; import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.HashMap; import java.util.stream.Collectors; import java.util.logging.Logger; @@ -42,20 +45,24 @@ public class ReviewersCheck extends CommitCheck { this.utils = utils; } - private boolean hasRole(Project project, String role, String username, int version) { + private String nextRequiredRole(String role, Map count) { switch (role) { case "lead": - return project.isLead(username, version); + return count.get("reviewer") != 0 ? + "reviewer" : nextRequiredRole("reviewer", count); case "reviewer": - return project.isReviewer(username, version); + return count.get("committer") != 0 ? + "committer" : nextRequiredRole("committer", count); case "committer": - return project.isCommitter(username, version); + return count.get("author") != 0 ? + "author" : nextRequiredRole("author", count); case "author": - return project.isAuthor(username, version); + return count.get("contributor") != 0 ? + "contributor" : nextRequiredRole("contributor", count); case "contributor": - return census.isContributor(username); + return null; default: - throw new IllegalStateException("Unsupported role: " + role); + throw new IllegalArgumentException("Unexpected role: " + role); } } @@ -69,22 +76,19 @@ Iterator check(Commit commit, CommitMessage message, JCheckConfiguration var project = census.project(conf.general().project()); var version = conf.census().version(); var domain = conf.census().domain(); - var role = conf.checks().reviewers().role(); - var required = conf.checks().reviewers().minimum(); + + var numLeadRole = conf.checks().reviewers().lead(); + var numReviewerRole = conf.checks().reviewers().reviewers(); + var numCommitterRole = conf.checks().reviewers().committers(); + var numAuthorRole = conf.checks().reviewers().authors(); + var numContributorRole = conf.checks().reviewers().contributors(); + var ignore = conf.checks().reviewers().ignore(); var reviewers = message.reviewers() .stream() .filter(r -> !ignore.contains(r)) .collect(Collectors.toList()); - var actual = reviewers.stream() - .filter(reviewer -> hasRole(project, role, reviewer, version)) - .count(); - if (actual < required) { - log.finer("issue: too few reviewers found"); - return iterator(new TooFewReviewersIssue(Math.toIntExact(actual), required, metadata)); - } - var invalid = reviewers.stream() .filter(r -> !census.isContributor(r)) .collect(Collectors.toList()); @@ -93,6 +97,55 @@ Iterator check(Commit commit, CommitMessage message, JCheckConfiguration return iterator(new InvalidReviewersIssue(invalid, project, metadata)); } + var requirements = Map.of( + "lead", numLeadRole, + "reviewer", numReviewerRole, + "committer", numCommitterRole, + "author", numAuthorRole, + "contributor", numContributorRole); + + var roles = new HashMap(); + for (var reviewer : reviewers) { + String role = null; + if (project.isLead(reviewer, version)) { + role = "lead"; + } else if (project.isReviewer(reviewer, version)) { + role = "reviewer"; + } else if (project.isCommitter(reviewer, version)) { + role = "committer"; + } else if (project.isAuthor(reviewer, version)) { + role = "author"; + } else if (census.isContributor(reviewer)) { + role = "contributor"; + } else { + throw new IllegalStateException("No role for reviewer: " + reviewer); + } + + roles.put(reviewer, role); + } + + var missing = new HashMap(requirements); + for (var reviewer : reviewers) { + var role = roles.get(reviewer); + if (missing.get(role) == 0) { + var next = nextRequiredRole(role, missing); + if (next != null) { + missing.put(next, missing.get(next) - 1); + } + } else { + missing.put(role, missing.get(role) - 1); + } + } + + for (var role : missing.keySet()) { + int required = requirements.get(role); + int n = missing.get(role); + if (n > 0) { + log.finer("issue: too few reviewers with role " + role + " found"); + return iterator(new TooFewReviewersIssue(required - n, required, role, metadata)); + } + } + var username = commit.author().name(); var email = commit.author().email(); if (email != null && email.endsWith("@" + domain)) { diff --git a/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersConfiguration.java b/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersConfiguration.java index cd9d1e53d..2a5a9df79 100644 --- a/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersConfiguration.java +++ b/jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersConfiguration.java @@ -28,24 +28,42 @@ import java.util.stream.Collectors; public class ReviewersConfiguration { - static final ReviewersConfiguration DEFAULT = new ReviewersConfiguration(1, "reviewer", List.of("duke")); + static final ReviewersConfiguration DEFAULT = new ReviewersConfiguration(0, 1, 0, 0, 0, List.of("duke")); - private final int minimum; - private final String role; + private final int lead; + private final int reviewers; + private final int committers; + private final int authors; + private final int contributors; private final List ignore; - ReviewersConfiguration(int minimum, String role, List ignore) { - this.minimum = minimum; - this.role = role; + ReviewersConfiguration(int lead, int reviewers, int committers, int authors, int contributors, List ignore) { + this.lead = lead; + this.reviewers = reviewers; + this.committers = committers; + this.authors = authors; + this.contributors = contributors; this.ignore = ignore; } - public int minimum() { - return minimum; + public int lead() { + return lead; } - public String role() { - return role; + public int reviewers() { + return reviewers; + } + + public int committers() { + return committers; + } + + public int authors() { + return authors; + } + + public int contributors() { + return contributors; } public List ignore() { @@ -61,9 +79,36 @@ static ReviewersConfiguration parse(Section s) { return DEFAULT; } - var minimum = s.get("minimum", DEFAULT.minimum()); - var role = s.get("role", DEFAULT.role()); + var lead = s.get("lead", DEFAULT.lead()); + var reviewers = s.get("reviewers", DEFAULT.reviewers()); + var committers = s.get("committers", DEFAULT.committers()); + var authors = s.get("authors", DEFAULT.authors()); + var contributors = s.get("contributors", DEFAULT.contributors()); + + if (s.contains("minimum")) { + var minimum = s.get("minimum").asInt(); + if (s.contains("role")) { + var role = s.get("role").asString(); + if (role.equals("lead")) { + lead = minimum; + } else if (role.equals("reviewer")) { + reviewers = minimum; + } else if (role.equals("committer")) { + committers = minimum; + } else if (role.equals("author")) { + authors = minimum; + } else if (role.equals("contributor")) { + contributors = minimum; + } else { + throw new IllegalArgumentException("Unexpected role: " + role); + } + } else { + reviewers = minimum; + } + } + var ignore = s.get("ignore", DEFAULT.ignore()); - return new ReviewersConfiguration(minimum, role, ignore); + + return new ReviewersConfiguration(lead, reviewers, committers, authors, contributors, ignore); } } diff --git a/jcheck/src/main/java/org/openjdk/skara/jcheck/TooFewReviewersIssue.java b/jcheck/src/main/java/org/openjdk/skara/jcheck/TooFewReviewersIssue.java index 7fb188f2c..2eb3a070f 100644 --- a/jcheck/src/main/java/org/openjdk/skara/jcheck/TooFewReviewersIssue.java +++ b/jcheck/src/main/java/org/openjdk/skara/jcheck/TooFewReviewersIssue.java @@ -25,11 +25,13 @@ public class TooFewReviewersIssue extends CommitIssue { private final int numActual; private final int numRequired; + private final String role; - TooFewReviewersIssue(int numActual, int numRequired, CommitIssue.Metadata metadata) { + TooFewReviewersIssue(int numActual, int numRequired, String role, CommitIssue.Metadata metadata) { super(metadata); this.numActual = numActual; this.numRequired = numRequired; + this.role = role; } public int numRequired() { @@ -40,6 +42,10 @@ public int numActual() { return numActual; } + public String role() { + return role; + } + @Override public void accept(IssueVisitor v) { v.visit(this); diff --git a/jcheck/src/test/java/org/openjdk/skara/jcheck/ReviewersCheckTests.java b/jcheck/src/test/java/org/openjdk/skara/jcheck/ReviewersCheckTests.java index 960c33d37..78d069b18 100644 --- a/jcheck/src/test/java/org/openjdk/skara/jcheck/ReviewersCheckTests.java +++ b/jcheck/src/test/java/org/openjdk/skara/jcheck/ReviewersCheckTests.java @@ -57,6 +57,9 @@ class ReviewersCheckTests { " ", " Qux", " ", + " ", + " Contributor", + " ", " ", " Test", " ", @@ -80,8 +83,7 @@ class ReviewersCheckTests { "project = test", "[checks]", "error = reviewers", - "[checks \"reviewers\"]", - "role = reviewer" + "[checks \"reviewers\"]" ); private static Commit commit(List reviewers) { @@ -109,13 +111,27 @@ private static JCheckConfiguration conf() { return conf(1); } - private static JCheckConfiguration conf(int minimum) { - return conf(minimum, List.of()); + private static JCheckConfiguration conf(int reviewers) { + return conf(reviewers, 0, 0); + } + + private static JCheckConfiguration conf(int reviewers, List ignored) { + return conf(reviewers, 0, 0, ignored); + } + + private static JCheckConfiguration conf(int reviewers, int committers) { + return conf(reviewers, committers, 0); } - private static JCheckConfiguration conf(int minimum, List ignored) { + private static JCheckConfiguration conf(int reviewers, int committers, int authors) { + return conf(reviewers, committers, authors, List.of()); + } + + private static JCheckConfiguration conf(int reviewers, int committers, int authors, List ignored) { var lines = new ArrayList(CONFIGURATION); - lines.add("minimum = " + minimum); + lines.add("reviewers = " + reviewers); + lines.add("committers = " + committers); + lines.add("authors = " + authors); lines.add("ignore = " + String.join(", ", ignored)); return JCheckConfiguration.parse(lines); } @@ -155,6 +171,7 @@ void committerAsReviewerShouldFail() throws IOException { var issue = (TooFewReviewersIssue) issues.get(0); assertEquals(0, issue.numActual()); assertEquals(1, issue.numRequired()); + assertEquals("reviewer", issue.role()); assertEquals(commit, issue.commit()); assertEquals(Severity.ERROR, issue.severity()); assertEquals(check, issue.check()); @@ -171,6 +188,7 @@ void authorAsReviewerShouldFail() throws IOException { var issue = (TooFewReviewersIssue) issues.get(0); assertEquals(0, issue.numActual()); assertEquals(1, issue.numRequired()); + assertEquals("reviewer", issue.role()); assertEquals(commit, issue.commit()); assertEquals(Severity.ERROR, issue.severity()); assertEquals(check, issue.check()); @@ -187,6 +205,7 @@ void noReviewersShouldFail() throws IOException { var issue = (TooFewReviewersIssue) issues.get(0); assertEquals(0, issue.numActual()); assertEquals(1, issue.numRequired()); + assertEquals("reviewer", issue.role()); assertEquals(commit, issue.commit()); assertEquals(Severity.ERROR, issue.severity()); assertEquals(check, issue.check()); @@ -203,6 +222,7 @@ void multipleInvalidReviewersShouldFail() throws IOException { var issue = (TooFewReviewersIssue) issues.get(0); assertEquals(0, issue.numActual()); assertEquals(1, issue.numRequired()); + assertEquals("reviewer", issue.role()); assertEquals(commit, issue.commit()); assertEquals(Severity.ERROR, issue.severity()); assertEquals(check, issue.check()); @@ -215,10 +235,9 @@ void uknownReviewersShouldFail() throws IOException { var issues = toList(check.check(commit, message(commit), conf(1))); assertEquals(1, issues.size()); - assertTrue(issues.get(0) instanceof TooFewReviewersIssue); - var issue = (TooFewReviewersIssue) issues.get(0); - assertEquals(0, issue.numActual()); - assertEquals(1, issue.numRequired()); + assertTrue(issues.get(0) instanceof InvalidReviewersIssue); + var issue = (InvalidReviewersIssue) issues.get(0); + assertEquals(List.of("unknown", "user"), issue.invalid()); assertEquals(commit, issue.commit()); assertEquals(Severity.ERROR, issue.severity()); assertEquals(check, issue.check()); @@ -281,4 +300,87 @@ void ignoredReviewersShouldBeExcluded() throws IOException { assertEquals(1, issues.size()); assertTrue(issues.get(0) instanceof TooFewReviewersIssue); } + + @Test + void requiringCommitterAndReviwerShouldPass() throws IOException { + var commit = commit(List.of("bar", "baz")); + var check = new ReviewersCheck(census(), utils); + var issues = toList(check.check(commit, message(commit), conf(1, 1))); + + assertEquals(0, issues.size()); + } + + @Test + void missingRoleShouldFail() throws IOException { + var commit = commit(List.of("bar", "qux")); + var check = new ReviewersCheck(census(), utils); + var issues = toList(check.check(commit, message(commit), conf(1, 1))); + + assertEquals(1, issues.size()); + assertTrue(issues.get(0) instanceof TooFewReviewersIssue); + var issue = (TooFewReviewersIssue) issues.get(0); + assertEquals(0, issue.numActual()); + assertEquals(1, issue.numRequired()); + assertEquals("committer", issue.role()); + assertEquals(commit, issue.commit()); + assertEquals(Severity.ERROR, issue.severity()); + assertEquals(check, issue.check()); + } + + @Test + void relaxedRoleShouldPass() throws IOException { + var commit = commit(List.of("bar", "qux")); + var check = new ReviewersCheck(census(), utils); + var issues = toList(check.check(commit, message(commit), conf(0, 1, 1))); + + assertEquals(0, issues.size()); + } + + @Test + void relaxedRoleAndMissingRoleShouldFail() throws IOException { + var commit = commit(List.of("bar", "contributor")); + var check = new ReviewersCheck(census(), utils); + var issues = toList(check.check(commit, message(commit), conf(0, 1, 1))); + + assertEquals(1, issues.size()); + assertTrue(issues.get(0) instanceof TooFewReviewersIssue); + var issue = (TooFewReviewersIssue) issues.get(0); + assertEquals(0, issue.numActual()); + assertEquals(1, issue.numRequired()); + assertEquals("author", issue.role()); + assertEquals(commit, issue.commit()); + assertEquals(Severity.ERROR, issue.severity()); + assertEquals(check, issue.check()); + } + + @Test + void legacyConfigurationShouldWork() throws IOException { + var commit = commit(List.of("bar")); + var check = new ReviewersCheck(census(), utils); + var legacyConf = new ArrayList<>(CONFIGURATION); + legacyConf.add("minimum = 1"); + legacyConf.add("role = reviewer"); + var issues = toList(check.check(commit, message(commit), JCheckConfiguration.parse(legacyConf))); + assertEquals(0, issues.size()); + } + + @Test + void legacyConfigurationShouldAcceptRole() throws IOException { + var commit = commit(List.of("baz")); + var check = new ReviewersCheck(census(), utils); + var legacyConf = new ArrayList<>(CONFIGURATION); + legacyConf.add("minimum = 1"); + legacyConf.add("role = reviewer"); + var issues = toList(check.check(commit, message(commit), JCheckConfiguration.parse(legacyConf))); + + assertEquals(1, issues.size()); + assertTrue(issues.get(0) instanceof TooFewReviewersIssue); + var issue = (TooFewReviewersIssue) issues.get(0); + assertEquals(0, issue.numActual()); + assertEquals(1, issue.numRequired()); + assertEquals("reviewer", issue.role()); + assertEquals(commit, issue.commit()); + assertEquals(Severity.ERROR, issue.severity()); + assertEquals(check, issue.check()); + } }