Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify status checks output for "error" and "unstable" steps #125

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Simplify status checks output for "error" and "unstable" steps #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6e125f
Use message from "error" step in title
KalleOlaviNiemitalo Sep 5, 2021
8ba8c0d
Omit step name and arguments if trivial
KalleOlaviNiemitalo Sep 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 63 additions & 11 deletions src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,102 +22,109 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Stack;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;

class FlowExecutionAnalyzer {
private static final Logger LOGGER = Logger.getLogger(FlowExecutionAnalyzer.class.getName());
private static final String TRUNCATED_MESSAGE = "\n\nOutput truncated.";

private final Run<?, ?> run;
private final FlowExecution execution;
private final Stack<Integer> indentationStack = new Stack<>();
private final boolean suppressLogs;

FlowExecutionAnalyzer(final Run<?, ?> run, final FlowExecution execution, final boolean suppressLogs) {
this.run = run;
this.execution = execution;
this.suppressLogs = suppressLogs;
}

private static Optional<String> getStageOrBranchName(final FlowNode node) {
return getParallelName(node)
.map(Optional::of)
.orElse(getStageName(node));
}

private static Optional<String> getStageName(final FlowNode node) {
return Optional.ofNullable(node)
.filter(n -> n.getAction(ThreadNameAction.class) == null)
.map(n -> n.getAction(LabelAction.class))
.map(LabelAction::getDisplayName);
}

private static Optional<String> getParallelName(final FlowNode node) {
return Optional.ofNullable(node)
.filter(n -> n.getAction(LabelAction.class) != null)
.map(n -> n.getAction(ThreadNameAction.class))
.map(ThreadNameAction::getThreadName);
}

private Pair<String, String> processStageOrBranchRow(final FlowGraphTable.Row row,
final String stageOrBranchName) {
final StringBuilder nodeTextBuilder = new StringBuilder();
while (!indentationStack.isEmpty() && row.getTreeDepth() < indentationStack.peek()) {
indentationStack.pop();
}
if (indentationStack.isEmpty() || row.getTreeDepth() > indentationStack.peek()) {
indentationStack.push(row.getTreeDepth());
}
nodeTextBuilder.append(String.join("", Collections.nCopies(indentationStack.size(), " ")));
nodeTextBuilder.append("* ");

nodeTextBuilder.append(stageOrBranchName);

if (row.getNode().isActive()) {
nodeTextBuilder.append(" *(running)*");
}
else if (row.getDurationMillis() > 0) {
nodeTextBuilder.append(String.format(" *(%s)*", row.getDurationString()));
}
nodeTextBuilder.append("\n");
return Pair.of(nodeTextBuilder.toString(), "");
}

private Pair<String, String> processErrorOrWarningRow(final FlowGraphTable.Row row, final ErrorAction errorAction,
final WarningAction warningAction) {
FlowNode flowNode = row.getNode();

StringBuilder nodeSummaryBuilder = new StringBuilder();
StringBuilder nodeTextBuilder = new StringBuilder();

List<String> location = flowNode.getEnclosingBlocks().stream()
.map(FlowExecutionAnalyzer::getStageOrBranchName)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());

Collections.reverse(location);

location.add(flowNode.getDisplayName());

nodeSummaryBuilder.append(String.format("### `%s`%n", String.join(" / ", location)));

nodeSummaryBuilder.append(String.format("%s in `%s` step", errorAction == null ? "Warning" : "Error",
flowNode.getDisplayFunctionName()));
String arguments = ArgumentsAction.getStepArgumentsAsString(flowNode);
if (arguments == null) {
nodeSummaryBuilder.append(".\n");
}
else {
nodeSummaryBuilder.append(String.format(", with arguments `%s`.%n", arguments));
if (errorAction != null && isTrivialErrorOrUnstable(flowNode, "error", errorAction.getDisplayName())
|| warningAction != null && isTrivialErrorOrUnstable(flowNode, "unstable", warningAction.getMessage())) {

Check warning on line 111 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / CheckStyle

IndentationCheck 

NORMAL:
'||' has incorrect indentation level 12, expected level should be 16.
Raw output 
<p>Since Checkstyle 3.1</p><p> Checks correct indentation of Java code. </p><p> The idea behind this is that while pretty printers are sometimes convenient for bulk reformats of legacy code, they often either aren't configurable enough or just can't anticipate how format should be done. Sometimes this is personal preference, other times it is practical experience. In any case, this check should just ensure that a minimal set of indentation rules is followed. </p>

Check warning on line 111 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / CheckStyle

EmptyBlockCheck 

NORMAL:
Must have at least one statement.
Raw output 
<p>Since Checkstyle 3.0</p><p> Checks for empty blocks. This check does not validate sequential blocks. </p><p> Sequential blocks won't be checked. Also, no violations for fallthrough: </p><pre><code> switch (a) { case 1: // no violation case 2: // no violation case 3: someMethod(); { } // no violation default: break; } </code></pre><p> This check processes LITERAL_CASE and LITERAL_DEFAULT separately. So, if tokens=LITERAL_DEFAULT, following code will not trigger any violation, as the empty block belongs to LITERAL_CASE: </p><p> Configuration: </p><pre><code> &lt;module name="EmptyBlock"&gt; &lt;property name="tokens" value="LITERAL_DEFAULT"/&gt; &lt;/module&gt; </code></pre><p> Result: </p><pre><code> switch (a) { default: // no violation for "default:" as empty block belong to "case 1:" case 1: { } } </code></pre>
// Suppress the step name and arguments because they
// would be mostly redundant with other text.
} else {

Check warning on line 114 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / CheckStyle

RightCurlyCheck 

NORMAL:
'}' at column 9 should be alone on a line.
Raw output 
<p>Since Checkstyle 3.0</p><p> Checks the placement of right curly braces (<code>'}'</code>) for if-else, try-catch-finally blocks, while-loops, for-loops, method definitions, class definitions, constructor definitions, instance and static initialization blocks. The policy to verify is specified using the property <code> option</code>. For right curly brace of expression blocks please follow issue <a href="https://github.com/checkstyle/checkstyle/issues/5945">#5945</a>. </p>

Check warning on line 114 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / PMD

EmptyIfStmt 

NORMAL:
Avoid empty if statements.
nodeSummaryBuilder.append(String.format("%s in `%s` step", errorAction == null ? "Warning" : "Error",

Check warning on line 115 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE 

HIGH:
Redundant nullcheck of errorAction, which is known to be non-null in io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.processErrorOrWarningRow(FlowGraphTable$Row, ErrorAction, WarningAction)
Raw output 
<p> This method contains a redundant check of a known non-null value against the constant null.</p>
flowNode.getDisplayFunctionName()));
String arguments = ArgumentsAction.getStepArgumentsAsString(flowNode);
if (arguments == null) {
nodeSummaryBuilder.append(".\n");
}
else {
nodeSummaryBuilder.append(String.format(", with arguments `%s`.%n", arguments));
}
}

nodeTextBuilder.append(String.join("", Collections.nCopies(indentationStack.size() + 1, " ")));
if (warningAction == null) {

Check warning on line 127 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE 

NORMAL:
Redundant nullcheck of warningAction, which is known to be non-null in io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.processErrorOrWarningRow(FlowGraphTable$Row, ErrorAction, WarningAction)
Raw output 
<p> This method contains a redundant check of a known non-null value against the constant null.</p>
nodeTextBuilder.append(String.format("**Error**: *%s*", errorAction.getDisplayName()));
nodeSummaryBuilder.append(String.format("```%n%s%n```%n", errorAction.getDisplayName()));
if (!suppressLogs) {
Expand Down Expand Up @@ -175,16 +182,61 @@
.build();
}

private String getPotentialTitle(final FlowNode flowNode, final ErrorAction errorAction) {
final String whereBuildFailed = String.format("%s in '%s' step", errorAction == null ? "warning" : "error",
flowNode.getDisplayFunctionName());
private String getPotentialTitle(@NonNull final FlowNode flowNode, final ErrorAction errorAction) {
String whereBuildFailed = null;
if (errorAction != null
&& isTrivialErrorOrUnstable(flowNode, "error", errorAction.getDisplayName())) {
whereBuildFailed = summarize(errorAction.getDisplayName());

Check warning on line 189 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE 

NORMAL:
Possible null pointer dereference in io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.getPotentialTitle(FlowNode, ErrorAction) due to return value of called method
Raw output 
<p> The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null. This may lead to a <code>NullPointerException</code> when the code is executed. </p>
}
if (whereBuildFailed == null) {
whereBuildFailed = String.format("%s in '%s' step", errorAction == null ? "warning" : "error",

Check warning on line 192 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE 

HIGH:
Redundant nullcheck of errorAction, which is known to be non-null in io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.getPotentialTitle(FlowNode, ErrorAction)
Raw output 
<p> This method contains a redundant check of a known non-null value against the constant null.</p>
flowNode.getDisplayFunctionName());
}

List<FlowNode> enclosingStagesAndParallels = getEnclosingStagesAndParallels(flowNode);
List<String> enclosingBlockNames = getEnclosingBlockNames(enclosingStagesAndParallels);

return StringUtils.join(new ReverseListIterator(enclosingBlockNames), "/") + ": " + whereBuildFailed;
}

/**
* Check whether the node is an "error" or "unstable" step with the
* specified message and no other arguments. In that case, the caller
* can simplify the output.
*
* @param node The flow node to examine.
* @param name The expected name of the step; either "error" or "unstable".
* @param message The error or warning that was reported from the node.
*/
private static boolean isTrivialErrorOrUnstable(@NonNull final FlowNode node,

Check warning on line 211 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / CheckStyle

JavadocMethodCheck 

NORMAL:
@return tag should be present and have description.
Raw output 
<p>Since Checkstyle 3.0</p><p> Checks the Javadoc of a method or constructor. By default, does not check for unused throws. To allow documented <code>java.lang.RuntimeException</code>s that are not declared, set property allowUndeclaredRTE to true. The scope to verify is specified using the <code>Scope</code> class and defaults to <code>Scope.PRIVATE</code>. To verify another scope, set property scope to a different <a href="property_types.html#scope">scope</a>. </p><p> Error messages about parameters and type parameters for which no param tags are present can be suppressed by defining property <code>allowMissingParamTags</code>. Error messages about exceptions which are declared to be thrown, but for which no throws tag is present can be suppressed by defining property <code>allowMissingThrowsTags</code>. Error messages about methods which return non-void but for which no return tag is present can be suppressed by defining property <code>allowMissingReturnTag</code>. </p><p> Javadoc is not required on a method that is tagged with the <code>@Override</code> annotation. However under Java 5 it is not possible to mark a method required for an interface (this was <i>corrected</i> under Java 6). Hence Checkstyle supports using the convention of using a single <code>{@inheritDoc}</code> tag instead of all the other tags. </p><p> Note that only inheritable items will allow the <code>{@inheritDoc}</code> tag to be used in place of comments. Static methods at all visibilities, private non-static methods and constructors are not inheritable. </p><p> For example, if the following method is implementing a method required by an interface, then the Javadoc could be done as: </p><pre> /** {@inheritDoc} */ public int checkReturnTag(final int aTagIndex, JavadocTag[] aTags, int aLineNo)</pre><p> The classpath may need to be configured to locate the class information. The classpath configuration is dependent on the mechanism used to invoke Checkstyle. </p>
@NonNull final String name,
final String message) {
if (node instanceof StepNode) {
StepDescriptor d = ((StepNode) node).getDescriptor();
if (d != null && d.getFunctionName().equals(name)) {
Map<String, Object> arguments = ArgumentsAction.getArguments(node);
if (arguments != null && arguments.size() == 1) {

Check warning on line 218 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE 

LOW:
Redundant nullcheck of arguments, which is known to be non-null in io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.isTrivialErrorOrUnstable(FlowNode, String, String)
Raw output 
<p> This method contains a redundant check of a known non-null value against the constant null.</p>
Object argument = arguments.get("message");
return argument instanceof String
&& argument.equals(message);
}
}
}

return false;
}

private static String summarize(String message) {

Check warning on line 229 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / PMD

MethodArgumentCouldBeFinal 

NORMAL:
Parameter 'message' is not assigned and could be declared final.
Raw output 
Reports method and constructor parameters that can be made final because they are never reassigned within the body of the method. This rule ignores unused parameters so as not to overlap with the rule {% rule java/bestpractices/UnusedFormalParameter %}. It will also ignore the parameters of abstract methods. <pre> <code> class Foo { // reported, parameter can be declared final public String foo1(String param) { return param; } // not reported, parameter is declared final public String foo2(final String param) { return param.trim(); } // not reported because param is unused public String unusedParam(String param) { return &quot;abc&quot;; } } </code> </pre> <a href="https://docs.pmd-code.org/pmd-doc-7.3.0/pmd_rules_java_codestyle.html#methodargumentcouldbefinal"> See PMD documentation. </a>
if (message != null) {
final String firstLine = message.split("\r?\n", 2)[0];
if (!firstLine.isEmpty()) {
return firstLine;
Copy link

@justinmchase justinmchase Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps add a maximum length on here too in case the first line is something crazy long?

return firstLine.length > 256 ? firstLine.substr(0, 255) + "…" : firstLine;

}
}

return null;

Check warning on line 237 in src/main/java/io/jenkins/plugins/checks/status/FlowExecutionAnalyzer.java

View check run for this annotation

ci.jenkins.io / SpotBugs

NP_NONNULL_RETURN_VIOLATION 

HIGH:
io.jenkins.plugins.checks.status.FlowExecutionAnalyzer.summarize(String) may return null, but is declared @nonnull
Raw output 
<p> This method may return a null value, but the method (or a superclass method which it overrides) is declared to return @Nonnull. </p>
}

private static boolean isStageNode(@NonNull final FlowNode node) {
if (node instanceof StepNode) {
StepDescriptor d = ((StepNode) node).getDescriptor();
Expand Down
6 changes: 1 addition & 5 deletions src/test/java/io/jenkins/plugins/checks/status/BuildStatusChecksPublisherITest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.junit.After;
import org.junit.Test;
import org.junit.internal.Checks;

Check warning on line 17 in src/test/java/io/jenkins/plugins/checks/status/BuildStatusChecksPublisherITest.java

View check run for this annotation

ci.jenkins.io / PMD

UnnecessaryImport 

NORMAL:
Unused import 'org.junit.internal.Checks'.
Raw output 
Reports import statements that can be removed. They are either unused, duplicated, or the members they import are already implicitly in scope, because they're in java.lang, or the current package. If some imports cannot be resolved, for instance because you run PMD with an incomplete auxiliary classpath, some imports may be conservatively marked as used even if they're not to avoid false positives. <pre> <code> import java.io.File; // not used, can be removed import java.util.Collections; // used below import java.util.*; // so this one is not used import java.lang.Object; // imports from java.lang, unnecessary import java.lang.Object; // duplicate, unnecessary public class Foo { static Object emptyList() { return Collections.emptyList(); } } </code> </pre> <a href="https://docs.pmd-code.org/pmd-doc-7.3.0/pmd_rules_java_codestyle.html#unnecessaryimport"> See PMD documentation. </a>
import org.jvnet.hudson.test.TestExtension;

import java.util.List;
Expand Down Expand Up @@ -179,7 +179,6 @@
assertThat(output.getTitle()).isPresent().get().isEqualTo("In parallel/p1/p1s1: warning in 'unstable' step");
assertThat(output.getSummary()).isPresent().get().asString().isEqualToIgnoringNewLines(""
+ "### `In parallel / p1 / p1s1 / Set stage result to unstable`\n"
+ "Warning in `unstable` step, with arguments `something went wrong`.\n"
+ "```\n"
+ "something went wrong\n"
+ "```\n"
Expand All @@ -195,19 +194,17 @@
});

// Details 8, final checks
details = checksDetails.get(8);
assertThat(details.getStatus()).isEqualTo(ChecksStatus.COMPLETED);
assertThat(details.getConclusion()).isEqualTo(ChecksConclusion.FAILURE);
assertThat(details.getOutput()).isPresent().get().satisfies(output -> {
assertThat(output.getTitle()).isPresent().get().isEqualTo("Fails: error in 'error' step");
assertThat(output.getTitle()).isPresent().get().isEqualTo("Fails: a fatal error occurs");

Check warning on line 201 in src/test/java/io/jenkins/plugins/checks/status/BuildStatusChecksPublisherITest.java

View check run for this annotation

ci.jenkins.io / CPD

CPD 

LOW:
Found duplicated code.
Raw output 
<pre><code>details &#61; checksDetails.get(8); assertThat(details.getStatus()).isEqualTo(ChecksStatus.COMPLETED); assertThat(details.getConclusion()).isEqualTo(ChecksConclusion.FAILURE); assertThat(details.getOutput()).isPresent().get().satisfies(output -&gt; { assertThat(output.getTitle()).isPresent().get().isEqualTo(&#34;Fails: a fatal error occurs&#34;);</code></pre>
assertThat(output.getSummary()).isPresent().get().asString().matches(Pattern.compile(".*"
+ "### `In parallel / p1 / p1s1 / Set stage result to unstable`\\s+"
+ "Warning in `unstable` step, with arguments `something went wrong`\\.\\s+"
+ "```\\s+"
+ "something went wrong\\s+"
+ "```\\s+"
+ "### `Fails / Error signal`\\s+"
+ "Error in `error` step, with arguments `a fatal error occurs`\\.\\s+"
+ "```\\s+"
+ "a fatal error occurs\\s+"
+ "```\\s+", Pattern.DOTALL));
Expand Down Expand Up @@ -260,14 +257,13 @@

assertThat(checksDetails).hasSize(9);

ChecksDetails details = checksDetails.get(8);
assertThat(details.getStatus()).isEqualTo(ChecksStatus.COMPLETED);
assertThat(details.getConclusion()).isEqualTo(ChecksConclusion.FAILURE);
assertThat(details.getOutput()).isPresent().get().satisfies(output -> {
assertThat(output.getTitle()).isPresent().get().isEqualTo("Fails: error in 'archiveArtifacts' step");

Check warning on line 264 in src/test/java/io/jenkins/plugins/checks/status/BuildStatusChecksPublisherITest.java

View check run for this annotation

ci.jenkins.io / CPD

CPD 

LOW:
Found duplicated code.
Raw output 
<pre><code>details &#61; checksDetails.get(8); assertThat(details.getStatus()).isEqualTo(ChecksStatus.COMPLETED); assertThat(details.getConclusion()).isEqualTo(ChecksConclusion.FAILURE); assertThat(details.getOutput()).isPresent().get().satisfies(output -&gt; { assertThat(output.getTitle()).isPresent().get().isEqualTo(&#34;Fails: a fatal error occurs&#34;);</code></pre>
assertThat(output.getSummary()).isPresent().get().asString().matches(Pattern.compile(".*"
+ "### `In parallel / p1 / p1s1 / Set stage result to unstable`\\s+"
+ "Warning in `unstable` step, with arguments `something went wrong`\\.\\s+"
+ "```\\s+"
+ "something went wrong\\s+"
+ "```\\s+"
Expand Down
Loading