Attn abayer

Thanks, therealwaldo

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 svanoort for his thoughts.

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.

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

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.