Skip to content

Commit

Permalink
SKARA-2307
Browse files Browse the repository at this point in the history
  • Loading branch information
@zhaosongzs
zhaosongzs committed Jun 24, 2024
1 parent 2e66296 commit c50dc78
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 18 deletions.
21 changes: 11 additions & 10 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
class PullRequestCheckIssueVisitor implements IssueVisitor {
private final List<CheckAnnotation> annotations = new LinkedList<>();
private final Set<Check> enabledChecks;
private final Map<Class<? extends Check>, String> errorFailedChecks = new HashMap<>();
private final Map<Class<? extends Check>, String> warningFailedChecks = new HashMap<>();
private final Map<Class<? extends Check>, List<String>> errorFailedChecks = new HashMap<>();
private final Map<Class<? extends Check>, List<String>> warningFailedChecks = new HashMap<>();
private boolean readyForReview;

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
Expand All @@ -62,26 +62,27 @@ private void setNotReadyForReviewOnError(Severity severity) {

private void addMessage(Check check, String message, Severity severity) {
if (severity == Severity.ERROR) {
errorFailedChecks.put(check.getClass(), message);
errorFailedChecks.computeIfAbsent(check.getClass(), k -> new ArrayList<>()).add(message);
} else if (severity == Severity.WARNING) {
warningFailedChecks.put(check.getClass(), message);
warningFailedChecks.computeIfAbsent(check.getClass(), k -> new ArrayList<>()).add(message);
}
}

List<String> errorFailedChecksMessages() {
return new ArrayList<>(errorFailedChecks.values());
return errorFailedChecks.values().stream().flatMap(List::stream).toList();
}

List<String> warningFailedChecksMessages() {
return new ArrayList<>(warningFailedChecks.values());
return warningFailedChecks.values().stream().flatMap(List::stream).toList();
}

List<String> hiddenErrorMessages() {
return errorFailedChecks.entrySet().stream()
.filter(entry -> !displayedChecks.contains(entry.getKey()))
.map(Map.Entry::getValue)
.sorted()
.collect(Collectors.toList());
.filter(entry -> !displayedChecks.contains(entry.getKey()))
.map(Map.Entry::getValue)
.flatMap(List::stream)
.sorted()
.collect(Collectors.toList());
}

/**
Expand Down
23 changes: 15 additions & 8 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -622,13 +622,16 @@ void executableFile(TestInfo testInfo) throws IOException {
localRepo.push(masterHash, author.authenticatedUrl(), "master", true);

// Make a change with a corresponding PR
Files.writeString(tempFolder.path().resolve("executable.exe"), "Executable file contents", StandardCharsets.UTF_8);
Files.setPosixFilePermissions(tempFolder.path().resolve("executable.exe"), Set.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable.exe"));
Files.writeString(tempFolder.path().resolve("executable1.exe"), "Executable file contents", StandardCharsets.UTF_8);
Files.setPosixFilePermissions(tempFolder.path().resolve("executable1.exe"), Set.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable1.exe"));
Files.writeString(tempFolder.path().resolve("executable2.exe"), "Executable file contents", StandardCharsets.UTF_8);
Files.setPosixFilePermissions(tempFolder.path().resolve("executable2.exe"), Set.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable2.exe"));
var editHash = localRepo.commit("Make it executable", "duke", "duke@openjdk.org");
localRepo.push(editHash, author.authenticatedUrl(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "Another PR");
pr.setBody("This should now be ready");
pr.setBody("This should not be ready");

// Check the status
TestBotRunner.runPeriodicItems(checkBot);
Expand All @@ -638,19 +641,23 @@ void executableFile(TestInfo testInfo) throws IOException {
assertEquals(1, checks.size());
var check = checks.get("jcheck");
assertEquals(CheckStatus.FAILURE, check.status());
assertTrue(check.summary().orElseThrow().contains("Executable files are not allowed (file: executable.exe)"));
assertTrue(check.summary().orElseThrow().contains("Executable files are not allowed (file: executable1.exe)"));
assertTrue(check.summary().orElseThrow().contains("Executable files are not allowed (file: executable2.exe)"));

// Additional errors should be displayed in the body
assertTrue(pr.store().body().contains("## Error"));
assertTrue(pr.store().body().contains("Executable files are not allowed (file: executable.exe)"));
assertTrue(pr.store().body().contains("Executable files are not allowed (file: executable1.exe)"));
assertTrue(pr.store().body().contains("Executable files are not allowed (file: executable2.exe)"));

// The PR should not yet be ready for review
assertFalse(pr.store().labelNames().contains("rfr"));
assertFalse(pr.store().labelNames().contains("ready"));

// Drop that error
Files.setPosixFilePermissions(tempFolder.path().resolve("executable.exe"), Set.of(PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable.exe"));
Files.setPosixFilePermissions(tempFolder.path().resolve("executable1.exe"), Set.of(PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable1.exe"));
Files.setPosixFilePermissions(tempFolder.path().resolve("executable2.exe"), Set.of(PosixFilePermission.OWNER_READ));
localRepo.add(Path.of("executable2.exe"));
var updatedHash = localRepo.commit("Make it unexecutable", "duke", "duke@openjdk.org");
localRepo.push(updatedHash, author.authenticatedUrl(), "edit");
TestBotRunner.runPeriodicItems(checkBot);
Expand Down

0 comments on commit c50dc78

Please sign in to comment.