[FIXED JENKINS-48752] Drastically simplify and fix changed calculation

The previous was...complicated. In some contexts, we do need to check the execution result, not just the run result, but that led to me writing some crappy code that meant that changed always triggered for any status but SUCCESS. Sigh. So instead, specifically get the worst status from the run and the execution and use that for comparison. Suddenly everything's simpler. =)
jenkinsci · Jan 29, 2018 · 08be448 · 08be448
1 parent a247e5d
commit 08be448
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 15 deletions.
diff --git a/...ain/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Changed.groovy b/...ain/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Changed.groovy
@@ -41,24 +41,17 @@ class Changed extends BuildCondition {
         Result execResult = getExecutionResult(r)
         // Only look at the previous completed build.
         WorkflowRun prev = r.getPreviousCompletedBuild()
+
+        // Get the *worst* result of either the execution or the run. If the run's result is null, that's effectively
+        // SUCCESS.
+        Result runResult = execResult.combine(r.getResult() ?: Result.SUCCESS)
+
         // If there's no previous build, we're inherently changed.
         if (prev == null) {
             return true
-        }
-        // If the current build's result isn't null (i.e., it's got a specified status), and it's different than the
-        // previous build's result, we're changed.
-        else if ((execResult != null && prev.getResult() != execResult) ||
-            (r.getResult() != null && prev.getResult() != r.getResult())) {
-            return true
-        }
-        // If the current build's result is null and the previous build's result is not SUCCESS, we're changed.
-        else if ((execResult == Result.SUCCESS && prev.getResult() != Result.SUCCESS) ||
-            (r.getResult() == null && prev.getResult() != Result.SUCCESS)) {
-            return true
-        }
-        // And in any other condition, we're not changed, so return false.
-        else {
-            return false
+        } else {
+            // Otherwise, compare the combined execution/run result to the previous result.
+            return runResult != prev.getResult()
         }
     }
 

diff --git a/...test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BuildConditionResponderTest.java b/...test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BuildConditionResponderTest.java
@@ -207,4 +207,28 @@ public void contextResultOverridesRunResult() throws Exception {
                 .logNotContains("MOST DEFINITELY FINISHED")
                 .go();
     }
+
+    @Issue("JENKINS-48752")
+    @Test
+    public void changedAndNotSuccess() throws Exception {
+        WorkflowRun b = getAndStartNonRepoBuild("postOnChangeFailed");
+        j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b));
+        j.assertLogContains("[Pipeline] { (foo)", b);
+        j.assertLogContains("I FAILED", b);
+
+        WorkflowJob job = b.getParent();
+        job.setDefinition(new CpsFlowDefinition(pipelineSourceFromResources("postOnChangeUnstable"), true));
+        WorkflowRun b2 = j.assertBuildStatus(Result.UNSTABLE, j.waitForCompletion(job.scheduleBuild2(0).get()));
+        j.assertLogContains("[Pipeline] { (foo)", b2);
+        j.assertLogContains("hello", b2);
+        j.assertLogContains("I CHANGED", b2);
+
+        // Now make sure we don't get any alert this time.
+        WorkflowRun b3 = j.assertBuildStatus(Result.UNSTABLE, j.waitForCompletion(job.scheduleBuild2(0).get()));
+        j.assertLogContains("[Pipeline] { (foo)", b3);
+        j.assertLogContains("hello", b3);
+        j.assertLogNotContains("I CHANGED", b3);
+        j.assertLogNotContains("I FAILED", b3);
+    }
+
 }
diff --git a/pipeline-model-definition/src/test/resources/postOnChangeUnstable.groovy b/pipeline-model-definition/src/test/resources/postOnChangeUnstable.groovy
@@ -0,0 +1,48 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2018, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+pipeline {
+    agent none
+    stages {
+        stage("foo") {
+            steps {
+                echo "hello"
+                script {
+                    currentBuild.result = "UNSTABLE"
+                }
+            }
+        }
+    }
+    post {
+        changed {
+            echo "I CHANGED"
+        }
+        failure {
+            echo "I FAILED"
+        }
+    }
+}
+
+
+