Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-48178

Unstable build from junit not picked up in currentBuild.result

    Details

    • Similar Issues:

      Description

      Some time back, declarative pipelines were marking a stage as unstable, and I was able to have something like:

      pipeline {
        stage {
          steps {
            sh 'runuittests'
          }
          post {
            always {
              junit 'junit.xml'
            }
          }
        }
        stage {
          when {
            expression {
              currentBuild.resultIsBetterOrEqualTo('SUCCESS')
            }
          }
          steps {
            // Only runs when tests are stable
          }
        }
      }

      However, this stopped working at some point in the recent past.  

       

      currentBuild.result does not seem to be populated when junit marks the build unstable.

       

        Attachments

          Issue Links

            Activity

            therealwaldo Will Freeman created issue -
            Hide
            therealwaldo Will Freeman added a comment -
            Show
            therealwaldo Will Freeman added a comment - Attn Andrew Bayer
            abayer Andrew Bayer made changes -
            Field Original Value New Value
            Assignee Andrew Bayer [ abayer ]
            Hide
            abayer Andrew Bayer added a comment -

            Thanks, Will Freeman

            So this is the same underlying issue as JENKINS-37663: junit doesn't call Run#setResult(Result) any more, it calls getContext().setResult(Result), which eventually flows down to CpsFlowExecution#setResult(). I thought I was smart to do it that way, because hey, avoiding actually explicitly setting the build result as a whole is a good thing! Once we eventually have proper per-stage/step statuses, we definitely want to avoid doing that. But now currentBuild.result and friends are no longer meaningful, which is nonideal. The JENKINS-37663 fix was to make Declarative's post conditions smart enough to look at the CpsFlowExecution#getResult(), but that's a lot harder to pull off here because RunWrapper (which is what currentBuild is an instance of, more or less) is in workflow-support, so doesn't know anything about CpsFlowExecution#getResult(). That's not a method on FlowExecution, just CpsFlowExecution.

            And the more I think about it, the more I wonder if getContext().setResult(Result) was really that much smarter to use. It's true that we can call that again in the future with a better result than it currently has, which we can't do with Run#setResult(Result), but to take advantage of that, we'd need all kinds of special logic for tracking the status before we enter a try/catch or catchError and resetting back to that afterwards, which we don't have at this point. So I'd say there are two possible options here:

            • Add FlowExecution#setResult(Result) and FlowExecution#getResult() abstract methods, and change CpsFlowExecution to override them. Finally, update RunWrapper in workflow-support to pick up FlowExecution#getResult() and use similar logic to what we have in Declarative to decide which of FlowExecution#getResult() and Run#getResult() is the appropriate one to use. This doesn't help for any other cases of something expecting Run#getResult() to be accurate and current, though, and this whole approach is feeling like premature optimization.
            • Roll junit back to using Run#setResult(Result). Declarative's fine with that - its logic is smart enough to know when to ignore CpsFlowExecution#getResult() in favor of Run#getResult() anyway, so we won't break anything by changing that. It does mean that down the road, we'll have to make changes to junit to support better status granularity, but like I mentioned above, I think we were probably going to have to do that anyway.

            As you may have guessed, I'm leaning towards option #2.

            cc Sam Van Oort for his thoughts.

            Show
            abayer Andrew Bayer added a comment - Thanks, Will Freeman So this is the same underlying issue as JENKINS-37663 : junit doesn't call Run#setResult(Result) any more, it calls getContext().setResult(Result) , which eventually flows down to CpsFlowExecution#setResult() . I thought I was smart to do it that way, because hey, avoiding actually explicitly setting the build result as a whole is a good thing! Once we eventually have proper per-stage/step statuses, we definitely want to avoid doing that. But now currentBuild.result and friends are no longer meaningful, which is nonideal. The JENKINS-37663 fix was to make Declarative's post conditions smart enough to look at the CpsFlowExecution#getResult() , but that's a lot harder to pull off here because RunWrapper (which is what currentBuild is an instance of, more or less) is in workflow-support , so doesn't know anything about CpsFlowExecution#getResult() . That's not a method on FlowExecution , just CpsFlowExecution . And the more I think about it, the more I wonder if getContext().setResult(Result) was really that much smarter to use. It's true that we can call that again in the future with a better result than it currently has, which we can't do with Run#setResult(Result) , but to take advantage of that, we'd need all kinds of special logic for tracking the status before we enter a try/catch or catchError and resetting back to that afterwards, which we don't have at this point. So I'd say there are two possible options here: Add FlowExecution#setResult(Result) and FlowExecution#getResult() abstract methods, and change CpsFlowExecution to override them. Finally, update RunWrapper in workflow-support to pick up FlowExecution#getResult() and use similar logic to what we have in Declarative to decide which of FlowExecution#getResult() and Run#getResult() is the appropriate one to use. This doesn't help for any other cases of something expecting Run#getResult() to be accurate and current, though, and this whole approach is feeling like premature optimization. Roll junit back to using Run#setResult(Result) . Declarative's fine with that - its logic is smart enough to know when to ignore CpsFlowExecution#getResult() in favor of Run#getResult() anyway, so we won't break anything by changing that. It does mean that down the road, we'll have to make changes to junit to support better status granularity, but like I mentioned above, I think we were probably going to have to do that anyway. As you may have guessed, I'm leaning towards option #2. cc Sam Van Oort for his thoughts.
            Hide
            bcooksley Ben Cooksley added a comment -

            Would this issue apply equally to regular Jenkins Pipelines (non-declarative ones) as well?

            We've been hit by an issue which is basically the same as the above (currentBuild.result no longer being changed by JUnit to unstable), only we don't use declarative pipelines.

            Show
            bcooksley Ben Cooksley added a comment - Would this issue apply equally to regular Jenkins Pipelines (non-declarative ones) as well? We've been hit by an issue which is basically the same as the above (currentBuild.result no longer being changed by JUnit to unstable), only we don't use declarative pipelines.
            Hide
            abayer Andrew Bayer added a comment -

            Ben Cooksley - yup, currentBuild.result isn't picking up the junit UNSTABLE result regardless of whether you're using Declarative.

            Show
            abayer Andrew Bayer added a comment - Ben Cooksley - yup, currentBuild.result isn't picking up the junit UNSTABLE result regardless of whether you're using Declarative.
            abayer Andrew Bayer made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            abayer Andrew Bayer made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]
            Hide
            abayer Andrew Bayer added a comment -

            PR up for junit at https://github.com/jenkinsci/junit-plugin/pull/91 to switch to setting the Run result, getting currentBuild.result and friends back to behaving like expected.

            Show
            abayer Andrew Bayer added a comment - PR up for junit at https://github.com/jenkinsci/junit-plugin/pull/91 to switch to setting the Run result, getting currentBuild.result and friends back to behaving like expected.
            abayer Andrew Bayer made changes -
            Remote Link This issue links to "junit PR #91 (Web Link)" [ 18137 ]
            abayer Andrew Bayer made changes -
            Status In Review [ 10005 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            abayer Andrew Bayer made changes -
            Component/s junit-plugin [ 15499 ]
            Component/s workflow-cps-plugin [ 21713 ]
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            src/main/java/hudson/tasks/junit/pipeline/JUnitResultsStepExecution.java
            src/test/java/hudson/tasks/junit/pipeline/JUnitResultsStepTest.java
            http://jenkins-ci.org/commit/junit-plugin/acd8f4843602a4629344f4cb404cec2dfb7a54d2
            Log:
            [FIXED JENKINS-48178] Set run result rather than context result

            This fixes currentBuild.result to be UNSTABLE after test failures are
            encountered. We'll want to revisit this in the future once we have
            more granular stage/step status, but going with
            getContext().setResult(...) was, in retrospect, premature optimization.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: src/main/java/hudson/tasks/junit/pipeline/JUnitResultsStepExecution.java src/test/java/hudson/tasks/junit/pipeline/JUnitResultsStepTest.java http://jenkins-ci.org/commit/junit-plugin/acd8f4843602a4629344f4cb404cec2dfb7a54d2 Log: [FIXED JENKINS-48178] Set run result rather than context result This fixes currentBuild.result to be UNSTABLE after test failures are encountered. We'll want to revisit this in the future once we have more granular stage/step status, but going with getContext().setResult(...) was, in retrospect, premature optimization.
            Hide
            abayer Andrew Bayer added a comment -

            Releasing as 1.23 as we speak.

            Show
            abayer Andrew Bayer added a comment - Releasing as 1.23 as we speak.
            abayer Andrew Bayer made changes -
            Link This issue is duplicated by JENKINS-48267 [ JENKINS-48267 ]
            Hide
            iko IKO BH added a comment - - edited

            Andrew Bayer

            I use JUnit 1.24 and still failed tests captured by JUnit don't get the pipline to unstable state. A failed test causes the pipline to go to failed status.

            My Pipeline:

            pipeline {
              agent {
                docker {
                  image 'qnib/pytest'
                  args '-v /home/user/git/:/test-reports'
                }
              }
              stages {
                stage('Test') {
                  steps {
                    sh 'pytest --junitxml result.xml testing/'
                  }
                  post {
                    always {
                      junit 'result.xml'
                    }
                  }
                }
              }
            }

            Show
            iko IKO BH added a comment - - edited Andrew Bayer I use JUnit 1.24 and still failed tests captured by JUnit don't get the pipline to unstable state. A failed test causes the pipline to go to failed status. My Pipeline: pipeline {   agent {     docker {       image 'qnib/pytest'       args '-v /home/user/git/:/test-reports'     }   }   stages {     stage('Test') {       steps {         sh 'pytest --junitxml result.xml testing/'       }       post {         always {           junit 'result.xml'         }       }     }   } }
            Hide
            dnussbaumer Dan Nussbaumer added a comment -

            This bug still exists in version 1.24

            Show
            dnussbaumer Dan Nussbaumer added a comment - This bug still exists in version 1.24
            dnussbaumer Dan Nussbaumer made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]

              People

              • Assignee:
                abayer Andrew Bayer
                Reporter:
                therealwaldo Will Freeman
              • Votes:
                1 Vote for this issue
                Watchers:
                10 Start watching this issue

                Dates

                • Created:
                  Updated: