Merge pull request #3 from jglick/JENKINS-26156-BodyInvoker.withDispl…

…ayName [JENKINS-26156] BodyInvoker.displayName was broken
jenkinsci · Apr 12, 2016 · 5e8cbc2 · 5e8cbc2
2 parents 654e8b4 + 5827a00
commit 5e8cbc2
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 86 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java
@@ -6,7 +6,6 @@
 import com.cloudbees.groovy.cps.Outcome;
 import com.cloudbees.groovy.cps.impl.CpsCallableInvocation;
 import com.cloudbees.groovy.cps.impl.FunctionCallEnv;
-import com.cloudbees.groovy.cps.impl.SourceLocation;
 import com.cloudbees.groovy.cps.impl.TryBlockEnv;
 import com.cloudbees.groovy.cps.sandbox.SandboxInvoker;
 import com.google.common.util.concurrent.FutureCallback;
@@ -18,14 +17,12 @@
 import org.jenkinsci.plugins.workflow.cps.nodes.StepEndNode;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
 import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
-import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.steps.BodyExecution;
 import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
 import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 import org.jenkinsci.plugins.workflow.steps.StepContext;
 import org.jenkinsci.plugins.workflow.steps.StepExecution;
 
-import javax.annotation.CheckForNull;
 import javax.annotation.concurrent.GuardedBy;
 import java.io.IOException;
 import java.util.Collection;
@@ -37,6 +34,7 @@
 import java.util.logging.Logger;
 
 import static java.util.logging.Level.*;
+import javax.annotation.Nonnull;
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
 
 /**
@@ -86,15 +84,9 @@ class CpsBodyExecution extends BodyExecution {
     @GuardedBy("this")
     private Outcome outcome;
 
-    /**
-     * @see CpsBodyInvoker#createBodyBlockNode
-     */
-    private final boolean createBodyBlockNode;
-
-    public CpsBodyExecution(CpsStepContext context, List<BodyExecutionCallback> callbacks, boolean createBodyBlockNode) {
+    CpsBodyExecution(CpsStepContext context, List<BodyExecutionCallback> callbacks) {
         this.context = context;
         this.callbacks = callbacks;
-        this.createBodyBlockNode = createBodyBlockNode;
     }
 
     /**
@@ -117,7 +109,7 @@ public CpsBodyExecution(CpsStepContext context, List<BodyExecutionCallback> call
                 sn.addAction(a);
         }
 
-        StepContext sc = subContext(sn);
+        StepContext sc = new CpsBodySubContext(context, sn);
         for (BodyExecutionCallback c : callbacks) {
             c.onStart(sc);
         }
@@ -289,7 +281,7 @@ public Next receive(Object o) {
             en.addAction(new ErrorAction(t));
 
             setOutcome(new Outcome(null,t));
-            StepContext sc = subContext(en);
+            StepContext sc = new CpsBodySubContext(context, en);
             for (BodyExecutionCallback c : callbacks) {
                 c.onFailure(sc, t);
             }
@@ -306,7 +298,7 @@ public Next receive(Object o) {
             StepEndNode en = addBodyEndFlowNode();
 
             setOutcome(new Outcome(o,null));
-            StepContext sc = subContext(en);
+            StepContext sc = new CpsBodySubContext(context, en);
             for (BodyExecutionCallback c : callbacks) {
                 c.onSuccess(sc, o);
             }
@@ -317,58 +309,38 @@ public Next receive(Object o) {
         private static final long serialVersionUID = 1L;
     }
 
-    /**
-     * Creates a sub-context to call {@link BodyExecutionCallback}.
-     * If {@link #createBodyBlockNode} is false, then we don't have distinctive
-     * {@link FlowNode}, so we just hand out the master context.
-     */
-    private StepContext subContext(FlowNode n) {
-        if (n==null)
-            return context;
-        else
-            return new CpsBodySubContext(context,n);
-    }
-
     /**
      * Inserts the flow node that indicates the beginning of the body invocation.
      *
      * @see #addBodyEndFlowNode()
      */
-    private @CheckForNull StepStartNode addBodyStartFlowNode(FlowHead head) {
-        if (createBodyBlockNode) {
-            StepStartNode start = new StepStartNode(head.getExecution(),
-                    context.getStepDescriptor(), head.get());
-            this.startNodeId = start.getId();
-            start.addAction(new BodyInvocationAction());
-            head.setNewHead(start);
-            return start;
-        } else {
-            return null;
-        }
+    private @Nonnull StepStartNode addBodyStartFlowNode(FlowHead head) {
+        StepStartNode start = new StepStartNode(head.getExecution(),
+                context.getStepDescriptor(), head.get());
+        this.startNodeId = start.getId();
+        start.addAction(new BodyInvocationAction());
+        head.setNewHead(start);
+        return start;
     }
 
     /**
      * Inserts the flow node that indicates the beginning of the body invocation.
      *
      * @see #addBodyStartFlowNode(FlowHead)
      */
-    private @CheckForNull StepEndNode addBodyEndFlowNode() {
-        if (createBodyBlockNode) {
-            try {
-                FlowHead head = CpsThread.current().head;
-
-                StepEndNode end = new StepEndNode(head.getExecution(),
-                        getBodyStartNode(), head.get());
-                end.addAction(new BodyInvocationAction());
-                head.setNewHead(end);
-
-                return end;
-            } catch (IOException e) {
-                LOGGER.log(WARNING, "Failed to grow the flow graph", e);
-                throw new Error(e);
-            }
-        } else {
-            return null;
+    private @Nonnull StepEndNode addBodyEndFlowNode() {
+        try {
+            FlowHead head = CpsThread.current().head;
+
+            StepEndNode end = new StepEndNode(head.getExecution(),
+                    getBodyStartNode(), head.get());
+            end.addAction(new BodyInvocationAction());
+            head.setNewHead(end);
+
+            return end;
+        } catch (IOException e) {
+            LOGGER.log(WARNING, "Failed to grow the flow graph", e);
+            throw new Error(e);
         }
     }
 

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java
@@ -27,18 +27,16 @@
 import com.google.common.util.concurrent.FutureCallback;
 import hudson.model.Action;
 import org.jenkinsci.plugins.workflow.actions.LabelAction;
-import org.jenkinsci.plugins.workflow.cps.nodes.StepEndNode;
-import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
 import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
 import org.jenkinsci.plugins.workflow.steps.BodyInvoker;
 
-import javax.annotation.Nullable;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import javax.annotation.Nonnull;
 
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
 
@@ -67,11 +65,6 @@ public final class CpsBodyInvoker extends BodyInvoker {
 
     private String displayName;
 
-    /**
-     * If false, do not create inner {@link StepStartNode}/{@link StepEndNode}.
-     */
-    private boolean createBodyBlockNode = true;
-
     /**
      * Set to non-null once {@linkplain #start() started}.
      */
@@ -106,9 +99,8 @@ public CpsBodyInvoker withCallback(BodyExecutionCallback callback) {
     }
 
     @Override
-    public CpsBodyInvoker withDisplayName(@Nullable String name) {
+    public CpsBodyInvoker withDisplayName(@Nonnull String name) {
         this.displayName = name;
-        createBodyBlockNode = (name==null);
         return this;
     }
 
@@ -120,16 +112,11 @@ public CpsBodyInvoker withDisplayName(@Nullable String name) {
     @Override
     public CpsBodyExecution start() {
         if (execution!=null)    throw new IllegalStateException("Already started");
-        execution = new CpsBodyExecution(owner, callbacks, createBodyBlockNode);
+        execution = new CpsBodyExecution(owner, callbacks);
 
         if (displayName!=null)
             startNodeActions.add(new LabelAction(displayName));
 
-        if (!createBodyBlockNode) {
-            if (!startNodeActions.isEmpty())
-                throw new IllegalStateException("Can't specify Actions if there will be no StepStartNode");
-        }
-
         if (owner.isCompleted()) {
             // if this step is already done, no further body invocations can happen doing so will end up
             // causing two CpsThreads competing on the same FlowHead.

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepEndNode.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepEndNode.java
@@ -1,7 +1,6 @@
 package org.jenkinsci.plugins.workflow.cps.nodes;
 
 import hudson.model.Action;
-import org.jenkinsci.plugins.workflow.actions.BodyInvocationAction;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
 import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
@@ -39,12 +38,12 @@ protected String getTypeDisplayName() {
 
     @Override
     protected String getTypeFunctionName() {
-        StepDescriptor d = getDescriptor();
         boolean isBody = getStartNode().isBody();
         if (isBody) {
-            return "} //" + (d != null ? d.getFunctionName() : getStartNode().getStepName());
+            return "}";
         } else {
-            return getStartNode().getStepName() + " : End";
+            StepDescriptor d = getDescriptor();
+            return "// " + (d != null ? d.getFunctionName() : getStartNode().getStepName());
         }
     }
 

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepStartNode.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepStartNode.java
@@ -49,11 +49,11 @@ protected String getTypeDisplayName() {
 
     @Override
     protected String getTypeFunctionName() {
-        StepDescriptor d = getDescriptor();
         if (isBody()) {
-            return (d != null ? d.getFunctionName() : descriptorId) + " {";
+            return "{";
         } else {
-            return getStepName() + " : Start";
+            StepDescriptor d = getDescriptor();
+            return (d != null ? d.getFunctionName() : descriptorId);
         }
     }
 

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java b/src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java
@@ -82,7 +82,7 @@ public static class Execution extends AbstractStepExecutionImpl {
                 context.newBodyInvoker().
                         withContexts(EnvironmentExpander.merge(context.get(EnvironmentExpander.class), new ExpanderImpl(this))).
                         withCallback(BodyExecutionCallback.wrap(context)).
-                        withDisplayName(null).start();
+                        start();
                 return false;
             }
             @Override public void onResume() {

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java b/src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java
@@ -49,6 +49,8 @@
 import org.jvnet.hudson.test.JenkinsRule;
 
 import javax.mail.Folder;
+import org.junit.ClassRule;
+import org.jvnet.hudson.test.BuildWatcher;
 
 /**
  * Test of {@link WorkflowJob} that doesn't involve Jenkins restarts.
@@ -57,6 +59,8 @@
  */
 public class WorkflowJobNonRestartingTest extends AbstractCpsFlowTest {
 
+    @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
+
     WorkflowJob p;
 
     @Before public void setUp() throws Exception {
@@ -132,7 +136,6 @@ public void testRetry() throws Exception {
 
         String log = JenkinsRule.getLog(b);
         jenkins.assertLogNotContains("\tat ", b);
-        System.err.println(log);
 
         int idx = 0;
         for (String msg : new String[] {
@@ -152,14 +155,14 @@ public void testRetry() throws Exception {
 
         idx = 0;
         for (String msg : new String[] {
-            "[Pipeline] Retry the body up to N times : Start",
-            "[Pipeline] retry {",
-            "[Pipeline] } //retry",
-            "[Pipeline] retry {",
-            "[Pipeline] } //retry",
-            "[Pipeline] retry {",
-            "[Pipeline] } //retry",
-            "[Pipeline] Retry the body up to N times : End",
+            "[Pipeline] retry",
+            "[Pipeline] {",
+            "[Pipeline] }",
+            "[Pipeline] {",
+            "[Pipeline] }",
+            "[Pipeline] {",
+            "[Pipeline] }",
+            "[Pipeline] // retry",
         }) {
             idx = log.indexOf(msg, idx + 1);
             assertTrue(msg + " not found", idx != -1);