-
Notifications
You must be signed in to change notification settings - Fork 528
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.
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
JENKINS-39203 - Only show blocks containing a WarningAction as unstable #1954
Changes from all commits
1c75bf6
c108556
853e73a
4723daa
7d54632
d3e61e9
8eeacbf
d5861ae
86ed9c4
7ec470b
6da5350
629d57f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test changed, does it mean existing behaviour might break? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
} | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because |
||
// 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 { | ||
|
||
|
@@ -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); | ||
} | ||
|
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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should look into whether |
||
} | ||
stage ("stage 2 wait") { | ||
sleep 5 | ||
|
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') | ||
} |
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" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
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') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
<revision>1.17.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--> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So do you need to update some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Or the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
@@ -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> | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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</version> | ||
</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</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
|
@@ -397,7 +403,7 @@ | |
<dependency> | ||
<groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
<artifactId>workflow-basic-steps</artifactId> | ||
<version>2.12</version> | ||
<version>2.16</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
|
@@ -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> | ||
|
There was a problem hiding this comment.
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, butfirstNode
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 inStatusAndTiming.computeChunkStatus2
anyway.My guess as to the problem is that in some cases
firstExecuted
ends up being identical to or later thanlastNode
in the graph, and so setting up the graph traversal here in StatusAndTiming.findWarningsBetween fails and the method returns null.