Merge pull request #6 from jglick/ParallelStepException-JENKINS-25894

[JENKINS-25894] Fix reporting of errors from parallel step

src/main/java/org/jenkinsci/plugins/workflow/cps/steps/ParallelStep.java

@@ -5,6 +5,7 @@
 import groovy.lang.Closure;
 import hudson.Extension;
 import hudson.model.TaskListener;
+import java.io.IOException;
 
 import org.jenkinsci.plugins.workflow.cps.CpsVmThreadOnly;
 import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
@@ -23,6 +24,8 @@
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
 
@@ -35,6 +38,8 @@
  */
 public class ParallelStep extends Step {
 
+    private static final Logger LOGGER = Logger.getLogger(ParallelStep.class.getName());
+
     /** should a failure in a parallel branch terminate other still executing branches. */
     private final boolean failFast;
 
@@ -113,8 +118,15 @@ public void onSuccess(StepContext context, Object result) {
             @Override
             public void onFailure(StepContext context, Throwable t) {
                 handler.outcomes.put(name, new Outcome(null, t));
+                try {
+                    context.get(TaskListener.class).getLogger().println("Failed in branch " + name);
+                } catch (IOException | InterruptedException x) {
+                    LOGGER.log(Level.WARNING, null, x);
+                }
                 if (handler.originalFailure == null) {
                     handler.originalFailure = new SimpleEntry<String, Throwable>(name, t);
+                } else {
+                    handler.originalFailure.getValue().addSuppressed(t);
                 }
                 checkAllDone(true);
             }
@@ -149,8 +161,7 @@ private void checkAllDone(boolean stepFailed) {
                 }
                 // all done
                 if (handler.originalFailure!=null) {
-                    // wrap the exception so that the call stack leading up to parallel is visible
-                    handler.context.onFailure(new ParallelStepException(handler.originalFailure.getKey(), handler.originalFailure.getValue()));
+                    handler.context.onFailure(handler.originalFailure.getValue());
                 } else {
                     handler.context.onSuccess(success);
                 }

src/test/java/org/jenkinsci/plugins/workflow/cps/steps/ParallelStepTest.java

@@ -11,10 +11,9 @@
 import org.jenkinsci.plugins.workflow.SingleJobTestBase;
 import org.jenkinsci.plugins.workflow.actions.ThreadNameAction;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
-import org.jenkinsci.plugins.workflow.cps.steps.ParallelStep;
-import org.jenkinsci.plugins.workflow.cps.steps.ParallelStepException;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.steps.EchoStep;
 import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
 import org.jenkinsci.plugins.workflow.steps.durable_task.BatchScriptStep;
@@ -72,7 +71,6 @@ public void failure_in_subflow_will_cause_join_to_fail() throws Exception {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition(join(
                     "import "+AbortException.class.getName(),
-                    "import "+ParallelStepException.class.getName(),
 
                     "node {",
                     "  try {",
@@ -83,9 +81,8 @@ public void failure_in_subflow_will_cause_join_to_fail() throws Exception {
                     "      a: { sleep 3; writeFile text: '', file: 'a.done' }",
                     "    )",
                     "    assert false;",
-                    "  } catch (ParallelStepException e) {",
-                    "    assert e.name=='b'",
-                    "    assert e.cause instanceof AbortException",
+                    "  } catch (AbortException e) {",
+                    "    assert e.message == 'died'",
                     "  }",
                     "}"
                 )));
@@ -111,7 +108,6 @@ public void failure_in_subflow_will_fail_fast() throws Exception {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition(join(
                     "import "+AbortException.class.getName(),
-                    "import "+ParallelStepException.class.getName(),
 
                     "node {",
                     "  try {",
@@ -123,10 +119,8 @@ public void failure_in_subflow_will_fail_fast() throws Exception {
                     "      failFast: true",
                     "    )",
                     "    assert false",
-                    "  } catch (ParallelStepException e) {",
-                    "    echo e.toString()",
-                    "    assert e.name=='b'",
-                    "    assert e.cause instanceof AbortException",
+                    "  } catch (AbortException e) {",
+                    "    assert e.message == 'died'",
                     "  }",
                     "}"
                 )));
@@ -148,9 +142,6 @@ public void failFast_has_no_effect_on_success() throws Exception {
             @Override public void evaluate() throws Throwable {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition(join(
-                    "import "+AbortException.class.getName(),
-                    "import "+ParallelStepException.class.getName(),
-
                     "node {",
                     "    parallel(",
                     "      a: { echo 'hello from a';sleep 1;echo 'goodbye from a' },",
@@ -169,6 +160,72 @@ public void failFast_has_no_effect_on_success() throws Exception {
         });
     }
 
+    @Issue("JENKINS-25894")
+    @Test public void failureReporting() throws Exception {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+                p.setDefinition(new CpsFlowDefinition("parallel a: {semaphore 'a'}, b: {semaphore 'b'}", true));
+                // Original bug report: AbortException not properly handled.
+                WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
+                SemaphoreStep.success("a/1", null);
+                SemaphoreStep.failure("b/1", new AbortException("normal failure"));
+                story.j.assertBuildStatus(Result.FAILURE, story.j.waitForCompletion(b1));
+                story.j.assertLogContains("Failed in branch b", b1);
+                // Apparently !b1.isBuilding() before WorkflowRun.finish has printed the stack trace, so need to wait for StreamBuildListener.finished:
+                story.j.waitForMessage("Finished: FAILURE", b1);
+                story.j.assertLogContains("normal failure", b1);
+                story.j.assertLogNotContains("AbortException", b1);
+                // Other exceptions should include a stack trace.
+                WorkflowRun b2 = p.scheduleBuild2(0).waitForStart();
+                SemaphoreStep.success("a/2", null);
+                SemaphoreStep.failure("b/2", new IllegalStateException("ouch"));
+                story.j.assertBuildStatus(Result.FAILURE, story.j.waitForCompletion(b2));
+                story.j.assertLogContains("Failed in branch b", b2);
+                story.j.waitForMessage("Finished: FAILURE", b2);
+                story.j.assertLogContains("java.lang.IllegalStateException: ouch", b2);
+                story.j.assertLogContains("\tat " + ParallelStepTest.class.getName(), b2);
+                // If multiple branches fail, we want to see all the stack traces.
+                WorkflowRun b3 = p.scheduleBuild2(0).waitForStart();
+                try {
+                    failureInA();
+                    fail();
+                } catch (IllegalStateException x) {
+                    SemaphoreStep.failure("a/3", x);
+                }
+                try {
+                    failureInB();
+                    fail();
+                } catch (IllegalStateException x) {
+                    SemaphoreStep.failure("b/3", x);
+                }
+                story.j.assertBuildStatus(Result.FAILURE, story.j.waitForCompletion(b3));
+                story.j.assertLogContains("Failed in branch a", b3);
+                story.j.assertLogContains("Failed in branch b", b3);
+                story.j.waitForMessage("Finished: FAILURE", b3);
+                story.j.assertLogContains("java.lang.IllegalStateException: first problem", b3);
+                story.j.assertLogContains("\tat " + ParallelStepTest.class.getName() + ".failureInA", b3);
+                story.j.assertLogContains("java.lang.IllegalStateException: second problem", b3);
+                story.j.assertLogContains("\tat " + ParallelStepTest.class.getName() + ".failureInB", b3);
+                // Also check stack traces within the script.
+                p.setDefinition(new CpsFlowDefinition(
+                    "parallel bad: {\n" +
+                    "  throw new IllegalStateException('bad')\n" +
+                    "}"));
+                WorkflowRun b4 = story.j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
+                story.j.assertLogContains("Failed in branch bad", b4);
+                story.j.waitForMessage("Finished: FAILURE", b4);
+                story.j.assertLogContains("java.lang.IllegalStateException: bad", b4);
+                story.j.assertLogContains("\tat WorkflowScript.run(WorkflowScript:2)", b4);
+            }
+        });
+    }
+    private static void failureInA() {
+        throw new IllegalStateException("first problem");
+    }
+    private static void failureInB() {
+        throw new IllegalStateException("second problem");
+    }
 
     @Test
     public void localMethodCallWithinBranch() {