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

Jump to bottom

JENKINS-39203 - Only show blocks containing a WarningAction as unstable #1954

Merged
merged 12 commits into from
Jun 6, 2019
Merged

JENKINS-39203 - Only show blocks containing a WarningAction as unstable #1954

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1c75bf6
[JENKINS-39203] Only show blocks containing a WarningAction as unstable
dwnusbaum Apr 18, 2019
c108556
[JENKINS-43292] Add tests for parallel branches with failFast
dwnusbaum Apr 18, 2019
853e73a
[JENKINS-39203] Switch to incremental versions of dependencies
dwnusbaum Apr 22, 2019
4723daa
[JENKINS-39203] Add smoke test for stages with unstable status
dwnusbaum Apr 22, 2019
7d54632
[JENKINS-39203] Handle WarningAction when displaying individual steps
dwnusbaum Apr 24, 2019
d3e61e9
[JENKINS-39203] Use WarningAction.getResult for per-step results
dwnusbaum May 1, 2019
8eeacbf
Merge branch 'master' into JENKINS-39203
dwnusbaum May 1, 2019
d5861ae
Remove old baseline from weekly ATH builds
dwnusbaum May 1, 2019
86ed9c4
Update parent pom to fix Javadoc crash on JDK 11
dwnusbaum May 1, 2019
7ec470b
[JENKINS-39203] Update to released versions of plugins
dwnusbaum May 15, 2019
6da5350
Merge branch 'master' into JENKINS-39203
dwnusbaum May 29, 2019
629d57f
Use SemaphoreStep instead of Thread.sleep in PipelineNodeTest.nodesWi…
dwnusbaum Jun 5, 2019
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
4 changes: 2 additions & 2 deletions ...-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/PipelineNodeGraphVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ protected void handleChunkDone(@Nonnull MemoryFlowChunk chunk) {
if (parallelNestedStages && chunk.getFirstNode().isActive()) {
status = new NodeRunStatus(chunk.getFirstNode());
} else {
FlowNode nodeBefore = chunk.getNodeBefore(), lastNode = chunk.getLastNode(), nodeAfter = chunk.getNodeAfter();
FlowNode nodeBefore = chunk.getNodeBefore(), firstNode = chunk.getFirstNode(), lastNode = chunk.getLastNode(), nodeAfter = chunk.getNodeAfter();
status = new NodeRunStatus(
StatusAndTiming.computeChunkStatus2(run, nodeBefore, firstExecuted, lastNode, nodeAfter));
StatusAndTiming.computeChunkStatus2(run, nodeBefore, firstNode, lastNode, nodeAfter));
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought this change wasn't strictly necessary, but it turns out that it is, and it fixes things like JENKINS-57579. I can't quite tell what firstExecuted is expected to be, and it seems like it will vary based on context, but firstNode is always (even for in-progress builds), going to be the start of a stage step block or parallel branch block, and that is what we expect over in StatusAndTiming.computeChunkStatus2 anyway.

My guess as to the problem is that in some cases firstExecuted ends up being identical to or later than lastNode in the graph, and so setting up the graph traversal here in StatusAndTiming.findWarningsBetween fails and the method returns null.

}
} else {
status = new NodeRunStatus(firstExecuted);
Expand Down
52 changes: 50 additions & 2 deletions ...line-api-impl/src/test/java/io/jenkins/blueocean/rest/impl/pipeline/GraphBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,16 @@ public void downstreamBuildLinksSeq() throws Exception {
}

@Test
@Issue("JENKINS-39203")
public void sillyLongName() throws Exception {
WorkflowRun run = createAndRunJob("SillyLongName",
"earlyUnstableStatusShouldReportPunStateAsRunningAndResultAsUnknown.jenkinsfile",
Result.UNSTABLE);
NodeGraphBuilder graph = NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(run);
List<FlowNodeWrapper> nodes = graph.getPipelineNodes();

assertStageAndEdges(nodes, "stage 1 marked as unstable", BlueRun.BlueRunState.NOT_BUILT, BlueRun.BlueRunResult.NOT_BUILT, "stage 2 wait");
assertStageAndEdges(nodes, "stage 2 wait", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.UNSTABLE);
assertStageAndEdges(nodes, "stage 1 marked as unstable", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.UNSTABLE, "stage 2 wait");
assertStageAndEdges(nodes, "stage 2 wait", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS);
Copy link
Member

Choose a reason for hiding this comment

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

This test changed, does it mean existing behaviour might break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this test in particular is the exact case described in JENKINS-39203. stage 2 wait succeeded, but previously it would have been shown as unstable. The fix is that only the stage that actually set the status to unstable should be marked as unstable. I'm not sure why stage 1 was previously computed to be NOT_BUILT, but I will see if I can figure it out.

Copy link
Member

Choose a reason for hiding this comment

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

Its probably all good, i just get super nervous when tests change. Josh will be able to give better feedback next week

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this test is definitely expected to change since it was asserting exactly the behavior that we don't want anymore (i.e. it was testing the bug). No rush on reviewing, this is on top of 3 other PRs and will likely change over time, so probably better to wait to review until I upgrade it from draft status to real PR status anyway.


assertEquals("Unexpected stages in graph", 2, nodes.size());
}
Expand Down Expand Up @@ -276,6 +277,39 @@ public void parallelStagesNonNested() throws Exception {
assertEquals("Unexpected stages in graph", 3, nodes.size());
}

@Test
@Issue("JENKINS-43292")
public void parallelFailFast() throws Exception {
WorkflowRun run = createAndRunJob("parallelFailFast", "parallelFailFast.jenkinsfile", Result.FAILURE);
NodeGraphBuilder graph = NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(run);
List<FlowNodeWrapper> nodes = graph.getPipelineNodes();

assertStageAndEdges(nodes, "Parallel", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.FAILURE, "aborts", "fails", "succeeds");
assertStageAndEdges(nodes, "aborts", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.ABORTED);
assertStageAndEdges(nodes, "fails", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.FAILURE);
assertStageAndEdges(nodes, "succeeds", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS);

assertEquals("Unexpected stages in graph", 4, nodes.size());
}

@Test
@Issue("JENKINS-43292")
public void parallelFailFastDeclarative() throws Exception {
WorkflowRun run = createAndRunJob("parallelFailFastDeclarative", "parallelFailFastDeclarative.jenkinsfile", Result.FAILURE);
NodeGraphBuilder graph = NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(run);
List<FlowNodeWrapper> nodes = graph.getPipelineNodes();

// top should be BlueRunResult.FAILURE, but the status is currently computed from the beginning of top to the
Copy link
Member Author

Choose a reason for hiding this comment

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

This is because StandardChunkVisitor doesn't handled nested stages. See further discussion and an ignored test here: https://github.com/jenkinsci/pipeline-graph-analysis-plugin/pull/25/files#r277844096.

// begining of aborts, which doesn't make sense, but we don't show a status for parents of sequential stages
// anyway so it doesn't really matter.
assertStageAndEdges(nodes, "top", "aborts", "fails", "succeeds");
assertStageAndEdges(nodes, "aborts", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.ABORTED);
assertStageAndEdges(nodes, "fails", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.FAILURE);
assertStageAndEdges(nodes, "succeeds", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS);

assertEquals("Unexpected stages in graph", 4, nodes.size());
}

@Test
public void restartStage() throws Exception {

Expand Down Expand Up @@ -361,6 +395,20 @@ public void testDynamicInnerStage() throws Exception {
assertEquals("Unexpected stages in graph", 4, nodes.size());
}

@Test
public void unstableSmokes() throws Exception {
WorkflowRun run = createAndRunJob("unstableSmokes", "unstableSmokes.jenkinsfile", Result.FAILURE);
NodeGraphBuilder graph = NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(run);
List<FlowNodeWrapper> nodes = graph.getPipelineNodes();

assertStageAndEdges(nodes, "unstable-one", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.UNSTABLE, "success");
assertStageAndEdges(nodes, "success", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "unstable-two");
assertStageAndEdges(nodes, "unstable-two", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.UNSTABLE, "failure");
assertStageAndEdges(nodes, "failure", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.FAILURE);

assertEquals("Unexpected stages in graph", 4, nodes.size());
}

private FlowNodeWrapper assertStageAndEdges(Collection<FlowNodeWrapper> searchNodes, String stageName, String... edgeNames) {
return assertStageAndEdges(searchNodes, stageName, BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, edgeNames);
}
Expand Down
2 changes: 1 addition & 1 deletion ...l/pipeline/earlyUnstableStatusShouldReportPunStateAsRunningAndResultAsUnknown.jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
node {
stage ("stage 1 marked as unstable") {
currentBuild.result = 'UNSTABLE';
unstable('foo')
Copy link
Member

Choose a reason for hiding this comment

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

We should look into whether RunWrapper.setResult can be deprecated. Tricky because this was done as a method on a POJO rather than a Step, so we have no TaskListener available. Ideally currentBuild would be deprecated as a whole: jenkinsci/workflow-basic-steps-plugin#71

}
stage ("stage 2 wait") {
sleep 5
Expand Down
9 changes: 9 additions & 0 deletions ...l/src/test/resources/io/jenkins/blueocean/rest/impl/pipeline/parallelFailFast.jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
parallel failFast: true,
aborts: {
sleep 5
}, fails: {
sleep 1
error('branch fails')
}, succeeds: {
echo('branch succeeds')
}
27 changes: 27 additions & 0 deletions ...resources/io/jenkins/blueocean/rest/impl/pipeline/parallelFailFastDeclarative.jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
pipeline {
agent none
stages {
stage("top") {
failFast true
parallel {
stage("aborts") {
steps {
sleep 5
}
}
stage("fails") {
steps {
sleep 1
error "stage fails"
}
}
stage("succeeds") {
steps {
echo "stage succeeds"
}
}
}
}
}
}

21 changes: 21 additions & 0 deletions ...mpl/src/test/resources/io/jenkins/blueocean/rest/impl/pipeline/unstableSmokes.jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
stage('unstable-one') {
echo('foo')
timeout(1) {
unstable('oops-one')
}
echo('bar')
}
stage('success') {
echo('baz')
}
stage('unstable-two') {
try {
error('will-be-caught')
} catch (e) {
unstable('oops-two')
}
}
stage('failure') {
unstable('oops-masked')
error('oops-failure')
}
26 changes: 16 additions & 10 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<revision>1.15.0</revision>
<changelist>-SNAPSHOT</changelist>
<java.level>8</java.level>
<jenkins.version>2.121.1</jenkins.version> <!-- Should be kept in sync with the version resulting from Dockerfile FROM statement-->
<jenkins.version>2.138.4</jenkins.version> <!-- Should be kept in sync with the version resulting from Dockerfile FROM statement-->
Copy link
Member Author

Choose a reason for hiding this comment

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

Required for the workflow-api update. Current LTS is 2.164.2, not sure if you want to go all the way to that version though.

Copy link
Member

Choose a reason for hiding this comment

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

So do you need to update some Dockerfile as the comment asks?

Copy link
Member Author

@dwnusbaum dwnusbaum Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The Dockerfile uses FROM jenkins/jenkins:lts, and that version is currently 2.164.2. I wasn't sure if we want to update to such a new version, hence my comment in the PR. Perhaps the comment in pom.xml should just be deleted now that the Dockerfile always tracks the latest LTS.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the comment in pom.xml should just be deleted now that the Dockerfile always tracks the latest LTS.

Or the Dockerfile should be made to pin a version. Depends on what it is being used for.

Copy link
Member

Choose a reason for hiding this comment

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

I would however update the one in Jenkinsfile (but that looks like its already 2.138.4, so it doesn't matter)

Copy link
Member Author

@dwnusbaum dwnusbaum Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah good point, this line needs to be deleted or modified since we wouldn't be able to test on a baseline older than 2.138.4 anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead.

I did learn recently when i cleaned up the jenkinsfile, the weekly ath isn't run anymore anyways so no fears of breaking things

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in d5861ae.

<javadoc.exec.goal>javadoc-no-fork</javadoc.exec.goal> <!-- stop initialize phase plugins executing twice -->
<frontend-version>1.7.5</frontend-version>
<node.version>10.13.0</node.version>
Expand All @@ -45,6 +45,7 @@
<git.version>3.8.0</git.version>
<access-modifier-checker.failOnError>true</access-modifier-checker.failOnError>
<workflow-support.version>3.2</workflow-support.version>
<pipeline-model-definition.version>1.3.7</pipeline-model-definition.version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to a property to make it easy to keep everything in sync, and the update is required for the JENKINS-43292 regression tests to work.

</properties>

<scm>
Expand Down Expand Up @@ -330,7 +331,7 @@
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<version>1.3.2</version>
<version>${pipeline-model-definition.version}</version>
<exclusions>
<exclusion>
<groupId>org.jenkins-ci.plugins</groupId>
Expand All @@ -341,32 +342,37 @@
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-stage-tags-metadata</artifactId>
<version>1.3.2</version>
<version>${pipeline-model-definition.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-api</artifactId>
<version>1.3.2</version>
<version>${pipeline-model-definition.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-extensions</artifactId>
Copy link
Member Author

Choose a reason for hiding this comment

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

Just one of the other Declarative plugins, not sure why it wasn't here already.

<version>${pipeline-model-definition.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>2.58</version>
<version>2.66</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.32</version>
<version>2.34-rc893.b179faa50943</version> <!-- TODO: https://github.com/jenkinsci/workflow-api-plugin/pull/91 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>2.29</version>
<version>2.32</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>pipeline-graph-analysis</artifactId>
<version>1.7</version>
<version>1.10-rc139.a41947b4c6f1</version> <!-- TODO: https://github.com/jenkinsci/pipeline-graph-analysis-plugin/pull/25 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -397,7 +403,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-basic-steps</artifactId>
<version>2.12</version>
<version>2.16-rc702.1c79899fca89</version> <!-- TODO: https://github.com/jenkinsci/workflow-basic-steps-plugin/pull/83 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -427,7 +433,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.46</version>
<version>1.56</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
Expand Down