Skip to content

Commit

Permalink
2088: Skara /reviewers command is no longer effective
Browse files Browse the repository at this point in the history
Reviewed-by: erikj, zsong
  • Loading branch information
edvbld committed Nov 2, 2023
1 parent 0bbb956 commit 410d4e5
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ static List<String> get(ReadOnlyRepository repository, Hash hash, HostUser botUs
var updatedLimits = ReviewersTracker.updatedRoleLimits(currentConfiguration, additionalReviewers.get().number(), additionalReviewers.get().role());
ret.add("[checks \"reviewers\"]");
updatedLimits.forEach((role, count) -> ret.add(role + "=" + count));
ret.add("minimum=disable");
if (reviewMerge) {
ret.add("merge=check");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.test.*;

import java.io.*;
import java.util.*;

import org.junit.jupiter.api.*;
import static org.junit.jupiter.api.Assertions.*;

class AdditionalConfigurationTests {
@Test
void minimumShouldBeDisabled(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var bot = credentials.getHostedRepository();

var censusBuilder = credentials.getCensusBuilder()
.addReviewer(integrator.forge().currentUser().id())
.addCommitter(author.forge().currentUser().id());
var prBot = PullRequestBot.newBuilder().repo(bot).censusRepo(censusBuilder.build()).build();

// Populate the projects repository
var localRepoFolder = tempFolder.path().resolve("localrepo");
var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
assertFalse(CheckableRepository.hasBeenEdited(localRepo));
localRepo.push(masterHash, author.authenticatedUrl(), "master", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.authenticatedUrl(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "123: This is a pull request");

var reviewerPr = (TestPullRequest)integrator.pullRequest(pr.id());

// Require two reviewers
reviewerPr.addComment("/reviewers 2");
TestBotRunner.runPeriodicItems(prBot);

var additional = AdditionalConfiguration.get(localRepo, masterHash, bot.forge().currentUser(),
pr.comments(), false);
var expected = List.of(
"[checks \"reviewers\"]",
"lead=0",
"reviewers=1",
"committers=0",
"authors=1",
"contributors=0",
"minimum=disable"
);
assertEquals(expected, additional);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -130,31 +130,40 @@ static ReviewersConfiguration parse(Section s) {
var contributors = s.get("contributors", 0);

if (s.contains("minimum")) {
// Reset defaults to 0
lead = 0;
reviewers = 0;
committers = 0;
authors = 0;
contributors = 0;

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;
var isMinimumDisabled = s.get("minimum").asString().trim().toLowerCase().equals("disable");
if (!isMinimumDisabled) {
for (var role : List.of("lead", "reviewers", "committers", "authors", "contributors")) {
if (s.contains(role)) {
throw new IllegalStateException("Cannot combine 'minimum' with '" + role + "'");
}
}

// Reset defaults to 0
lead = 0;
reviewers = 0;
committers = 0;
authors = 0;
contributors = 0;

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 {
throw new IllegalArgumentException("Unexpected role: " + role);
reviewers = minimum;
}
} else {
reviewers = minimum;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -698,4 +698,21 @@ private String constructReviewRequirement(int leadNum, int reviewerNum, int comm
return String.format(hasReview, totalNum, totalNum > 1 ? "s" : "", String.join(", ", requireList));
}

@Test
void minimumCanBeDisabled() {
var conf = new ArrayList<>(CONFIGURATION);
conf.add("reviewers = 1");
conf.add("minimum = disable");
assertEquals(constructReviewRequirement(0, 1, 0, 0, 0),
JCheckConfiguration.parse(conf).checks().reviewers().getReviewRequirements());
}

@Test
void minimumWithAnotherRoleTrows() {
var conf = new ArrayList<>(CONFIGURATION);
conf.add("reviewers = 1");
conf.add("minimum = 1");
assertThrows(IllegalStateException.class, () -> JCheckConfiguration.parse(conf));
}

}

1 comment on commit 410d4e5

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.