[JENKINS-58085] Fix issue where stages are shown as waiting to start …

…when they are running/completed (#2017)

* [JENKINS-58085] Fix issue where stage progress is not correctly shown in the UI

* [JENKINS-58085] Add minimal regression test

blueocean-pipeline-api-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/PipelineNodeGraphVisitor.java

@@ -687,7 +687,8 @@ public List<BluePipelineNode> union(List<FlowNodeWrapper> previousNodes, final L
 
         boolean graphsAreCompatible = true;
 
-        Map<String, FlowNodeWrapper> newNodes = new HashMap<>(previousNodes.size()); // indexed by id
+        // A map from the ID of a node from previousNodes to the corresponding node for the current build.
+        Map<String, FlowNodeWrapper> newNodes = new HashMap<>(previousNodes.size());
 
         // Start with the currently-executing nodes
         for (FlowNodeWrapper currentNodeWrapper : nodes) {
@@ -710,14 +711,14 @@ public List<BluePipelineNode> union(List<FlowNodeWrapper> previousNodes, final L
 
                 // New wrapper object based on current execution
                 FlowNodeWrapper newNodeWrapper = new FlowNodeWrapper(
-                    oldNodeWrapper.getNode(), // Use old node because we want to show the final id not intermediary
+                    currentNodeWrapper.getNode(),
                     currentNodeWrapper.getStatus(),
                     currentNodeWrapper.getTiming(),
                     currentNodeWrapper.getInputStep(),
                     run
                 );
 
-                newNodes.put(nodeId, newNodeWrapper);
+                newNodes.put(oldNodeWrapper.getId(), newNodeWrapper);
 
             } else {
                 // Node does not exist in previous graph, user probably changed pipleine definition.
@@ -729,9 +730,9 @@ public List<BluePipelineNode> union(List<FlowNodeWrapper> previousNodes, final L
         // Walk the old graph, create new wrappers for any stages not yet started
         if (graphsAreCompatible) {
             for (FlowNodeWrapper oldNodeWrapper : previousNodes) {
-                final String nodeId = oldNodeWrapper.getId();
+                final String oldNodeId = oldNodeWrapper.getId();
 
-                if (!newNodes.containsKey(nodeId)) {
+                if (!newNodes.containsKey(oldNodeId)) {
                     FlowNodeWrapper newNodeWrapper = new FlowNodeWrapper(
                         oldNodeWrapper.getNode(),
                         new NodeRunStatus(null, null),
@@ -740,17 +741,17 @@ public List<BluePipelineNode> union(List<FlowNodeWrapper> previousNodes, final L
                         run
                     );
 
-                    newNodes.put(nodeId, newNodeWrapper);
+                    newNodes.put(oldNodeId, newNodeWrapper);
                 }
             }
         }
 
         // Re-create edges and parentage based on previous run
         if (graphsAreCompatible) {
             for (FlowNodeWrapper oldNodeWrapper : previousNodes) {
-                final String nodeId = oldNodeWrapper.getId();
+                final String oldNodeId = oldNodeWrapper.getId();
 
-                FlowNodeWrapper newNodeWrapper = newNodes.get(nodeId);
+                FlowNodeWrapper newNodeWrapper = newNodes.get(oldNodeId);
 
                 for (FlowNodeWrapper oldEdge : oldNodeWrapper.edges) {
                     FlowNodeWrapper newEdge = newNodes.get(oldEdge.getId());
@@ -773,8 +774,8 @@ public List<BluePipelineNode> union(List<FlowNodeWrapper> previousNodes, final L
             // code that makes too many assumptions
 
             for (FlowNodeWrapper oldNodeWrapper : previousNodes) {
-                final String nodeId = oldNodeWrapper.getId();
-                FlowNodeWrapper newNodeWrapper = newNodes.get(nodeId);
+                final String oldNodeId = oldNodeWrapper.getId();
+                FlowNodeWrapper newNodeWrapper = newNodes.get(oldNodeId);
                 newList.add(new PipelineNodeImpl(newNodeWrapper, () -> parent, run));
             }
 

blueocean-pipeline-api-impl/src/test/java/io/jenkins/blueocean/rest/impl/pipeline/GraphBuilderTest.java

@@ -4,18 +4,21 @@
 import com.google.common.io.Resources;
 import hudson.model.Result;
 import io.jenkins.blueocean.listeners.NodeDownstreamBuildAction;
+import io.jenkins.blueocean.rest.hal.Link;
 import io.jenkins.blueocean.rest.model.BlueRun;
 import org.apache.commons.lang3.StringUtils;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import org.junit.Assert;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
 
 import java.net.URL;
 import java.util.Collection;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 
@@ -409,6 +412,53 @@ public void unstableSmokes() throws Exception {
         assertEquals("Unexpected stages in graph", 4, nodes.size());
     }
 
+    /**
+     * Builds a basic Pipeline where the overall graph structure (in terms of stages) is the same from one build
+     * to the next, but there are differences in the number of FlowNodes (and thus different node IDs for
+     * corresponding nodes like the starts of stages) in each build.
+     *
+     * If this test breaks, it probably means that something is mixing up node IDs across different builds when it
+     * should not be doing so.
+     */
+    @Test
+    public void unionDifferentNodeIdsSameStructure() throws Exception {
+        WorkflowJob p = createJob("unionDifferentNodeIdsSameStructure", "unionDifferentNodeIdsSameStructure.jenkinsfile");
+        // Run the first build, it should complete successfully. We don't care about its structure.   
+        WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
+        SemaphoreStep.waitForStart("second/1", b1);
+        SemaphoreStep.success("second/1", null);
+        SemaphoreStep.waitForStart("third/1", b1);
+        SemaphoreStep.success("third/1", null);
+        j.waitForCompletion(b1);
+        j.assertBuildStatus(Result.SUCCESS, b1);
+        // Run the second build. Its graph as computed by PipelineNodeGraphVisitor will be created by combining the current build with the previous build.
+        WorkflowRun b2 = p.scheduleBuild2(0).waitForStart();
+        SemaphoreStep.waitForStart("second/2", b2);
+        {
+            List<FlowNodeWrapper> nodes = unionPipelineNodes(b1, b2);
+            assertStageAndEdges(nodes, "first", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "second");
+            assertStageAndEdges(nodes, "second", BlueRun.BlueRunState.RUNNING, BlueRun.BlueRunResult.UNKNOWN, "third");
+            assertStageAndEdges(nodes, "third", null, (BlueRun.BlueRunResult) null);
+        }
+        SemaphoreStep.success("second/2", null);
+        SemaphoreStep.waitForStart("third/2", b2);
+        {
+            List<FlowNodeWrapper> nodes = unionPipelineNodes(b1, b2);
+            assertStageAndEdges(nodes, "first", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "second");
+            assertStageAndEdges(nodes, "second", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "third");
+            assertStageAndEdges(nodes, "third", BlueRun.BlueRunState.RUNNING, BlueRun.BlueRunResult.UNKNOWN);
+        }
+        SemaphoreStep.success("third/2", null);
+        j.waitForCompletion(b2);
+        j.assertBuildStatus(Result.SUCCESS, b2);
+        {
+            List<FlowNodeWrapper> nodes = unionPipelineNodes(b1, b2);
+            assertStageAndEdges(nodes, "first", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "second");
+            assertStageAndEdges(nodes, "second", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, "third");
+            assertStageAndEdges(nodes, "third", BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS);
+        }
+    }
+
     private FlowNodeWrapper assertStageAndEdges(Collection<FlowNodeWrapper> searchNodes, String stageName, String... edgeNames) {
         return assertStageAndEdges(searchNodes, stageName, BlueRun.BlueRunState.FINISHED, BlueRun.BlueRunResult.SUCCESS, edgeNames);
     }
@@ -500,4 +550,19 @@ private WorkflowJob createJob(String jobName, String jenkinsFileName) throws jav
         job.setDefinition(new CpsFlowDefinition(jenkinsFile, true));
         return job;
     }
+
+    /**
+     * Compute the union of the specified builds using {@link PipelineNodeGraphVisitor#union}.
+     * @param b1 the old build.
+     * @param b2 the current build.
+     * @return the union of the builds converted to {@code List<FlowNodeWrapper>} to be used with methods like {@link #assertStageAndEdges}.
+     */
+    private List<FlowNodeWrapper> unionPipelineNodes(WorkflowRun b1, WorkflowRun b2) {
+        List<FlowNodeWrapper> oldNodes = NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(b1).getPipelineNodes();
+        return NodeGraphBuilder.NodeGraphBuilderFactory.getInstance(b2)
+                    .union(oldNodes, new Link("unused"))
+                    .stream()
+                    .map(bpn -> ((PipelineNodeImpl) bpn).getFlowNodeWrapper())
+                    .collect(Collectors.toList());
+    }
 }

blueocean-pipeline-api-impl/src/test/resources/io/jenkins/blueocean/rest/impl/pipeline/unionDifferentNodeIdsSameStructure.jenkinsfile

@@ -0,0 +1,13 @@
+stage('first') {
+    // Changes the number of nodes in this stage on every build, so any code that
+    // relies on matching Node IDs across builds will break.
+    for (int i = 0; i < currentBuild.number; i++) {
+        echo "${i}"
+    }
+}
+stage('second') {
+    semaphore 'second'
+}
+stage('third') {
+    semaphore 'third'
+}