From fac771d8f342783b29d030d518269556a68d0e89 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Sat, 27 Jun 2020 21:34:13 +0800 Subject: [PATCH 01/12] Use optional for ChecksDetails attributes --- .../plugins/checks/api/ChecksDetails.java | 103 +++++++-------- .../plugins/checks/api/ChecksStatus.java | 2 +- .../checks/github/GitHubChecksDetails.java | 117 +++++++++++++----- 3 files changed, 128 insertions(+), 94 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index 85d2f47c..31a6369f 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -1,15 +1,11 @@ package io.jenkins.plugins.checks.api; -import java.net.URI; import java.time.LocalDateTime; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; -import org.apache.commons.lang3.StringUtils; - -import edu.umd.cs.findbugs.annotations.Nullable; - import static java.util.Objects.requireNonNull; public class ChecksDetails { @@ -40,8 +36,8 @@ private ChecksDetails(final String name, final ChecksStatus status, final String * * @return the unique name of a check */ - public String getName() { - return name; + public Optional getName() { + return Optional.ofNullable(name); } /** @@ -58,9 +54,8 @@ public ChecksStatus getStatus() { * * @return the url of a site or null */ - @Nullable - public String getDetailsURL() { - return detailsURL; + public Optional getDetailsURL() { + return Optional.ofNullable(detailsURL); } /** @@ -68,8 +63,8 @@ public String getDetailsURL() { * * @return the start time of a check */ - public LocalDateTime getStartedAt() { - return startedAt; + public Optional getStartedAt() { + return Optional.ofNullable(startedAt); } /** @@ -86,8 +81,8 @@ public ChecksConclusion getConclusion() { * * @return the complete time of a check */ - public LocalDateTime getCompletedAt() { - return completedAt; + public Optional getCompletedAt() { + return Optional.ofNullable(completedAt); } /** @@ -95,9 +90,8 @@ public LocalDateTime getCompletedAt() { * * @return An {@link ChecksOutput} of a check or null */ - @Nullable - public ChecksOutput getOutput() { - return output; + public Optional getOutput() { + return Optional.ofNullable(output); } /** @@ -113,8 +107,8 @@ public List getActions() { * Builder for {@link ChecksDetails}. */ public static class ChecksDetailsBuilder { - private final String name; - private final ChecksStatus status; + private String name; + private ChecksStatus status; private String detailsURL; private LocalDateTime startedAt; private ChecksConclusion conclusion; @@ -123,30 +117,43 @@ public static class ChecksDetailsBuilder { private List actions; /** - * Construct a builder with the given name and status. + * Construct a builder for {@link ChecksDetails}. + */ + public ChecksDetailsBuilder() { + this.status = ChecksStatus.NONE; + this.conclusion = ChecksConclusion.NONE; + this.actions = Collections.emptyList(); + } + + /** + * Set the name of the check. * *

- * The name will be the same as the check run's name shown on GitHub UI and GitHub uses this name to - * identify a check run, so make sure this name is unique, e.g. "Coverage". + * Note that for GitHub check runs, the name shown on GitHub UI will be the same as this attribute and + * GitHub uses this attribute to identify a check run, so make sure this name is unique, e.g. "Coverage". *

* * @param name - * the name of the check run - * @param status - * the status which the check run will be set - * - * @throws IllegalArgumentException if the name is blank or the status is null + * the check's name + * @return this builder + * @throws NullPointerException if the {@code name} is null */ - public ChecksDetailsBuilder(final String name, final ChecksStatus status) { - if (StringUtils.isBlank(name)) { - throw new IllegalArgumentException("check name should not be blank"); - } + public ChecksDetailsBuilder withName(final String name) { + this.name = requireNonNull(name); + return this; + } - this.name = name; + /** + * Set the status of the check. + * + * @param status + * the check's status + * @return this builder + * @throws NullPointerException if the {@code status} is null + */ + public ChecksDetailsBuilder withStatus(final ChecksStatus status) { this.status = requireNonNull(status); - - conclusion = ChecksConclusion.NONE; - actions = Collections.emptyList(); + return this; } /** @@ -164,10 +171,7 @@ public ChecksDetailsBuilder(final String name, final ChecksStatus status) { * @throws IllegalArgumentException if the {@code detailsURL} doesn't use http or https scheme */ public ChecksDetailsBuilder withDetailsURL(final String detailsURL) { - if (!StringUtils.equalsAny(URI.create(detailsURL).getScheme(), "http", "https")) { - throw new IllegalArgumentException("details URL must use http or https scheme: " + detailsURL); - } - this.detailsURL = detailsURL; + this.detailsURL = requireNonNull(detailsURL); return this; } @@ -204,9 +208,6 @@ public ChecksDetailsBuilder withStartedAt(final LocalDateTime startedAt) { * @throws IllegalArgumentException if the {@code status} is not {@link ChecksStatus#COMPLETED} */ public ChecksDetailsBuilder withConclusion(final ChecksConclusion conclusion) { - if (status != ChecksStatus.COMPLETED) { - throw new IllegalArgumentException("status must be completed when setting conclusion"); - } this.conclusion = requireNonNull(conclusion); return this; } @@ -267,24 +268,6 @@ public ChecksDetailsBuilder withActions(final List actions) { * @throws IllegalArgumentException if {@code conclusion} is null when {@code status} is {@code completed} */ public ChecksDetails build() { - if (conclusion == ChecksConclusion.NONE) { - if (startedAt == null) { - startedAt = LocalDateTime.now(); - } - - if (status == ChecksStatus.COMPLETED) { - throw new IllegalArgumentException("conclusion must be set when status is completed"); - } - - if (completedAt != null) { - throw new IllegalArgumentException("conclusion must be set when completed at is provided"); - } - } else { - if (completedAt == null) { - completedAt = LocalDateTime.now(); - } - } - return new ChecksDetails(name, status, detailsURL, startedAt, conclusion, completedAt, output, actions); } } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksStatus.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksStatus.java index 7d21d197..cf80924d 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksStatus.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksStatus.java @@ -4,5 +4,5 @@ * Status for a specific check. */ public enum ChecksStatus { - QUEUED, IN_PROGRESS, COMPLETED + NONE, QUEUED, IN_PROGRESS, COMPLETED } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index ed41386e..27718040 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -1,12 +1,14 @@ package io.jenkins.plugins.checks.github; +import java.net.URI; import java.time.LocalDateTime; +import java.time.ZoneOffset; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; -import edu.umd.cs.findbugs.annotations.CheckForNull; +import org.apache.commons.lang3.StringUtils; -import org.kohsuke.github.GHCheckRun; import org.kohsuke.github.GHCheckRun.AnnotationLevel; import org.kohsuke.github.GHCheckRun.Conclusion; import org.kohsuke.github.GHCheckRun.Status; @@ -38,6 +40,9 @@ class GitHubChecksDetails { * @param details the details of a generic check run */ public GitHubChecksDetails(final ChecksDetails details) { + if (details.getStatus() == ChecksStatus.COMPLETED && details.getConclusion() == ChecksConclusion.NONE) { + throw new IllegalArgumentException("The conclusion is null when status is completed."); + } this.details = details; } @@ -47,11 +52,13 @@ public GitHubChecksDetails(final ChecksDetails details) { * @return the name of the check */ public String getName() { - return details.getName(); + return details.getName() + .filter(name -> !StringUtils.isBlank(name)) + .orElseThrow(() -> new IllegalArgumentException("The check name is blank.")); } /** - * Returns the {@link GHCheckRun.Status} of a GitHub check run. + * Returns the {@link Status} of a GitHub check run. * * * @return the status of a check run @@ -59,10 +66,16 @@ public String getName() { */ public Status getStatus() { switch (details.getStatus()) { - case QUEUED: return Status.QUEUED; - case IN_PROGRESS: return Status.IN_PROGRESS; - case COMPLETED: return Status.COMPLETED; - default: throw new IllegalArgumentException("unsupported checks conclusion: " + details.getStatus()); + case QUEUED: + return Status.QUEUED; + case IN_PROGRESS: + return Status.IN_PROGRESS; + case COMPLETED: + return Status.COMPLETED; + case NONE: + throw new IllegalArgumentException("Status is null."); + default: + throw new IllegalArgumentException("Unsupported checks status: " + details.getStatus()); } } @@ -71,7 +84,13 @@ public Status getStatus() { * * @return an URL of the site */ - public String getDetailsURL() { + public Optional getDetailsURL() { + details.getDetailsURL() + .ifPresent(url -> { + if (!StringUtils.equalsAny(URI.create(url).getScheme(), "http", "https")) { + throw new IllegalArgumentException("The details url is not http or https scheme: " + url); + } + }); return details.getDetailsURL(); } @@ -81,25 +100,35 @@ public String getDetailsURL() { * @return the start time of a check */ public LocalDateTime getStartedAt() { - return details.getStartedAt(); + return details.getStartedAt() + .orElse(LocalDateTime.now(ZoneOffset.UTC)); } /** - * Returns the {@link GHCheckRun.Conclusion} of a completed GitHub check run. + * Returns the {@link Conclusion} of a completed GitHub check run. * * @return the conclusion of a completed check run * @throws IllegalArgumentException if the conclusion of the {@code details} is not one of {@link ChecksConclusion} */ - public Conclusion getConclusion() { + public Optional getConclusion() { switch (details.getConclusion()) { - case SKIPPED: return Conclusion.CANCELLED; // TODO: Open a PR to add SKIPPED in Conclusion - case TIME_OUT: return Conclusion.TIMED_OUT; - case CANCELED: return Conclusion.CANCELLED; - case FAILURE: return Conclusion.FAILURE; - case NEUTRAL: return Conclusion.NEUTRAL; - case SUCCESS: return Conclusion.SUCCESS; - case ACTION_REQUIRED: return Conclusion.ACTION_REQUIRED; - default: throw new IllegalArgumentException("unsupported checks conclusion: " + details.getConclusion()); + case SKIPPED: // TODO: Open a PR to add SKIPPED in Conclusion + case CANCELED: + return Optional.of(Conclusion.CANCELLED); + case TIME_OUT: + return Optional.of(Conclusion.TIMED_OUT); + case FAILURE: + return Optional.of(Conclusion.FAILURE); + case NEUTRAL: + return Optional.of(Conclusion.NEUTRAL); + case SUCCESS: + return Optional.of(Conclusion.SUCCESS); + case ACTION_REQUIRED: + return Optional.of(Conclusion.ACTION_REQUIRED); + case NONE: + return Optional.empty(); + default: + throw new IllegalArgumentException("Unsupported checks conclusion: " + details.getConclusion()); } } @@ -109,27 +138,26 @@ public Conclusion getConclusion() { * @return the completed time of a check */ public LocalDateTime getCompletedAt() { - return details.getCompletedAt(); + return details.getCompletedAt() + .orElse(LocalDateTime.now(ZoneOffset.UTC)); } /** - * Returns the {@link GHCheckRunBuilder.Output} of a GitHub check run. + * Returns the {@link Output} of a GitHub check run. * * @return the output of a check run or null */ - @CheckForNull - public Output getOutput() { - ChecksOutput checksOutput = details.getOutput(); - if (checksOutput == null) { - return null; + public Optional getOutput() { + Output output = null; + if (details.getOutput().isPresent()) { + ChecksOutput checksOutput = details.getOutput().get(); + output = new Output(checksOutput.getTitle(), checksOutput.getSummary()) + .withText(checksOutput.getText()); + checksOutput.getChecksAnnotations().stream().map(this::getAnnotation).forEach(output::add); + checksOutput.getChecksImages().stream().map(this::getImage).forEach(output::add); } - Output output = new Output(checksOutput.getTitle(), - checksOutput.getSummary()); - output.withText(checksOutput.getText()); - checksOutput.getChecksAnnotations().stream().map(this::getAnnotation).forEach(output::add); - checksOutput.getChecksImages().stream().map(this::getImage).forEach(output::add); - return output; + return Optional.ofNullable(output); } public List getActions() { @@ -139,6 +167,29 @@ public List getActions() { } private Action getAction(final ChecksAction checksAction) { + if (details.getActions().size() > 3) { + throw new IllegalArgumentException(String.format( + "A maximum of three actions are supported, but %d are provided.", + details.getActions().size())); + } + + for (ChecksAction action : details.getActions()) { + if (action.getLabel().length() > 20) { + throw new IllegalArgumentException("The action's label exceeds the maximum 20 characters: " + + action.getLabel()); + } + + if (action.getDescription().length() > 40) { + throw new IllegalArgumentException("The action's description exceeds the maximum 40 characters: " + + action.getDescription()); + } + + if (action.getIdentifier().length() > 20) { + throw new IllegalArgumentException("The action's identifier exceeds the maximum 20 characters: " + + action.getIdentifier()); + } + } + return new Action(checksAction.getLabel(), checksAction.getDescription(), checksAction.getIdentifier()); } From 4db4712590eb3a40c10666829f6c975b611c8a3f Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Sat, 27 Jun 2020 21:57:24 +0800 Subject: [PATCH 02/12] Refactor GitHubChecksPublisher to adapt Optional usage --- .../io/jenkins/plugins/checks/api/ChecksDetails.java | 2 +- .../plugins/checks/github/GitHubChecksPublisher.java | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index 31a6369f..39383385 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -52,7 +52,7 @@ public ChecksStatus getStatus() { /** * Returns the url of a site with full details of a check. * - * @return the url of a site or null + * @return the url of a site */ public Optional getDetailsURL() { return Optional.ofNullable(detailsURL); diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java index 21cfd63b..8d680278 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java @@ -56,15 +56,12 @@ GHCheckRunBuilder createBuilder(final GitHub gitHub, final GitHubChecksDetails d GHCheckRunBuilder builder = gitHub.getRepository(context.getRepository()) .createCheckRun(details.getName(), context.getHeadSha()) .withStatus(details.getStatus()) - .withDetailsURL(StringUtils.defaultIfBlank(details.getDetailsURL(), context.getURL())) - .withConclusion(details.getConclusion()) + .withDetailsURL(details.getDetailsURL().isPresent() ? details.getDetailsURL().get() : context.getURL()) + .withConclusion(details.getConclusion().isPresent() ? details.getConclusion().get() : null) .withStartedAt(Date.from(details.getStartedAt().atZone(ZoneOffset.UTC).toInstant())) .withCompletedAt(Date.from(details.getCompletedAt().atZone(ZoneOffset.UTC).toInstant())); - if (details.getOutput() != null) { - builder.add(details.getOutput()); - } - + details.getOutput().ifPresent(builder::add); details.getActions().forEach(builder::add); return builder; From 77f3fecd55368a8ab805893f7f150e73946a79b0 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 14:18:54 +0800 Subject: [PATCH 03/12] Migrated the whole API to use Optional for hanlding null values --- .../jenkins/plugins/checks/JobListener.java | 14 +- .../plugins/checks/api/ChecksAnnotation.java | 172 +++++++++++------- .../plugins/checks/api/ChecksDetails.java | 1 - .../plugins/checks/api/ChecksImage.java | 9 +- .../plugins/checks/api/ChecksOutput.java | 103 +++++++---- .../checks/github/GitHubChecksDetails.java | 71 ++++---- .../checks/github/GitHubContextResolver.java | 2 - .../checks/api/ChecksAnnotationTest.java | 87 ++++----- .../plugins/checks/api/ChecksDetailsTest.java | 87 +++------ .../plugins/checks/api/ChecksImageTest.java | 14 +- .../plugins/checks/api/ChecksOutputTest.java | 35 ++-- .../github/GitHubCheckRunPublishITest.java | 22 ++- .../github/GitHubChecksDetailsTest.java | 10 +- 13 files changed, 353 insertions(+), 274 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/JobListener.java b/src/main/java/io/jenkins/plugins/checks/JobListener.java index 5c6c9c66..4322a630 100644 --- a/src/main/java/io/jenkins/plugins/checks/JobListener.java +++ b/src/main/java/io/jenkins/plugins/checks/JobListener.java @@ -26,7 +26,10 @@ public class JobListener extends RunListener> { @Override public void onInitialize(final Run run) { ChecksPublisher publisher = ChecksPublisherFactory.fromRun(run); - publisher.publish(new ChecksDetailsBuilder(CHECKS_NAME, ChecksStatus.QUEUED).build()); + publisher.publish(new ChecksDetailsBuilder() + .withName(CHECKS_NAME) + .withStatus(ChecksStatus.QUEUED) + .build()); } /** @@ -37,7 +40,10 @@ public void onInitialize(final Run run) { @Override public void onStarted(final Run run, final TaskListener listener) { ChecksPublisher publisher = ChecksPublisherFactory.fromRun(run); - publisher.publish(new ChecksDetailsBuilder(CHECKS_NAME, ChecksStatus.IN_PROGRESS).build()); + publisher.publish(new ChecksDetailsBuilder() + .withName(CHECKS_NAME) + .withStatus(ChecksStatus.IN_PROGRESS) + .build()); } /** @@ -49,7 +55,9 @@ public void onStarted(final Run run, final TaskListener listener) { public void onCompleted(final Run run, @NonNull final TaskListener listener) { ChecksPublisher publisher = ChecksPublisherFactory.fromRun(run); // TODO: extract result from run - publisher.publish(new ChecksDetailsBuilder(CHECKS_NAME, ChecksStatus.COMPLETED) + publisher.publish(new ChecksDetailsBuilder() + .withName(CHECKS_NAME) + .withStatus(ChecksStatus.COMPLETED) .withConclusion(ChecksConclusion.SUCCESS) .build()); } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksAnnotation.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksAnnotation.java index e2c4a853..138ded0d 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksAnnotation.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksAnnotation.java @@ -1,5 +1,7 @@ package io.jenkins.plugins.checks.api; +import java.util.Optional; + import static java.util.Objects.*; /** @@ -7,8 +9,8 @@ */ public class ChecksAnnotation { private final String path; - private final int startLine; - private final int endLine; + private final Integer startLine; + private final Integer endLine; private final ChecksAnnotationLevel annotationLevel; private final String message; private final Integer startColumn; @@ -17,7 +19,7 @@ public class ChecksAnnotation { private final String rawDetails; private ChecksAnnotation(final String path, - final int startLine, final int endLine, + final Integer startLine, final Integer endLine, final ChecksAnnotationLevel annotationLevel, final String message, final Integer startColumn, final Integer endColumn, @@ -41,47 +43,50 @@ private ChecksAnnotation(final String path, * the source */ public ChecksAnnotation(final ChecksAnnotation that) { - this(that.getPath(), that.getStartLine(), that.getEndLine(), that.getAnnotationLevel(), that.getMessage(), - that.getStartColumn(), that.getEndColumn(), that.getTitle(), that.getRawDetails()); + this(that.getPath().orElse(null), that.getStartLine().orElse(null), that.getEndLine().orElse(null), + that.getAnnotationLevel(), that.getMessage().orElse(null), that.getStartColumn().orElse(null), + that.getEndColumn().orElse(null), that.getTitle().orElse(null), + that.getRawDetails().orElse(null)); } - public String getPath() { - return path; + public Optional getPath() { + return Optional.ofNullable(path); } - public int getStartLine() { - return startLine; + public Optional getStartLine() { + return Optional.ofNullable(startLine); } - public int getEndLine() { - return endLine; + public Optional getEndLine() { + return Optional.ofNullable(endLine); } public ChecksAnnotationLevel getAnnotationLevel() { return annotationLevel; } - public String getMessage() { - return message; + public Optional getMessage() { + return Optional.ofNullable(message); } - public Integer getStartColumn() { - return startColumn; + public Optional getStartColumn() { + return Optional.ofNullable(startColumn); } - public Integer getEndColumn() { - return endColumn; + public Optional getEndColumn() { + return Optional.ofNullable(endColumn); } - public String getTitle() { - return title; + public Optional getTitle() { + return Optional.ofNullable(title); } - public String getRawDetails() { - return rawDetails; + public Optional getRawDetails() { + return Optional.ofNullable(rawDetails); } public enum ChecksAnnotationLevel { + NONE, NOTICE, WARNING, FAILURE @@ -91,76 +96,107 @@ public enum ChecksAnnotationLevel { * Builder for {@link ChecksAnnotation}. */ public static class ChecksAnnotationBuilder { - private final String path; - private final int startLine; - private final int endLine; - private final ChecksAnnotationLevel annotationLevel; - private final String message; + private String path; + private Integer startLine; + private Integer endLine; + private ChecksAnnotationLevel annotationLevel; + private String message; private Integer startColumn; private Integer endColumn; private String title; private String rawDetails; /** - * Constructs a builder with required parameters for a {@link ChecksAnnotation}. + * Constructs a builder for {@link ChecksAnnotation}. + */ + public ChecksAnnotationBuilder() { + this.annotationLevel = ChecksAnnotationLevel.NONE; + } + + /** + * Sets the path of the file to annotate. * * @param path - * the relative path of the file to annotation, e.g. assets/css/main.css + * the relative path of the file to annotation, + * e.g. src/main/java/io/jenkins/plugins/checks/api/ChecksAnnotation.java + * @return this builder + */ + public ChecksAnnotationBuilder withPath(final String path) { + this.path = requireNonNull(path); + return this; + } + + /** + * Sets the line of the single line annotation. + * + * @param line + * the line of code to annotate + * @return this builder + */ + public ChecksAnnotationBuilder withLine(final int line) { + withStartLine(line); + withEndLine(line); + return this; + } + + /** + * Sets the start line of annotation + * * @param startLine - * the start line of the annotation + * the start line of code to annotate + * @return this builder + */ + public ChecksAnnotationBuilder withStartLine(final Integer startLine) { + this.startLine = requireNonNull(startLine); + return this; + } + + /** + * Sets the end line of annotation. + * * @param endLine - * the end line of the annotation - * @param annotationLevel - * the level of the annotation, can be one of {@link ChecksAnnotationLevel#NOTICE}, - * {@link ChecksAnnotationLevel#WARNING}, {@link ChecksAnnotationLevel#FAILURE} - * @param message - * a short description of the feedback for the annotated code + * the end line of code to annotate + * @return this builder */ - public ChecksAnnotationBuilder(final String path, final int startLine, final int endLine, - final ChecksAnnotationLevel annotationLevel, final String message) { - this.path = requireNonNull(path); - this.startLine = startLine; - this.endLine = endLine; - this.annotationLevel = requireNonNull(annotationLevel); - this.message = requireNonNull(message); + public ChecksAnnotationBuilder withEndLine(final Integer endLine) { + this.endLine = requireNonNull(endLine); + return this; } /** - * Constructs a builder with required parameters for a {@link ChecksAnnotation}. + * Sets the annotation level, one of {@code NOTICE}, {@code WARNING}, or {@code FAILURE}. + * The default is {@code WARNING}. * - *

- * Note that for a GitHub check run annotation, the {@code message} must not exceed 64 KB. - *

+ * @param level + * the annotation level + * @return this builder + */ + public ChecksAnnotationBuilder withAnnotationLevel(final ChecksAnnotationLevel level) { + this.annotationLevel = requireNonNull(level); + return this; + } + + /** + * Sets a short description of the feedback for the annotation. * - * @param path - * the relative path of the file to annotation, e.g. assets/css/main.css - * @param line - * the line of the code to annotate - * @param annotationLevel - * the level of the annotation, can be one of {@link ChecksAnnotationLevel#NOTICE}, - * {@link ChecksAnnotationLevel#WARNING}, {@link ChecksAnnotationLevel#FAILURE} * @param message - * a short description of the feedback for the annotated code + * a short description + * @return this builder */ - public ChecksAnnotationBuilder(final String path, final int line, final ChecksAnnotationLevel annotationLevel, - final String message) { - this(path, line, line, annotationLevel, message); + public ChecksAnnotationBuilder withMessage(final String message) { + this.message = requireNonNull(message); + return this; } /** * Adds start column of the annotation. - * TODO: determine how GitHub behaves when the start and end column are not provided at the same time * * @param startColumn * the start column of the annotation * @return this builder */ - public ChecksAnnotationBuilder withStartColumn(final int startColumn) { - if (startLine != endLine) { - throw new IllegalArgumentException(String.format("startLine and endLine attributes must be the same " - + "when adding column, start line: %d, end line: %d", startLine, endLine)); - } - this.startColumn = startColumn; + public ChecksAnnotationBuilder withStartColumn(final Integer startColumn) { + this.startColumn = requireNonNull(startColumn); return this; } @@ -171,12 +207,8 @@ public ChecksAnnotationBuilder withStartColumn(final int startColumn) { * the end column of the annotation * @return this builder */ - public ChecksAnnotationBuilder withEndColumn(final int endColumn) { - if (startLine != endLine) { - throw new IllegalArgumentException(String.format("startLine and endLine attributes must be the same " - + "when adding column, start line: %d, end line: %d", startLine, endLine)); - } - this.endColumn = endColumn; + public ChecksAnnotationBuilder withEndColumn(final Integer endColumn) { + this.endColumn = requireNonNull(endColumn); return this; } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index 39383385..42d1e784 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -120,7 +120,6 @@ public static class ChecksDetailsBuilder { * Construct a builder for {@link ChecksDetails}. */ public ChecksDetailsBuilder() { - this.status = ChecksStatus.NONE; this.conclusion = ChecksConclusion.NONE; this.actions = Collections.emptyList(); } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java index 67ab56a9..718065b0 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java @@ -1,5 +1,7 @@ package io.jenkins.plugins.checks.api; +import java.util.Optional; + import static java.util.Objects.*; /** @@ -31,7 +33,7 @@ public ChecksImage(final String alt, final String imageUrl) { */ public ChecksImage(final ChecksImage that) { this(that.getAlt(), that.getImageUrl()); - this.caption = that.getCaption(); + this.caption = that.getCaption().orElse(null); } /** @@ -49,7 +51,6 @@ public String getAlt() { * @return the image URL */ public String getImageUrl() { - // TODO: determine if the image URL should http or https scheme return imageUrl; } @@ -58,8 +59,8 @@ public String getImageUrl() { * * @return the short description of the image */ - public String getCaption() { - return caption; + public Optional getCaption() { + return Optional.ofNullable(caption); } /** diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java index b6d725bd..68b2bc81 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java @@ -1,7 +1,9 @@ package io.jenkins.plugins.checks.api; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; import static java.util.Objects.*; @@ -33,19 +35,20 @@ private ChecksOutput(final String title, final String summary, final String text * the source to copy from */ public ChecksOutput(final ChecksOutput that) { - this(that.getTitle(), that.getSummary(), that.getText(), that.getChecksAnnotations(), that.getChecksImages()); + this(that.getTitle().orElse(null), that.getSummary().orElse(null), that.getText().orElse(null), + that.getChecksAnnotations(), that.getChecksImages()); } - public String getTitle() { - return title; + public Optional getTitle() { + return Optional.ofNullable(title); } - public String getSummary() { - return summary; + public Optional getSummary() { + return Optional.ofNullable(summary); } - public String getText() { - return text; + public Optional getText() { + return Optional.ofNullable(text); } public List getChecksAnnotations() { @@ -60,29 +63,47 @@ public List getChecksImages() { * Builder for {@link ChecksOutput}. */ public static class ChecksOutputBuilder { - private final String title; - private final String summary; + private String title; + private String summary; private String text; private List annotations; private List images; /** - * Construct a builder with given title and summary for a {@link ChecksOutput}. + * Construct a builder for a {@link ChecksOutput}. + * + */ + public ChecksOutputBuilder() { + this.annotations = Collections.emptyList(); + this.images = Collections.emptyList(); + } + + /** + * Sets the title of the check run. + * + * @param title + * the title of the check run + * @return this builder + */ + public ChecksOutputBuilder withTitle(final String title) { + this.title = requireNonNull(title); + return this; + } + + /** + * Sets the summary of the check run * *

- * Note that for a GitHub check run, the {@code summary} supports Markdown. + * Note that for the GitHub check runs, the {@code summary} supports Markdown. *

* - * @param title - * the title of a {@link ChecksOutput} * @param summary - * the summary of a {@link ChecksOutput} + * the summary of the check run + * @return this builder */ - public ChecksOutputBuilder(final String title, final String summary) { - this.title = requireNonNull(title); + public ChecksOutputBuilder withSummary(final String summary) { this.summary = requireNonNull(summary); - this.annotations = Collections.emptyList(); - this.images = Collections.emptyList(); + return this; } /** @@ -102,35 +123,54 @@ public ChecksOutputBuilder withText(final String text) { } /** - * Adds the {@link ChecksAnnotation} for a check run. + * Sets the {@link ChecksAnnotation} for a check run. * * @param annotations * the annotations list * @return this builder */ public ChecksOutputBuilder withAnnotations(final List annotations) { - requireNonNull(annotations); - this.annotations = Collections.unmodifiableList( - annotations.stream() + this.annotations = requireNonNull(annotations).stream() .map(ChecksAnnotation::new) - .collect(Collectors.toList()) - ); + .collect(Collectors.toList()); + return this; + } + + /** + * Adds the {@link ChecksAnnotation} + * + * @param annotation + * the annotation + * @return this builder + */ + public ChecksOutputBuilder addAnnotation(final ChecksAnnotation annotation) { + requireNonNull(annotation); + if (this.annotations == Collections.EMPTY_LIST) { + this.annotations = new ArrayList<>(); + } + this.annotations.add(new ChecksAnnotation(annotation)); return this; } /** - * Adds the {@link ChecksImage} for a check run. + * Sets the {@link ChecksImage} for a check run. * @param images * the images list * @return this builder */ public ChecksOutputBuilder withImages(final List images) { - requireNonNull(images); - this.images = Collections.unmodifiableList( - images.stream() + this.images = requireNonNull(images).stream() .map(ChecksImage::new) - .collect(Collectors.toList()) - ); + .collect(Collectors.toList()); + return this; + } + + public ChecksOutputBuilder addImage(final ChecksImage image) { + requireNonNull(image); + if (this.images == Collections.EMPTY_LIST) { + this.images = new ArrayList<>(); + } + this.images.add(new ChecksImage(image)); return this; } @@ -140,7 +180,8 @@ public ChecksOutputBuilder withImages(final List images) { * @return the built {@link ChecksOutput} */ public ChecksOutput build() { - return new ChecksOutput(title, summary, text, annotations, images); + return new ChecksOutput(title, summary, text, + Collections.unmodifiableList(annotations), Collections.unmodifiableList(images)); } } } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 27718040..70526e8d 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -66,14 +66,13 @@ public String getName() { */ public Status getStatus() { switch (details.getStatus()) { + case NONE: case QUEUED: return Status.QUEUED; case IN_PROGRESS: return Status.IN_PROGRESS; case COMPLETED: return Status.COMPLETED; - case NONE: - throw new IllegalArgumentException("Status is null."); default: throw new IllegalArgumentException("Unsupported checks status: " + details.getStatus()); } @@ -151,8 +150,12 @@ public Optional getOutput() { Output output = null; if (details.getOutput().isPresent()) { ChecksOutput checksOutput = details.getOutput().get(); - output = new Output(checksOutput.getTitle(), checksOutput.getSummary()) - .withText(checksOutput.getText()); + output = new Output( + checksOutput.getTitle().orElseThrow( + () -> new IllegalArgumentException("Title of output is required but not provided")), + checksOutput.getSummary().orElseThrow( + () -> new IllegalArgumentException("Summary of output is required but not proviede"))) + .withText(checksOutput.getText().orElse(null)); checksOutput.getChecksAnnotations().stream().map(this::getAnnotation).forEach(output::add); checksOutput.getChecksImages().stream().map(this::getImage).forEach(output::add); } @@ -167,53 +170,43 @@ public List getActions() { } private Action getAction(final ChecksAction checksAction) { - if (details.getActions().size() > 3) { - throw new IllegalArgumentException(String.format( - "A maximum of three actions are supported, but %d are provided.", - details.getActions().size())); - } - - for (ChecksAction action : details.getActions()) { - if (action.getLabel().length() > 20) { - throw new IllegalArgumentException("The action's label exceeds the maximum 20 characters: " - + action.getLabel()); - } - - if (action.getDescription().length() > 40) { - throw new IllegalArgumentException("The action's description exceeds the maximum 40 characters: " - + action.getDescription()); - } - - if (action.getIdentifier().length() > 20) { - throw new IllegalArgumentException("The action's identifier exceeds the maximum 20 characters: " - + action.getIdentifier()); - } - } - return new Action(checksAction.getLabel(), checksAction.getDescription(), checksAction.getIdentifier()); } private Annotation getAnnotation(final ChecksAnnotation checksAnnotation) { - return new Annotation(checksAnnotation.getPath(), - checksAnnotation.getStartLine(), checksAnnotation.getEndLine(), + return new Annotation( + checksAnnotation.getPath() + .orElseThrow(() -> new IllegalArgumentException("Path is required but not provided.")), + checksAnnotation.getStartLine() + .orElseThrow(() -> new IllegalArgumentException("Start line is required but not provided.")), + checksAnnotation.getEndLine(). + orElseThrow(() -> new IllegalArgumentException("End line is required but not provided.")), getAnnotationLevel(checksAnnotation.getAnnotationLevel()), - checksAnnotation.getMessage()) - .withTitle(checksAnnotation.getTitle()) - .withRawDetails(checksAnnotation.getRawDetails()) - .withStartColumn(checksAnnotation.getStartColumn()) - .withEndColumn(checksAnnotation.getEndColumn()); + checksAnnotation.getMessage() + .orElseThrow(() -> new IllegalArgumentException("Message is required but not provided."))) + .withTitle(checksAnnotation.getTitle().orElse(null)) + .withRawDetails(checksAnnotation.getRawDetails().orElse(null)) + .withStartColumn(checksAnnotation.getStartColumn().orElse(null)) + .withEndColumn(checksAnnotation.getEndColumn().orElse(null)); } private Image getImage(final ChecksImage checksImage) { - return new Image(checksImage.getAlt(), checksImage.getImageUrl()).withCaption(checksImage.getCaption()); + return new Image(checksImage.getAlt(), checksImage.getImageUrl()) + .withCaption(checksImage.getCaption().orElse(null)); } private AnnotationLevel getAnnotationLevel(final ChecksAnnotationLevel checksLevel) { switch (checksLevel) { - case NOTICE: return AnnotationLevel.NOTICE; - case FAILURE: return AnnotationLevel.FAILURE; - case WARNING: return AnnotationLevel.WARNING; - default: throw new IllegalArgumentException("unsupported checks annotation level: " + checksLevel); + case NOTICE: + return AnnotationLevel.NOTICE; + case FAILURE: + return AnnotationLevel.FAILURE; + case WARNING: + return AnnotationLevel.WARNING; + case NONE: + throw new IllegalArgumentException("Annotation level is required but not set."); + default: + throw new IllegalArgumentException("Unsupported checks annotation level: " + checksLevel); } } } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubContextResolver.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubContextResolver.java index e9759755..1c6b1270 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubContextResolver.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubContextResolver.java @@ -2,8 +2,6 @@ import com.cloudbees.plugins.credentials.CredentialsProvider; -import edu.umd.cs.findbugs.annotations.NonNull; - import org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials; import org.jenkinsci.plugins.github_branch_source.GitHubSCMSource; import hudson.model.Run; diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java index 54768f05..aea907fb 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java @@ -1,12 +1,13 @@ package io.jenkins.plugins.checks.api; +import java.util.Optional; + import org.junit.jupiter.api.Test; import io.jenkins.plugins.checks.api.ChecksAnnotation.ChecksAnnotationBuilder; import io.jenkins.plugins.checks.api.ChecksAnnotation.ChecksAnnotationLevel; import static io.jenkins.plugins.checks.api.ChecksAnnotationAssert.*; -import static org.assertj.core.api.Assertions.*; class ChecksAnnotationTest { private final static String PATH = @@ -22,72 +23,72 @@ void shouldBuildCorrectlyWithAllFields() { + "https://pmd.github.io/pmd-6.22.0/pmd_rules_java_bestpractices.html#unusedprivatefield\" " + "priority=\"3\">\n" + "Avoid unused private fields such as 'LOGGER'.\n" + ""; - final ChecksAnnotation annotation = new ChecksAnnotationBuilder(PATH, 20, 20, - ChecksAnnotationLevel.NOTICE, MESSAGE) + final ChecksAnnotation annotation = new ChecksAnnotationBuilder() + .withPath(PATH) + .withStartLine(20).withEndLine(20) + .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) + .withMessage(MESSAGE) .withStartColumn(33).withEndColumn(38) .withTitle(title) .withRawDetails(rawDetails) .build(); - assertThat(annotation).hasPath(PATH) - .hasStartLine(20) - .hasEndLine(20) + assertThat(annotation) + .hasPath(Optional.of(PATH)) + .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(MESSAGE) - .hasStartColumn(33) - .hasEndColumn(38) - .hasTitle(title) - .hasRawDetails(rawDetails); + .hasMessage(Optional.of(MESSAGE)) + .hasStartColumn(Optional.of(33)).hasEndColumn(Optional.of(38)) + .hasTitle(Optional.of(title)) + .hasRawDetails(Optional.of(rawDetails)); // test the constructor which takes only one parameter for annotation line - final ChecksAnnotation annotationWithSameLine = new ChecksAnnotationBuilder(PATH, 20, - ChecksAnnotationLevel.NOTICE, MESSAGE) + final ChecksAnnotation annotationWithSameLine = new ChecksAnnotationBuilder() + .withPath(PATH) + .withLine(20) + .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) + .withMessage(MESSAGE) .withStartColumn(33).withEndColumn(38) .withTitle(title) .withRawDetails(rawDetails) .build(); - assertThat(annotationWithSameLine).hasPath(PATH) - .hasStartLine(20) - .hasEndLine(20) + assertThat(annotationWithSameLine) + .hasPath(Optional.of(PATH)) + .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(MESSAGE) - .hasStartColumn(33) - .hasEndColumn(38) - .hasTitle(title) - .hasRawDetails(rawDetails); + .hasMessage(Optional.of(MESSAGE)) + .hasStartColumn(Optional.of(33)) + .hasEndColumn(Optional.of(38)) + .hasTitle(Optional.of(title)) + .hasRawDetails(Optional.of(rawDetails)); // test copy constructor final ChecksAnnotation copied = new ChecksAnnotation(annotation); - assertThat(copied).hasPath(PATH) - .hasStartLine(20) - .hasEndLine(20) + assertThat(copied) + .hasPath(Optional.of(PATH)) + .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(MESSAGE) - .hasStartColumn(33) - .hasEndColumn(38) - .hasTitle(title) - .hasRawDetails(rawDetails); + .hasMessage(Optional.of(MESSAGE)) + .hasStartColumn(Optional.of(33)) + .hasEndColumn(Optional.of(38)) + .hasTitle(Optional.of(title)) + .hasRawDetails(Optional.of(rawDetails)); } @Test void shouldBuildCorrectlyWithOnlyRequiredFields() { - final ChecksAnnotation annotation = new ChecksAnnotationBuilder(PATH, 20, 20, - ChecksAnnotationLevel.NOTICE, MESSAGE) + final ChecksAnnotation annotation = new ChecksAnnotationBuilder() + .withPath(PATH) + .withStartLine(20) + .withEndLine(20) + .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) + .withMessage(MESSAGE) .build(); - assertThat(annotation).hasStartLine(20) - .hasEndLine(20) + assertThat(annotation) + .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(MESSAGE); - } - - @Test - void shouldThrowIllegalArgumentExceptionWhenBuildWithColumnsAndDifferentLineAttributes() { - final ChecksAnnotationBuilder builder = new ChecksAnnotationBuilder(PATH, 20, 21, - ChecksAnnotationLevel.WARNING, MESSAGE); - - assertThatIllegalArgumentException().isThrownBy(() -> builder.withStartColumn(0)); - assertThatIllegalArgumentException().isThrownBy(() -> builder.withEndColumn(1)); + .hasMessage(Optional.of(MESSAGE)); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java index dce8eedb..2b431ccf 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java @@ -4,14 +4,15 @@ import java.time.ZoneOffset; import java.util.Arrays; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.Test; import io.jenkins.plugins.checks.api.ChecksDetails.ChecksDetailsBuilder; import io.jenkins.plugins.checks.api.ChecksOutput.ChecksOutputBuilder; -import static io.jenkins.plugins.checks.api.ChecksOutputAssert.*; import static io.jenkins.plugins.checks.api.ChecksDetailsAssert.*; +import static io.jenkins.plugins.checks.api.ChecksOutputAssert.*; import static org.assertj.core.api.Assertions.*; /** @@ -22,20 +23,25 @@ class ChecksDetailsTest { @Test void shouldBuildCorrectlyWithOnlyRequiredFields() { - ChecksDetails details = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.QUEUED) + ChecksDetails details = new ChecksDetailsBuilder() + .withName(CHECK_NAME) + .withStatus(ChecksStatus.QUEUED) .build(); - assertThat(details).hasName(CHECK_NAME) + assertThat(details).hasName(Optional.of(CHECK_NAME)) .hasStatus(ChecksStatus.QUEUED) - .hasDetailsURL(null) + .hasDetailsURL(Optional.empty()) .hasConclusion(ChecksConclusion.NONE) - .hasOutput(null) + .hasOutput(Optional.empty()) .hasNoActions(); } @Test void shouldBuildCorrectlyWithAllFields() { - ChecksOutput output = new ChecksOutputBuilder("output", "success").build(); + ChecksOutput output = new ChecksOutputBuilder() + .withTitle("output") + .withSummary("success") + .build(); List actions = Arrays.asList( new ChecksAction("action_1", "the first action", "1"), new ChecksAction("action_2", "the second action", "2")); @@ -48,7 +54,9 @@ void shouldBuildCorrectlyWithAllFields() { .atOffset(ZoneOffset.UTC) .toLocalDateTime(); - ChecksDetails details = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.COMPLETED) + ChecksDetails details = new ChecksDetailsBuilder() + .withName(CHECK_NAME) + .withStatus(ChecksStatus.COMPLETED) .withStartedAt(startedAt) .withCompletedAt(completedAt) .withDetailsURL(detailsURL) @@ -57,56 +65,21 @@ void shouldBuildCorrectlyWithAllFields() { .withActions(actions) .build(); - assertThat(details).hasName(CHECK_NAME) + assertThat(details) + .hasName(Optional.of(CHECK_NAME)) .hasStatus(ChecksStatus.COMPLETED) - .hasStartedAt(startedAt) - .hasCompletedAt(completedAt) - .hasDetailsURL(detailsURL); - assertThat(details.getOutput()).hasTitle("output").hasSummary("success"); - assertThat(details.getActions()).usingFieldByFieldElementComparator().containsExactlyElementsOf(actions); - } - - @Test - void shouldThrowExceptionsWhenConstructWithNullParameters() { - assertThatIllegalArgumentException() - .isThrownBy(() -> new ChecksDetailsBuilder("", ChecksStatus.QUEUED)); - assertThatIllegalArgumentException(). - isThrownBy(() -> new ChecksDetailsBuilder(null, ChecksStatus.QUEUED)); - assertThatNullPointerException() - .isThrownBy(() -> new ChecksDetailsBuilder(CHECK_NAME, null)); - assertThatIllegalArgumentException() - .isThrownBy(() -> new ChecksDetailsBuilder(null, null)); - assertThatIllegalArgumentException() - .isThrownBy(() -> new ChecksDetailsBuilder("", null)); - } - - @Test - void shouldThrowExceptionsWhenBuildWithNullParameters() { - ChecksDetailsBuilder builder = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.QUEUED); - - assertThatNullPointerException().isThrownBy(() -> builder.withDetailsURL(null)); - assertThatNullPointerException().isThrownBy(() -> builder.withStartedAt(null)); - assertThatNullPointerException().isThrownBy(() -> builder.withCompletedAt(null)); - assertThatIllegalArgumentException().isThrownBy(() -> builder.withConclusion(null)); - assertThatNullPointerException().isThrownBy(() -> builder.withOutput(null)); - assertThatNullPointerException().isThrownBy(() -> builder.withActions(null)); - } - - @Test - void shouldThrowExceptionsWhenBuildWithUnmatchedStatusAndConclusion() { - ChecksDetailsBuilder builderWithQueued = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.QUEUED); - ChecksDetailsBuilder builderWithCompleted = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.COMPLETED); - - assertThatIllegalArgumentException() - .isThrownBy(() -> builderWithQueued.withConclusion(ChecksConclusion.SUCCESS)); - assertThatIllegalArgumentException().isThrownBy(builderWithCompleted::build); - } - - @Test - void shouldThrowExceptionsWhenBuildWithInvalidDetailsURL() { - ChecksDetailsBuilder builder = new ChecksDetailsBuilder(CHECK_NAME, ChecksStatus.QUEUED); - - assertThatIllegalArgumentException().isThrownBy(() -> builder.withDetailsURL("ci.jenkins.io")); - assertThatIllegalArgumentException().isThrownBy(() -> builder.withDetailsURL("ftp://ci.jenkin.io")); + .hasStartedAt(Optional.of(startedAt)) + .hasCompletedAt(Optional.of(completedAt)) + .hasDetailsURL(Optional.of(detailsURL)); + + assertThat(details.getOutput().isPresent()) + .isTrue(); + assertThat(details.getOutput().get()) + .hasTitle(Optional.of("output")) + .hasSummary(Optional.of("success")); + + assertThat(details.getActions()) + .usingFieldByFieldElementComparator() + .containsExactlyElementsOf(actions); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java index 0fec6236..d0b2018b 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java @@ -1,5 +1,7 @@ package io.jenkins.plugins.checks.api; +import java.util.Optional; + import org.junit.jupiter.api.Test; import static io.jenkins.plugins.checks.api.ChecksImageAssert.*; @@ -12,13 +14,19 @@ void shouldConstructCorrectly() { final String caption = "charts generated by warning-ng-plugin"; final ChecksImage image = new ChecksImage(alt, imageUrl); - assertThat(image).hasAlt(alt).hasImageUrl(imageUrl); + assertThat(image) + .hasAlt(alt) + .hasImageUrl(imageUrl); image.withCaption("charts generated by warning-ng-plugin"); - assertThat(image).hasCaption(caption); + assertThat(image) + .hasCaption(Optional.of(caption)); // test copy constructor final ChecksImage copied = new ChecksImage(image); - assertThat(copied).hasAlt(alt).hasImageUrl(imageUrl).hasCaption(caption); + assertThat(copied) + .hasAlt(alt) + .hasImageUrl(imageUrl) + .hasCaption(Optional.of(caption)); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java index 69813951..bdd94512 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java @@ -2,6 +2,7 @@ import java.util.Arrays; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.Test; @@ -18,10 +19,14 @@ class ChecksOutputTest { @Test void shouldBuildCorrectlyWithOnlyRequiredFields() { - final ChecksOutput checksOutput = new ChecksOutputBuilder(TITLE, SUMMARY).build(); + final ChecksOutput checksOutput = new ChecksOutputBuilder() + .withTitle(TITLE) + .withSummary(SUMMARY) + .build(); - assertThat(checksOutput).hasTitle(TITLE) - .hasSummary(SUMMARY) + assertThat(checksOutput) + .hasTitle(Optional.of(TITLE)) + .hasSummary(Optional.of(SUMMARY)) .hasNoChecksAnnotations() .hasNoChecksImages(); } @@ -30,23 +35,29 @@ void shouldBuildCorrectlyWithOnlyRequiredFields() { void shouldBuildCorrectlyWithAllFields() { final String text = "#Markdown Supported Text"; - ChecksAnnotationBuilder builder = new ChecksAnnotationBuilder("src/main/java/1.java", 0, 10, - ChecksAnnotationLevel.WARNING, "first annotation"); + ChecksAnnotationBuilder builder = new ChecksAnnotationBuilder() + .withPath("src/main/java/1.java") + .withStartLine(0) + .withEndLine(10) + .withAnnotationLevel(ChecksAnnotationLevel.WARNING) + .withMessage("first annotation"); final List annotations = Arrays.asList(builder.withTitle("first").build(), builder.withTitle("second").build()); final List images = Arrays.asList(new ChecksImage("image_1", "https://www.image_1.com"), new ChecksImage("image_2", "https://www.image_2.com")); - final ChecksOutput checksOutput = new ChecksOutputBuilder(TITLE, SUMMARY) + final ChecksOutput checksOutput = new ChecksOutputBuilder() + .withTitle(TITLE) + .withSummary(SUMMARY) .withText(text) .withAnnotations(annotations) .withImages(images) .build(); - assertThat(checksOutput).hasTitle(TITLE) - .hasSummary(SUMMARY) - .hasText(text); + assertThat(checksOutput).hasTitle(Optional.of(TITLE)) + .hasSummary(Optional.of(SUMMARY)) + .hasText(Optional.of(text)); assertThat(checksOutput.getChecksAnnotations()) .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(annotations); assertThat(checksOutput.getChecksImages()) @@ -54,9 +65,9 @@ void shouldBuildCorrectlyWithAllFields() { // test copy constructor final ChecksOutput copied = new ChecksOutput(checksOutput); - assertThat(copied).hasTitle(TITLE) - .hasSummary(SUMMARY) - .hasText(text); + assertThat(copied).hasTitle(Optional.of(TITLE)) + .hasSummary(Optional.of(SUMMARY)) + .hasText(Optional.of(text)); assertThat(copied.getChecksAnnotations()) .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(annotations); assertThat(copied.getChecksImages()) diff --git a/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java b/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java index 3eb8d76b..4baaf194 100644 --- a/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java +++ b/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java @@ -44,23 +44,33 @@ void shouldPublishGitHubCheckRunCorrectly() throws IOException { when(context.getRepository()).thenReturn("XiongKezhi/Sandbox"); when(context.getHeadSha()).thenReturn("18c8e2fd86e7aa3748e279c14a00dc3f0b963e7f"); - ChecksDetails details = new ChecksDetailsBuilder("Jenkins", ChecksStatus.COMPLETED) + ChecksDetails details = new ChecksDetailsBuilder() + .withName("Jenkins") + .withStatus(ChecksStatus.COMPLETED) .withDetailsURL("https://ci.jenkins.io") .withStartedAt(LocalDateTime.ofEpochSecond(999_999, 0, ZoneOffset.UTC)) .withCompletedAt(LocalDateTime.ofEpochSecond(999_999, 0, ZoneOffset.UTC)) .withConclusion(ChecksConclusion.SUCCESS) - .withOutput(new ChecksOutputBuilder("Jenkins Check", "# A Successful Build") + .withOutput(new ChecksOutputBuilder() + .withTitle("Jenkins Check") + .withSummary("# A Successful Build") .withText("## 0 Failures") .withAnnotations(Arrays.asList( - new ChecksAnnotationBuilder("Jenkinsfile", 1, ChecksAnnotationLevel.NOTICE, - "say hello to Jenkins") + new ChecksAnnotationBuilder() + .withPath("Jenkinsfile") + .withLine(1) + .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) + .withMessage("say hello to Jenkins") .withStartColumn(0) .withEndColumn(20) .withTitle("Hello Jenkins") .withRawDetails("a simple echo command") .build(), - new ChecksAnnotationBuilder("Jenkinsfile", 2, ChecksAnnotationLevel.WARNING, - "say hello to GitHub Checks API") + new ChecksAnnotationBuilder() + .withPath("Jenkinsfile") + .withLine(2) + .withAnnotationLevel(ChecksAnnotationLevel.WARNING) + .withMessage("say hello to GitHub Checks API") .withStartColumn(0) .withEndColumn(30) .withTitle("Hello GitHub Checks API") diff --git a/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java b/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java index 6b900488..cb09468a 100644 --- a/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java +++ b/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java @@ -1,5 +1,7 @@ package io.jenkins.plugins.checks.github; +import java.util.Optional; + import org.junit.jupiter.api.Test; import org.kohsuke.github.GHCheckRun.Conclusion; @@ -15,7 +17,9 @@ class GitHubChecksDetailsTest { @Test void shouldReturnAllGitHubObjectsCorrectly() { - ChecksDetails details = new ChecksDetailsBuilder("checks", ChecksStatus.COMPLETED) + ChecksDetails details = new ChecksDetailsBuilder() + .withName("checks") + .withStatus(ChecksStatus.COMPLETED) .withConclusion(ChecksConclusion.SUCCESS) .withDetailsURL("https://ci.jenkins.io") .build(); @@ -23,7 +27,7 @@ void shouldReturnAllGitHubObjectsCorrectly() { GitHubChecksDetails gitHubDetails = new GitHubChecksDetails(details); assertThat(gitHubDetails.getName()).isEqualTo("checks"); assertThat(gitHubDetails.getStatus()).isEqualTo(Status.COMPLETED); - assertThat(gitHubDetails.getConclusion()).isEqualTo(Conclusion.SUCCESS); - assertThat(gitHubDetails.getDetailsURL()).isEqualTo("https://ci.jenkins.io"); + assertThat(gitHubDetails.getConclusion()).isEqualTo(Optional.of(Conclusion.SUCCESS)); + assertThat(gitHubDetails.getDetailsURL()).isEqualTo(Optional.of("https://ci.jenkins.io")); } } From 8f4ef32c7e7a59263d20181af72bcc42ad8a8fd1 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 20:50:15 +0800 Subject: [PATCH 04/12] Adapted tests to Optional usage --- .../plugins/checks/ContextResolver.java | 3 +- .../plugins/checks/api/ChecksDetails.java | 1 + .../plugins/checks/api/ChecksOutput.java | 3 +- .../checks/github/GitHubChecksDetails.java | 38 ++++-- .../checks/github/GitHubChecksPublisher.java | 15 ++- .../plugins/checks/api/ChecksActionTest.java | 8 +- .../checks/api/ChecksAnnotationTest.java | 72 +++++------ .../plugins/checks/api/ChecksDetailsTest.java | 32 ++--- .../plugins/checks/api/ChecksImageTest.java | 32 ++--- .../plugins/checks/api/ChecksOutputTest.java | 112 ++++++++++++------ .../checks/api/NullChecksPublisherTest.java | 20 ++++ 11 files changed, 196 insertions(+), 140 deletions(-) create mode 100644 src/test/java/io/jenkins/plugins/checks/api/NullChecksPublisherTest.java diff --git a/src/main/java/io/jenkins/plugins/checks/ContextResolver.java b/src/main/java/io/jenkins/plugins/checks/ContextResolver.java index 8e6ec018..6f7c87d4 100644 --- a/src/main/java/io/jenkins/plugins/checks/ContextResolver.java +++ b/src/main/java/io/jenkins/plugins/checks/ContextResolver.java @@ -22,8 +22,7 @@ public String resolveHeadSha(final SCMSource source, final Run run) { try { return resolveHeadSha(source.fetch(head, null)); } catch (IOException | InterruptedException e) { - throw new IllegalStateException(String.format("Could not resolve head sha, source: %s, run: %s", - source, run)); + throw new IllegalStateException("Could not resolve head sha, source: " + e); } } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index 42d1e784..c8bba079 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -244,6 +244,7 @@ public ChecksDetailsBuilder withOutput(final ChecksOutput output) { /** * Set the actions of a check. + * TODO: Add method to allow add action one by one * * @param actions * a list of actions diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java index 68b2bc81..78060b5a 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java @@ -181,7 +181,8 @@ public ChecksOutputBuilder addImage(final ChecksImage image) { */ public ChecksOutput build() { return new ChecksOutput(title, summary, text, - Collections.unmodifiableList(annotations), Collections.unmodifiableList(images)); + Collections.unmodifiableList(annotations), + Collections.unmodifiableList(images)); } } } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 70526e8d..05224a98 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -1,8 +1,8 @@ package io.jenkins.plugins.checks.github; import java.net.URI; -import java.time.LocalDateTime; import java.time.ZoneOffset; +import java.util.Date; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -40,9 +40,16 @@ class GitHubChecksDetails { * @param details the details of a generic check run */ public GitHubChecksDetails(final ChecksDetails details) { - if (details.getStatus() == ChecksStatus.COMPLETED && details.getConclusion() == ChecksConclusion.NONE) { - throw new IllegalArgumentException("The conclusion is null when status is completed."); + if (details.getConclusion() == ChecksConclusion.NONE) { + if (details.getStatus() == ChecksStatus.COMPLETED) { + throw new IllegalArgumentException("No conclusion has been set when status is completed."); + } + + if (details.getCompletedAt().isPresent()) { + throw new IllegalArgumentException("No conclusion has been set when \"completedAt\" is provided."); + } } + this.details = details; } @@ -98,9 +105,13 @@ public Optional getDetailsURL() { * * @return the start time of a check */ - public LocalDateTime getStartedAt() { - return details.getStartedAt() - .orElse(LocalDateTime.now(ZoneOffset.UTC)); + public Optional getStartedAt() { + if (details.getStartedAt().isPresent()) { + return Optional.of(Date.from( + details.getStartedAt().get() + .toInstant(ZoneOffset.UTC))); + } + return Optional.empty(); } /** @@ -136,9 +147,13 @@ public Optional getConclusion() { * * @return the completed time of a check */ - public LocalDateTime getCompletedAt() { - return details.getCompletedAt() - .orElse(LocalDateTime.now(ZoneOffset.UTC)); + public Optional getCompletedAt() { + if (details.getCompletedAt().isPresent()) { + return Optional.of(Date.from( + details.getCompletedAt().get() + .toInstant(ZoneOffset.UTC))); + } + return Optional.empty(); } /** @@ -147,10 +162,9 @@ public LocalDateTime getCompletedAt() { * @return the output of a check run or null */ public Optional getOutput() { - Output output = null; if (details.getOutput().isPresent()) { ChecksOutput checksOutput = details.getOutput().get(); - output = new Output( + Output output = new Output( checksOutput.getTitle().orElseThrow( () -> new IllegalArgumentException("Title of output is required but not provided")), checksOutput.getSummary().orElseThrow( @@ -160,7 +174,7 @@ public Optional getOutput() { checksOutput.getChecksImages().stream().map(this::getImage).forEach(output::add); } - return Optional.ofNullable(output); + return Optional.empty(); } public List getActions() { diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java index 8d680278..1b74054b 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java @@ -1,7 +1,7 @@ package io.jenkins.plugins.checks.github; import java.io.IOException; -import java.time.ZoneOffset; +import java.time.Instant; import java.util.Date; import org.apache.commons.lang3.StringUtils; @@ -52,14 +52,17 @@ public void publish(final ChecksDetails details) { @VisibleForTesting GHCheckRunBuilder createBuilder(final GitHub gitHub, final GitHubChecksDetails details, - final GitHubChecksContext context) throws IOException { + final GitHubChecksContext context) throws IOException{ GHCheckRunBuilder builder = gitHub.getRepository(context.getRepository()) .createCheckRun(details.getName(), context.getHeadSha()) .withStatus(details.getStatus()) - .withDetailsURL(details.getDetailsURL().isPresent() ? details.getDetailsURL().get() : context.getURL()) - .withConclusion(details.getConclusion().isPresent() ? details.getConclusion().get() : null) - .withStartedAt(Date.from(details.getStartedAt().atZone(ZoneOffset.UTC).toInstant())) - .withCompletedAt(Date.from(details.getCompletedAt().atZone(ZoneOffset.UTC).toInstant())); + .withDetailsURL(details.getDetailsURL().orElse(context.getURL())) + .withStartedAt(details.getStartedAt().orElse(Date.from(Instant.now()))); + + if (details.getConclusion().isPresent()) { + builder.withConclusion(details.getConclusion().get()) + .withCompletedAt(details.getCompletedAt().orElse(Date.from(Instant.now()))); + } details.getOutput().ifPresent(builder::add); details.getActions().forEach(builder::add); diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java index f55dcf9a..97f1cbfa 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java @@ -9,12 +9,18 @@ class ChecksActionTest { void shouldConstructCorrectly() { final ChecksAction action = new ChecksAction("re-run", "re-run the Jenkins build", "re-run id"); + assertThat(action.getLabel()).isEqualTo("re-run"); assertThat(action.getDescription()).isEqualTo("re-run the Jenkins build"); assertThat(action.getIdentifier()).isEqualTo("re-run id"); + } - // test copy constructor + @Test + void shouldCopyConstructCorrectly() { + final ChecksAction action = + new ChecksAction("re-run", "re-run the Jenkins build", "re-run id"); final ChecksAction copied = new ChecksAction(action); + assertThat(copied.getLabel()).isEqualTo("re-run"); assertThat(copied.getDescription()).isEqualTo("re-run the Jenkins build"); assertThat(copied.getIdentifier()).isEqualTo("re-run id"); diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java index aea907fb..9d2ce3a4 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksAnnotationTest.java @@ -13,24 +13,24 @@ class ChecksAnnotationTest { private final static String PATH = "github-checks-api-plugin/src/main/java/io/jenkins/plugins/checks/CheckGHEventSubscriber.java"; private final static String MESSAGE = "Avoid unused private fields such as 'LOGGER'"; + private final static String TITLE = "UnusedPrivateField"; + private final static String RAW_DETAILS = "\n" + "Avoid unused private fields such as 'LOGGER'.\n" + ""; @Test void shouldBuildCorrectlyWithAllFields() { - final String title = "UnusedPrivateField"; - final String rawDetails = "\n" + "Avoid unused private fields such as 'LOGGER'.\n" + ""; - final ChecksAnnotation annotation = new ChecksAnnotationBuilder() .withPath(PATH) .withStartLine(20).withEndLine(20) .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) .withMessage(MESSAGE) .withStartColumn(33).withEndColumn(38) - .withTitle(title) - .withRawDetails(rawDetails) + .withTitle(TITLE) + .withRawDetails(RAW_DETAILS) .build(); assertThat(annotation) @@ -39,56 +39,42 @@ void shouldBuildCorrectlyWithAllFields() { .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) .hasMessage(Optional.of(MESSAGE)) .hasStartColumn(Optional.of(33)).hasEndColumn(Optional.of(38)) - .hasTitle(Optional.of(title)) - .hasRawDetails(Optional.of(rawDetails)); + .hasTitle(Optional.of(TITLE)) + .hasRawDetails(Optional.of(RAW_DETAILS)); + } - // test the constructor which takes only one parameter for annotation line + @Test + void shouldBuildCorrectlyWithLine() { final ChecksAnnotation annotationWithSameLine = new ChecksAnnotationBuilder() - .withPath(PATH) .withLine(20) - .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .withMessage(MESSAGE) - .withStartColumn(33).withEndColumn(38) - .withTitle(title) - .withRawDetails(rawDetails) .build(); assertThat(annotationWithSameLine) - .hasPath(Optional.of(PATH)) - .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) - .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(Optional.of(MESSAGE)) - .hasStartColumn(Optional.of(33)) - .hasEndColumn(Optional.of(38)) - .hasTitle(Optional.of(title)) - .hasRawDetails(Optional.of(rawDetails)); - - // test copy constructor - final ChecksAnnotation copied = new ChecksAnnotation(annotation); - assertThat(copied) - .hasPath(Optional.of(PATH)) - .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) - .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(Optional.of(MESSAGE)) - .hasStartColumn(Optional.of(33)) - .hasEndColumn(Optional.of(38)) - .hasTitle(Optional.of(title)) - .hasRawDetails(Optional.of(rawDetails)); + .hasStartLine(Optional.of(20)) + .hasEndLine(Optional.of(20)); } @Test - void shouldBuildCorrectlyWithOnlyRequiredFields() { + void shouldCopyConstructCorrectly() { final ChecksAnnotation annotation = new ChecksAnnotationBuilder() .withPath(PATH) - .withStartLine(20) - .withEndLine(20) + .withStartLine(20).withEndLine(20) .withAnnotationLevel(ChecksAnnotationLevel.NOTICE) .withMessage(MESSAGE) + .withStartColumn(33).withEndColumn(38) + .withTitle(TITLE) + .withRawDetails(RAW_DETAILS) .build(); + final ChecksAnnotation copied = new ChecksAnnotation(annotation); - assertThat(annotation) + assertThat(copied) + .hasPath(Optional.of(PATH)) .hasStartLine(Optional.of(20)).hasEndLine(Optional.of(20)) .hasAnnotationLevel(ChecksAnnotationLevel.NOTICE) - .hasMessage(Optional.of(MESSAGE)); + .hasMessage(Optional.of(MESSAGE)) + .hasStartColumn(Optional.of(33)) + .hasEndColumn(Optional.of(38)) + .hasTitle(Optional.of(TITLE)) + .hasRawDetails(Optional.of(RAW_DETAILS)); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java index 2b431ccf..3957b326 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java @@ -12,7 +12,6 @@ import io.jenkins.plugins.checks.api.ChecksOutput.ChecksOutputBuilder; import static io.jenkins.plugins.checks.api.ChecksDetailsAssert.*; -import static io.jenkins.plugins.checks.api.ChecksOutputAssert.*; import static org.assertj.core.api.Assertions.*; /** @@ -21,31 +20,15 @@ class ChecksDetailsTest { private static final String CHECK_NAME = "Jenkins"; - @Test - void shouldBuildCorrectlyWithOnlyRequiredFields() { - ChecksDetails details = new ChecksDetailsBuilder() - .withName(CHECK_NAME) - .withStatus(ChecksStatus.QUEUED) - .build(); - - assertThat(details).hasName(Optional.of(CHECK_NAME)) - .hasStatus(ChecksStatus.QUEUED) - .hasDetailsURL(Optional.empty()) - .hasConclusion(ChecksConclusion.NONE) - .hasOutput(Optional.empty()) - .hasNoActions(); - } - @Test void shouldBuildCorrectlyWithAllFields() { - ChecksOutput output = new ChecksOutputBuilder() + final ChecksOutput output = new ChecksOutputBuilder() .withTitle("output") .withSummary("success") .build(); - List actions = Arrays.asList( + final List actions = Arrays.asList( new ChecksAction("action_1", "the first action", "1"), new ChecksAction("action_2", "the second action", "2")); - final String detailsURL = "https://ci.jenkins.io"; final LocalDateTime startedAt = LocalDateTime.of(2020, 6, 27, 1, 10) .atOffset(ZoneOffset.UTC) @@ -70,13 +53,12 @@ void shouldBuildCorrectlyWithAllFields() { .hasStatus(ChecksStatus.COMPLETED) .hasStartedAt(Optional.of(startedAt)) .hasCompletedAt(Optional.of(completedAt)) - .hasDetailsURL(Optional.of(detailsURL)); + .hasDetailsURL(Optional.of(detailsURL)) + .hasConclusion(ChecksConclusion.SUCCESS); - assertThat(details.getOutput().isPresent()) - .isTrue(); - assertThat(details.getOutput().get()) - .hasTitle(Optional.of("output")) - .hasSummary(Optional.of("success")); + assertThat(details.getOutput()) + .usingFieldByFieldValueComparator() + .contains(output); assertThat(details.getActions()) .usingFieldByFieldElementComparator() diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java index d0b2018b..c5b594c1 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java @@ -7,26 +7,30 @@ import static io.jenkins.plugins.checks.api.ChecksImageAssert.*; class ChecksImageTest { + final String ALT = "warnings-chart"; + final String IMAGE_URL = "https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/"; + final String CAPTION = "charts generated by warning-ng-plugin"; + @Test void shouldConstructCorrectly() { - final String alt = "warnings-chart"; - final String imageUrl = "https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/"; - final String caption = "charts generated by warning-ng-plugin"; - - final ChecksImage image = new ChecksImage(alt, imageUrl); - assertThat(image) - .hasAlt(alt) - .hasImageUrl(imageUrl); + final ChecksImage image = new ChecksImage(ALT, IMAGE_URL) + .withCaption(CAPTION); - image.withCaption("charts generated by warning-ng-plugin"); assertThat(image) - .hasCaption(Optional.of(caption)); + .hasAlt(ALT) + .hasImageUrl(IMAGE_URL) + .hasCaption(Optional.of(CAPTION)); + } - // test copy constructor + @Test + void shouldCopyConstructCorrectly() { + final ChecksImage image = new ChecksImage(ALT, IMAGE_URL) + .withCaption(CAPTION); final ChecksImage copied = new ChecksImage(image); + assertThat(copied) - .hasAlt(alt) - .hasImageUrl(imageUrl) - .hasCaption(Optional.of(caption)); + .hasAlt(ALT) + .hasImageUrl(IMAGE_URL) + .hasCaption(Optional.of(CAPTION)); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java index bdd94512..2aa8420b 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java @@ -1,6 +1,6 @@ package io.jenkins.plugins.checks.api; -import java.util.Arrays; +import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -10,67 +10,107 @@ import io.jenkins.plugins.checks.api.ChecksAnnotation.ChecksAnnotationLevel; import io.jenkins.plugins.checks.api.ChecksOutput.ChecksOutputBuilder; -import static io.jenkins.plugins.checks.api.ChecksOutputAssert.*; -import static org.assertj.core.api.Assertions.*; +import static io.jenkins.plugins.checks.api.ChecksOutputAssert.assertThat; +import static org.assertj.core.api.Assertions.assertThat; class ChecksOutputTest { private final static String TITLE = "Coverage Report"; private final static String SUMMARY = "All code have been covered"; + private final static String TEXT = "# Markdown Supported Text"; @Test - void shouldBuildCorrectlyWithOnlyRequiredFields() { + void shouldBuildCorrectlyWithAllFields() { + final List annotations = createAnnotations(); + final List images = createImages(); final ChecksOutput checksOutput = new ChecksOutputBuilder() .withTitle(TITLE) .withSummary(SUMMARY) + .withText(TEXT) + .withAnnotations(annotations.subList(0, 1)) + .addAnnotation(annotations.get(1)) + .withImages(images.subList(0, 1)) + .addImage(images.get(1)) .build(); assertThat(checksOutput) .hasTitle(Optional.of(TITLE)) .hasSummary(Optional.of(SUMMARY)) - .hasNoChecksAnnotations() - .hasNoChecksImages(); + .hasText(Optional.of(TEXT)); + assertThat(checksOutput.getChecksAnnotations()) + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(annotations); + assertThat(checksOutput.getChecksImages()) + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(images); } @Test - void shouldBuildCorrectlyWithAllFields() { - final String text = "#Markdown Supported Text"; + void shouldBuildCorrectlyWhenAddingAnnotations() { + final ChecksOutputBuilder builder = new ChecksOutputBuilder(); + final List annotations = createAnnotations(); + annotations.forEach(builder::addAnnotation); - ChecksAnnotationBuilder builder = new ChecksAnnotationBuilder() - .withPath("src/main/java/1.java") - .withStartLine(0) - .withEndLine(10) - .withAnnotationLevel(ChecksAnnotationLevel.WARNING) - .withMessage("first annotation"); - final List annotations = - Arrays.asList(builder.withTitle("first").build(), builder.withTitle("second").build()); - final List images = - Arrays.asList(new ChecksImage("image_1", "https://www.image_1.com"), - new ChecksImage("image_2", "https://www.image_2.com")); + assertThat(builder.build().getChecksAnnotations()) + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(annotations); + } + + @Test + void shouldBuildCorrectlyWhenAddingImages() { + final ChecksOutputBuilder builder = new ChecksOutputBuilder(); + final List images = createImages(); + images.forEach(builder::addImage); + assertThat(builder.build().getChecksImages()) + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(images); + } + + @Test + void shouldCopyConstructCorrectly() { + final List annotations = createAnnotations(); + final List images = createImages(); final ChecksOutput checksOutput = new ChecksOutputBuilder() .withTitle(TITLE) .withSummary(SUMMARY) - .withText(text) - .withAnnotations(annotations) - .withImages(images) + .withText(TEXT) + .withAnnotations(annotations.subList(0, 1)) + .addAnnotation(annotations.get(1)) + .withImages(images.subList(0, 1)) + .addImage(images.get(1)) .build(); - assertThat(checksOutput).hasTitle(Optional.of(TITLE)) - .hasSummary(Optional.of(SUMMARY)) - .hasText(Optional.of(text)); - assertThat(checksOutput.getChecksAnnotations()) - .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(annotations); - assertThat(checksOutput.getChecksImages()) - .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(images); - - // test copy constructor - final ChecksOutput copied = new ChecksOutput(checksOutput); - assertThat(copied).hasTitle(Optional.of(TITLE)) + ChecksOutput copied = new ChecksOutput(checksOutput); + assertThat(copied) + .hasTitle(Optional.of(TITLE)) .hasSummary(Optional.of(SUMMARY)) - .hasText(Optional.of(text)); + .hasText(Optional.of(TEXT)); assertThat(copied.getChecksAnnotations()) - .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(annotations); + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(annotations); assertThat(copied.getChecksImages()) - .usingFieldByFieldElementComparator().containsExactlyInAnyOrderElementsOf(images); + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(images); + } + + private List createAnnotations() { + final ChecksAnnotationBuilder builder = new ChecksAnnotationBuilder() + .withPath("src/main/java/1.java") + .withStartLine(0) + .withEndLine(10) + .withAnnotationLevel(ChecksAnnotationLevel.WARNING) + .withMessage("first annotation"); + + final List annotations = new ArrayList<>(); + annotations.add(builder.withTitle("first").build()); + annotations.add(builder.withTitle("second").build()); + return annotations; + } + + private List createImages() { + final List images = new ArrayList<>(); + images.add(new ChecksImage("image_1", "https://www.image_1.com")); + images.add(new ChecksImage("image_2", "https://www.image_2.com")); + return images; } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/NullChecksPublisherTest.java b/src/test/java/io/jenkins/plugins/checks/api/NullChecksPublisherTest.java new file mode 100644 index 00000000..9c3024b4 --- /dev/null +++ b/src/test/java/io/jenkins/plugins/checks/api/NullChecksPublisherTest.java @@ -0,0 +1,20 @@ +package io.jenkins.plugins.checks.api; + +import org.junit.jupiter.api.Test; + +import hudson.model.Run; + +import io.jenkins.plugins.checks.api.ChecksDetails.ChecksDetailsBuilder; +import io.jenkins.plugins.checks.api.ChecksPublisher.NullChecksPublisher; + +import static org.mockito.Mockito.*; + +class NullChecksPublisherTest { + @Test + void shouldPublishNothingWhenInvokingPublish() { + Run run = mock(Run.class); + + NullChecksPublisher publisher = new NullChecksPublisher(run); + publisher.publish(new ChecksDetailsBuilder().build()); + } +} \ No newline at end of file From e9b18fd52267a450bc1d5241860d0e77f097b3a2 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 21:23:36 +0800 Subject: [PATCH 05/12] Enable ChecksDetails to add a single action --- .../plugins/checks/api/ChecksDetails.java | 22 ++++++++++----- .../plugins/checks/api/ChecksOutput.java | 20 ++++++------- .../plugins/checks/api/ChecksDetailsTest.java | 28 ++++++++++++++++--- 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index c8bba079..eb2c677a 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -1,6 +1,7 @@ package io.jenkins.plugins.checks.api; import java.time.LocalDateTime; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; @@ -244,7 +245,6 @@ public ChecksDetailsBuilder withOutput(final ChecksOutput output) { /** * Set the actions of a check. - * TODO: Add method to allow add action one by one * * @param actions * a list of actions @@ -252,15 +252,22 @@ public ChecksDetailsBuilder withOutput(final ChecksOutput output) { * @throws NullPointerException if the {@code actions} is null */ public ChecksDetailsBuilder withActions(final List actions) { - requireNonNull(actions); - this.actions = Collections.unmodifiableList( - actions.stream() - .map(ChecksAction::new) - .collect(Collectors.toList()) + this.actions = requireNonNull(actions).stream() + .map(ChecksAction::new) + .collect(Collectors.toList() ); return this; } + public ChecksDetailsBuilder addAction(final ChecksAction action) { + requireNonNull(action); + if (actions == Collections.EMPTY_LIST) { + actions = new ArrayList<>(); + } + actions.add(new ChecksAction(action)); + return this; + } + /** * Actually build the {@code ChecksDetail}. * @@ -268,7 +275,8 @@ public ChecksDetailsBuilder withActions(final List actions) { * @throws IllegalArgumentException if {@code conclusion} is null when {@code status} is {@code completed} */ public ChecksDetails build() { - return new ChecksDetails(name, status, detailsURL, startedAt, conclusion, completedAt, output, actions); + return new ChecksDetails(name, status, detailsURL, startedAt, conclusion, completedAt, output, + Collections.unmodifiableList(actions)); } } } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java index 78060b5a..48b39b47 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java @@ -131,8 +131,8 @@ public ChecksOutputBuilder withText(final String text) { */ public ChecksOutputBuilder withAnnotations(final List annotations) { this.annotations = requireNonNull(annotations).stream() - .map(ChecksAnnotation::new) - .collect(Collectors.toList()); + .map(ChecksAnnotation::new) + .collect(Collectors.toList()); return this; } @@ -145,10 +145,10 @@ public ChecksOutputBuilder withAnnotations(final List annotati */ public ChecksOutputBuilder addAnnotation(final ChecksAnnotation annotation) { requireNonNull(annotation); - if (this.annotations == Collections.EMPTY_LIST) { - this.annotations = new ArrayList<>(); + if (annotations == Collections.EMPTY_LIST) { + annotations = new ArrayList<>(); } - this.annotations.add(new ChecksAnnotation(annotation)); + annotations.add(new ChecksAnnotation(annotation)); return this; } @@ -160,17 +160,17 @@ public ChecksOutputBuilder addAnnotation(final ChecksAnnotation annotation) { */ public ChecksOutputBuilder withImages(final List images) { this.images = requireNonNull(images).stream() - .map(ChecksImage::new) - .collect(Collectors.toList()); + .map(ChecksImage::new) + .collect(Collectors.toList()); return this; } public ChecksOutputBuilder addImage(final ChecksImage image) { requireNonNull(image); - if (this.images == Collections.EMPTY_LIST) { - this.images = new ArrayList<>(); + if (images == Collections.EMPTY_LIST) { + images = new ArrayList<>(); } - this.images.add(new ChecksImage(image)); + images.add(new ChecksImage(image)); return this; } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java index 3957b326..6feeb84f 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksDetailsTest.java @@ -2,6 +2,7 @@ import java.time.LocalDateTime; import java.time.ZoneOffset; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Optional; @@ -26,9 +27,7 @@ void shouldBuildCorrectlyWithAllFields() { .withTitle("output") .withSummary("success") .build(); - final List actions = Arrays.asList( - new ChecksAction("action_1", "the first action", "1"), - new ChecksAction("action_2", "the second action", "2")); + final List actions = createActions(); final String detailsURL = "https://ci.jenkins.io"; final LocalDateTime startedAt = LocalDateTime.of(2020, 6, 27, 1, 10) .atOffset(ZoneOffset.UTC) @@ -45,7 +44,8 @@ void shouldBuildCorrectlyWithAllFields() { .withDetailsURL(detailsURL) .withConclusion(ChecksConclusion.SUCCESS) .withOutput(output) - .withActions(actions) + .withActions(actions.subList(0, 1)) + .addAction(actions.get(1)) .build(); assertThat(details) @@ -64,4 +64,24 @@ void shouldBuildCorrectlyWithAllFields() { .usingFieldByFieldElementComparator() .containsExactlyElementsOf(actions); } + + @Test + void shouldBuildCorrectlyWhenAddingActions() { + ChecksDetailsBuilder builder = new ChecksDetailsBuilder(); + final List actions = Arrays.asList( + new ChecksAction("action_1", "the first action", "1"), + new ChecksAction("action_2", "the second action", "2")); + actions.forEach(builder::addAction); + + assertThat(builder.build().getActions()) + .usingFieldByFieldElementComparator() + .containsExactlyInAnyOrderElementsOf(actions); + } + + private List createActions() { + final List actions = new ArrayList<>(); + actions.add(new ChecksAction("action_1", "the first action", "1")); + actions.add(new ChecksAction("action_2", "the second action", "2")); + return actions; + } } From e34c5f9f36eb6092462c1f01464c51aec2262beb Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 21:56:07 +0800 Subject: [PATCH 06/12] Improve docs --- .../plugins/checks/api/ChecksDetails.java | 36 ++++++++++--------- .../plugins/checks/api/ChecksOutput.java | 9 ++++- .../checks/github/GitHubChecksDetails.java | 13 +++++-- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index eb2c677a..7ab17b77 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -89,7 +89,7 @@ public Optional getCompletedAt() { /** * Returns the {@link ChecksOutput} of a check * - * @return An {@link ChecksOutput} of a check or null + * @return An {@link ChecksOutput} of a check */ public Optional getOutput() { return Optional.ofNullable(output); @@ -146,6 +146,10 @@ public ChecksDetailsBuilder withName(final String name) { /** * Set the status of the check. * + *

+ * Note that for a GitHub check run, if the status is not set, the default "queued" will be used. + *

+ * * @param status * the check's status * @return this builder @@ -157,7 +161,11 @@ public ChecksDetailsBuilder withStatus(final ChecksStatus status) { } /** - * Set the url of a site with full details of a check. Note that the url must use http or https scheme. + * Set the url of a site with full details of a check. + * + *

+ * Note that for a GitHub check run, the url must use http or https scheme. + *

* *

* If the details url is not set, the Jenkins build url will be used, @@ -168,7 +176,6 @@ public ChecksDetailsBuilder withStatus(final ChecksStatus status) { * the url using http or https scheme * @return this builder * @throws NullPointerException if the {@code detailsURL} is null - * @throws IllegalArgumentException if the {@code detailsURL} doesn't use http or https scheme */ public ChecksDetailsBuilder withDetailsURL(final String detailsURL) { this.detailsURL = requireNonNull(detailsURL); @@ -178,11 +185,6 @@ public ChecksDetailsBuilder withDetailsURL(final String detailsURL) { /** * Set the time when a check starts. * - *

- * If this attribute is not set and {@code conclusion} is not set as well, the time when - * {@link ChecksDetailsBuilder#build()} is called will be used. - *

- * * @param startedAt * the time when a check starts * @return this builder @@ -197,15 +199,14 @@ public ChecksDetailsBuilder withStartedAt(final LocalDateTime startedAt) { * Set the conclusion of a check. * *

- * The conclusion should only be set when the {@code status} was set {@link ChecksStatus#COMPLETED} - * when constructing this builder. + * Note that for a GitHub check run, the conclusion should only be set when the {@code status} + * was {@link ChecksStatus#COMPLETED}. *

* * @param conclusion * the conclusion * @return this builder * @throws NullPointerException if the {@code conclusion} is null - * @throws IllegalArgumentException if the {@code status} is not {@link ChecksStatus#COMPLETED} */ public ChecksDetailsBuilder withConclusion(final ChecksConclusion conclusion) { this.conclusion = requireNonNull(conclusion); @@ -215,11 +216,6 @@ public ChecksDetailsBuilder withConclusion(final ChecksConclusion conclusion) { /** * Set the time when a check completes. * - *

- * If this attribute is not set while {@code conclusion} is set, the time when - * {@link ChecksDetailsBuilder#build()} is called will be used. - *

- * * @param completedAt * the time when a check completes * @return this builder @@ -259,6 +255,13 @@ public ChecksDetailsBuilder withActions(final List actions) { return this; } + /** + * Adds a {@link ChecksAction}. + * + * @param action + * the action + * @return this builder + */ public ChecksDetailsBuilder addAction(final ChecksAction action) { requireNonNull(action); if (actions == Collections.EMPTY_LIST) { @@ -272,7 +275,6 @@ public ChecksDetailsBuilder addAction(final ChecksAction action) { * Actually build the {@code ChecksDetail}. * * @return the built {@code ChecksDetail} - * @throws IllegalArgumentException if {@code conclusion} is null when {@code status} is {@code completed} */ public ChecksDetails build() { return new ChecksDetails(name, status, detailsURL, startedAt, conclusion, completedAt, output, diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java index 48b39b47..e7970133 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java @@ -137,7 +137,7 @@ public ChecksOutputBuilder withAnnotations(final List annotati } /** - * Adds the {@link ChecksAnnotation} + * Adds a {@link ChecksAnnotation}. * * @param annotation * the annotation @@ -165,6 +165,13 @@ public ChecksOutputBuilder withImages(final List images) { return this; } + /** + * Adds a {@link ChecksImage}. + * + * @param image + * the image + * @return this builder + */ public ChecksOutputBuilder addImage(final ChecksImage image) { requireNonNull(image); if (images == Collections.EMPTY_LIST) { diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 05224a98..1cdbd30e 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -7,6 +7,8 @@ import java.util.Optional; import java.util.stream.Collectors; +import javax.swing.text.html.Option; + import org.apache.commons.lang3.StringUtils; import org.kohsuke.github.GHCheckRun.AnnotationLevel; @@ -101,7 +103,7 @@ public Optional getDetailsURL() { } /** - * Returns the time when the check started. + * Returns the UTC time when the check started. * * @return the start time of a check */ @@ -143,7 +145,7 @@ public Optional getConclusion() { } /** - * Returns the time when the check completed. + * Returns the UTC time when the check completed. * * @return the completed time of a check */ @@ -159,7 +161,7 @@ public Optional getCompletedAt() { /** * Returns the {@link Output} of a GitHub check run. * - * @return the output of a check run or null + * @return the output of a check run */ public Optional getOutput() { if (details.getOutput().isPresent()) { @@ -177,6 +179,11 @@ public Optional getOutput() { return Optional.empty(); } + /** + * Returns the {@link Action} of a GitHub check run. + * + * @return the actions list of a check run. + */ public List getActions() { return details.getActions().stream() .map(this::getAction) From 5c4eca8d038644e3e18264f7d1b82ea992f33fe7 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 21:57:21 +0800 Subject: [PATCH 07/12] Fixed always getting null when invoking GitHubChecksDetails::getOutput --- .../io/jenkins/plugins/checks/github/GitHubChecksDetails.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 1cdbd30e..2db05e0d 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -174,6 +174,7 @@ public Optional getOutput() { .withText(checksOutput.getText().orElse(null)); checksOutput.getChecksAnnotations().stream().map(this::getAnnotation).forEach(output::add); checksOutput.getChecksImages().stream().map(this::getImage).forEach(output::add); + return Optional.of(output); } return Optional.empty(); From b651ba9902d38d0e4d07ac7070aabec9045a2c7c Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 21:59:40 +0800 Subject: [PATCH 08/12] Improve grammer for blank check name validation --- .../io/jenkins/plugins/checks/github/GitHubChecksDetails.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 2db05e0d..1c0e2292 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -62,7 +62,7 @@ public GitHubChecksDetails(final ChecksDetails details) { */ public String getName() { return details.getName() - .filter(name -> !StringUtils.isBlank(name)) + .filter(StringUtils::isBlank) .orElseThrow(() -> new IllegalArgumentException("The check name is blank.")); } From 178101e409236e217510eafbbc997fe15e9af305 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Mon, 29 Jun 2020 22:09:56 +0800 Subject: [PATCH 09/12] Fixed check name validation --- .../io/jenkins/plugins/checks/github/GitHubChecksDetails.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 1c0e2292..49c7ba69 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -62,7 +62,7 @@ public GitHubChecksDetails(final ChecksDetails details) { */ public String getName() { return details.getName() - .filter(StringUtils::isBlank) + .filter(StringUtils::isNotBlank) .orElseThrow(() -> new IllegalArgumentException("The check name is blank.")); } From 1736ff6dcf4d6146832e3fd07e532894ac7eced0 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Tue, 30 Jun 2020 01:02:41 +0800 Subject: [PATCH 10/12] Enhanced error handling when resolving head sha --- src/main/java/io/jenkins/plugins/checks/ContextResolver.java | 3 ++- .../io/jenkins/plugins/checks/github/GitHubChecksDetails.java | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/ContextResolver.java b/src/main/java/io/jenkins/plugins/checks/ContextResolver.java index 6f7c87d4..bf3658be 100644 --- a/src/main/java/io/jenkins/plugins/checks/ContextResolver.java +++ b/src/main/java/io/jenkins/plugins/checks/ContextResolver.java @@ -12,6 +12,7 @@ import jenkins.scm.api.SCMSource; public class ContextResolver { + // TODO: Refactor to avoid returning null @CheckForNull public SCMSource resolveSource(final Run run) { return SCMSource.SourceByItem.findSource(run.getParent()); @@ -22,7 +23,7 @@ public String resolveHeadSha(final SCMSource source, final Run run) { try { return resolveHeadSha(source.fetch(head, null)); } catch (IOException | InterruptedException e) { - throw new IllegalStateException("Could not resolve head sha, source: " + e); + throw new IllegalStateException("Could not resolve head sha: ", e); } } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 49c7ba69..7f0d40ed 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -7,8 +7,6 @@ import java.util.Optional; import java.util.stream.Collectors; -import javax.swing.text.html.Option; - import org.apache.commons.lang3.StringUtils; import org.kohsuke.github.GHCheckRun.AnnotationLevel; From fcc5b346d8dc27918000aa28258a68451c628926 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Tue, 30 Jun 2020 01:05:10 +0800 Subject: [PATCH 11/12] Improved test assertions grammer for Optional --- .../plugins/checks/github/GitHubChecksDetailsTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java b/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java index cb09468a..879661c5 100644 --- a/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java +++ b/src/test/java/io/jenkins/plugins/checks/github/GitHubChecksDetailsTest.java @@ -1,7 +1,5 @@ package io.jenkins.plugins.checks.github; -import java.util.Optional; - import org.junit.jupiter.api.Test; import org.kohsuke.github.GHCheckRun.Conclusion; @@ -27,7 +25,7 @@ void shouldReturnAllGitHubObjectsCorrectly() { GitHubChecksDetails gitHubDetails = new GitHubChecksDetails(details); assertThat(gitHubDetails.getName()).isEqualTo("checks"); assertThat(gitHubDetails.getStatus()).isEqualTo(Status.COMPLETED); - assertThat(gitHubDetails.getConclusion()).isEqualTo(Optional.of(Conclusion.SUCCESS)); - assertThat(gitHubDetails.getDetailsURL()).isEqualTo(Optional.of("https://ci.jenkins.io")); + assertThat(gitHubDetails.getConclusion()).isPresent().hasValue(Conclusion.SUCCESS); + assertThat(gitHubDetails.getDetailsURL()).isPresent().hasValue("https://ci.jenkins.io"); } } From 58d74e22941cfc8550c044e615dd0cd1a744c593 Mon Sep 17 00:00:00 2001 From: Kezhi Xiong Date: Wed, 1 Jul 2020 00:36:31 +0800 Subject: [PATCH 12/12] Refactored ChecksAction and ChecksImage to immutable and use constructor with all @Nullable parameters --- .../plugins/checks/api/ChecksAction.java | 35 ++++++-------- .../plugins/checks/api/ChecksDetails.java | 14 ++---- .../plugins/checks/api/ChecksImage.java | 46 ++++++------------- .../plugins/checks/api/ChecksOutput.java | 25 +++------- .../checks/github/GitHubChecksDetails.java | 17 ++++++- .../plugins/checks/api/ChecksActionTest.java | 17 ++----- .../plugins/checks/api/ChecksImageTest.java | 27 +++-------- .../plugins/checks/api/ChecksOutputTest.java | 4 +- .../github/GitHubCheckRunPublishITest.java | 4 +- 9 files changed, 64 insertions(+), 125 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksAction.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksAction.java index 599250be..62ed2baa 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksAction.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksAction.java @@ -1,6 +1,8 @@ package io.jenkins.plugins.checks.api; -import static java.util.Objects.*; +import java.util.Optional; + +import edu.umd.cs.findbugs.annotations.Nullable; public class ChecksAction { private final String label; @@ -22,31 +24,22 @@ public class ChecksAction { * @param identifier * a reference for the action on the integrator's system */ - public ChecksAction(final String label, final String description, final String identifier) { - this.label = requireNonNull(label); - this.description = requireNonNull(description); - this.identifier = requireNonNull(identifier); - } - - /** - * Copy constructor of the {@link ChecksOutput}. - * - * @param that - * the source to copy from - */ - public ChecksAction(final ChecksAction that) { - this(that.getLabel(), that.getDescription(), that.getIdentifier()); + public ChecksAction(@Nullable final String label, @Nullable final String description, + @Nullable final String identifier) { + this.label = label; + this.description = description; + this.identifier = identifier; } - public String getLabel() { - return label; + public Optional getLabel() { + return Optional.ofNullable(label); } - public String getDescription() { - return description; + public Optional getDescription() { + return Optional.ofNullable(description); } - public String getIdentifier() { - return identifier; + public Optional getIdentifier() { + return Optional.ofNullable(identifier); } } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java index 7ab17b77..c77a6614 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksDetails.java @@ -5,7 +5,6 @@ import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; import static java.util.Objects.requireNonNull; @@ -122,7 +121,7 @@ public static class ChecksDetailsBuilder { */ public ChecksDetailsBuilder() { this.conclusion = ChecksConclusion.NONE; - this.actions = Collections.emptyList(); + this.actions = new ArrayList<>(); } /** @@ -248,10 +247,7 @@ public ChecksDetailsBuilder withOutput(final ChecksOutput output) { * @throws NullPointerException if the {@code actions} is null */ public ChecksDetailsBuilder withActions(final List actions) { - this.actions = requireNonNull(actions).stream() - .map(ChecksAction::new) - .collect(Collectors.toList() - ); + this.actions = new ArrayList<>(requireNonNull(actions)); return this; } @@ -263,11 +259,7 @@ public ChecksDetailsBuilder withActions(final List actions) { * @return this builder */ public ChecksDetailsBuilder addAction(final ChecksAction action) { - requireNonNull(action); - if (actions == Collections.EMPTY_LIST) { - actions = new ArrayList<>(); - } - actions.add(new ChecksAction(action)); + actions.add(requireNonNull(action)); return this; } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java index 718065b0..f3182d87 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksImage.java @@ -2,7 +2,7 @@ import java.util.Optional; -import static java.util.Objects.*; +import edu.umd.cs.findbugs.annotations.Nullable; /** * An image of a check. Users may use a image to show the code coverage, issues trend, etc. @@ -10,30 +10,22 @@ public class ChecksImage { private final String alt; private final String imageUrl; - private String caption; + private final String caption; /** - * Construct an image with required parameters. + * Constructs an image with all parameters. * * @param alt * the alternative text for the image * @param imageUrl * the full URL of the image + * @param caption + * a short description of the image */ - public ChecksImage(final String alt, final String imageUrl) { - this.alt = requireNonNull(alt); - this.imageUrl = requireNonNull(imageUrl); - } - - /** - * Copy constructor. - * - * @param that - * the source - */ - public ChecksImage(final ChecksImage that) { - this(that.getAlt(), that.getImageUrl()); - this.caption = that.getCaption().orElse(null); + public ChecksImage(@Nullable final String alt, @Nullable final String imageUrl, @Nullable final String caption) { + this.alt = alt; + this.imageUrl = imageUrl; + this.caption = caption; } /** @@ -41,8 +33,8 @@ public ChecksImage(final ChecksImage that) { * * @return the alternative text for the image */ - public String getAlt() { - return alt; + public Optional getAlt() { + return Optional.ofNullable(alt); } /** @@ -50,8 +42,8 @@ public String getAlt() { * * @return the image URL */ - public String getImageUrl() { - return imageUrl; + public Optional getImageUrl() { + return Optional.ofNullable(imageUrl); } /** @@ -62,16 +54,4 @@ public String getImageUrl() { public Optional getCaption() { return Optional.ofNullable(caption); } - - /** - * Set the short description for the image - * - * @param caption - * A short image description - * @return this image - */ - public ChecksImage withCaption(final String caption) { - this.caption = requireNonNull(caption); - return this; - } } diff --git a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java index e7970133..bf3f71bb 100644 --- a/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java +++ b/src/main/java/io/jenkins/plugins/checks/api/ChecksOutput.java @@ -4,7 +4,6 @@ import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; import static java.util.Objects.*; @@ -74,8 +73,8 @@ public static class ChecksOutputBuilder { * */ public ChecksOutputBuilder() { - this.annotations = Collections.emptyList(); - this.images = Collections.emptyList(); + this.annotations = new ArrayList<>(); + this.images = new ArrayList<>(); } /** @@ -130,9 +129,7 @@ public ChecksOutputBuilder withText(final String text) { * @return this builder */ public ChecksOutputBuilder withAnnotations(final List annotations) { - this.annotations = requireNonNull(annotations).stream() - .map(ChecksAnnotation::new) - .collect(Collectors.toList()); + this.annotations = new ArrayList<>(requireNonNull(annotations)); return this; } @@ -144,11 +141,7 @@ public ChecksOutputBuilder withAnnotations(final List annotati * @return this builder */ public ChecksOutputBuilder addAnnotation(final ChecksAnnotation annotation) { - requireNonNull(annotation); - if (annotations == Collections.EMPTY_LIST) { - annotations = new ArrayList<>(); - } - annotations.add(new ChecksAnnotation(annotation)); + annotations.add(new ChecksAnnotation(requireNonNull(annotation))); return this; } @@ -159,9 +152,7 @@ public ChecksOutputBuilder addAnnotation(final ChecksAnnotation annotation) { * @return this builder */ public ChecksOutputBuilder withImages(final List images) { - this.images = requireNonNull(images).stream() - .map(ChecksImage::new) - .collect(Collectors.toList()); + this.images = new ArrayList<>(requireNonNull(images)); return this; } @@ -173,11 +164,7 @@ public ChecksOutputBuilder withImages(final List images) { * @return this builder */ public ChecksOutputBuilder addImage(final ChecksImage image) { - requireNonNull(image); - if (images == Collections.EMPTY_LIST) { - images = new ArrayList<>(); - } - images.add(new ChecksImage(image)); + images.add(requireNonNull(image)); return this; } diff --git a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java index 7f0d40ed..f34eafa2 100644 --- a/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java +++ b/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksDetails.java @@ -190,7 +190,16 @@ public List getActions() { } private Action getAction(final ChecksAction checksAction) { - return new Action(checksAction.getLabel(), checksAction.getDescription(), checksAction.getIdentifier()); + return new Action( + checksAction.getLabel() + .orElseThrow(() -> + new IllegalArgumentException("Label of action is required but not provided")), + checksAction.getDescription() + .orElseThrow(() -> + new IllegalArgumentException("Description of action is required but not provided")), + checksAction.getIdentifier() + .orElseThrow(() -> + new IllegalArgumentException("Identifier of action is required but not provided"))); } private Annotation getAnnotation(final ChecksAnnotation checksAnnotation) { @@ -211,7 +220,11 @@ private Annotation getAnnotation(final ChecksAnnotation checksAnnotation) { } private Image getImage(final ChecksImage checksImage) { - return new Image(checksImage.getAlt(), checksImage.getImageUrl()) + return new Image( + checksImage.getAlt() + .orElseThrow(() -> new IllegalArgumentException("alt of image is required but not provided.")), + checksImage.getImageUrl() + .orElseThrow(() -> new IllegalArgumentException("url of image is required but not provided."))) .withCaption(checksImage.getCaption().orElse(null)); } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java index 97f1cbfa..bd44b816 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksActionTest.java @@ -10,19 +10,8 @@ void shouldConstructCorrectly() { final ChecksAction action = new ChecksAction("re-run", "re-run the Jenkins build", "re-run id"); - assertThat(action.getLabel()).isEqualTo("re-run"); - assertThat(action.getDescription()).isEqualTo("re-run the Jenkins build"); - assertThat(action.getIdentifier()).isEqualTo("re-run id"); - } - - @Test - void shouldCopyConstructCorrectly() { - final ChecksAction action = - new ChecksAction("re-run", "re-run the Jenkins build", "re-run id"); - final ChecksAction copied = new ChecksAction(action); - - assertThat(copied.getLabel()).isEqualTo("re-run"); - assertThat(copied.getDescription()).isEqualTo("re-run the Jenkins build"); - assertThat(copied.getIdentifier()).isEqualTo("re-run id"); + assertThat(action.getLabel()).isPresent().hasValue("re-run"); + assertThat(action.getDescription()).isPresent().hasValue("re-run the Jenkins build"); + assertThat(action.getIdentifier()).isPresent().hasValue("re-run id"); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java index c5b594c1..1e78c6a7 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksImageTest.java @@ -7,30 +7,15 @@ import static io.jenkins.plugins.checks.api.ChecksImageAssert.*; class ChecksImageTest { - final String ALT = "warnings-chart"; - final String IMAGE_URL = "https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/"; - final String CAPTION = "charts generated by warning-ng-plugin"; - @Test void shouldConstructCorrectly() { - final ChecksImage image = new ChecksImage(ALT, IMAGE_URL) - .withCaption(CAPTION); + final ChecksImage image = new ChecksImage("warnings-chart", + "https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/", + "charts generated by warning-ng-plugin"); assertThat(image) - .hasAlt(ALT) - .hasImageUrl(IMAGE_URL) - .hasCaption(Optional.of(CAPTION)); - } - - @Test - void shouldCopyConstructCorrectly() { - final ChecksImage image = new ChecksImage(ALT, IMAGE_URL) - .withCaption(CAPTION); - final ChecksImage copied = new ChecksImage(image); - - assertThat(copied) - .hasAlt(ALT) - .hasImageUrl(IMAGE_URL) - .hasCaption(Optional.of(CAPTION)); + .hasAlt(Optional.of("warnings-chart")) + .hasImageUrl(Optional.of("https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/")) + .hasCaption(Optional.of("charts generated by warning-ng-plugin")); } } diff --git a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java index 2aa8420b..1b8b3897 100644 --- a/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java +++ b/src/test/java/io/jenkins/plugins/checks/api/ChecksOutputTest.java @@ -109,8 +109,8 @@ private List createAnnotations() { private List createImages() { final List images = new ArrayList<>(); - images.add(new ChecksImage("image_1", "https://www.image_1.com")); - images.add(new ChecksImage("image_2", "https://www.image_2.com")); + images.add(new ChecksImage("image_1", "https://www.image_1.com", null)); + images.add(new ChecksImage("image_2", "https://www.image_2.com", null)); return images; } } diff --git a/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java b/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java index 4baaf194..e436d9ff 100644 --- a/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java +++ b/src/test/java/io/jenkins/plugins/checks/github/GitHubCheckRunPublishITest.java @@ -79,8 +79,8 @@ void shouldPublishGitHubCheckRunCorrectly() throws IOException { )) .withImages(Collections.singletonList( new ChecksImage("Jenkins", - "https://ci.jenkins.io/static/cd5757a8/images/jenkins-header-logo-v2.svg") - .withCaption("Jenkins Symbol") + "https://ci.jenkins.io/static/cd5757a8/images/jenkins-header-logo-v2.svg", + "Jenkins Symbol") )) .build()) .withActions(Collections.singletonList(