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.