Defer evaluation of a step's DisplayName until its condition is evaluated.

[jww3]

Proposed fix for #2309

This fix relocates the logic for expanding a step's display name.

In the previous implementation, the display name was evaluated as part of step execution. It was therefore possible for display names to remain unevaluated for steps that ended up being skipped.

With this change, we now evaluate the display name right before we evaluate the step's condition.

In this way, the display name gets updated even if the condition check results in the step being skipped.

Workflow used to Test

name: Issue 2309 Test
on:
  workflow_dispatch:
jobs:   
  skippedstep:
    env:
      NPM_PACKAGE_ORG: "random-name"
    strategy:
      matrix:
        variation: ["aaa","bbb"]
    runs-on: self-hosted
    steps:
      - name: Step 1 -- Actual = ${{ github.actor }}; Expected = jww3 
        run: echo "this is expanded"
        if: false
      - name: Step 2 -- Actual = ${{ env.NPM_PACKAGE_ORG }}; Expected = random-name
        run: echo "I'll be expanded"
      - name: Step 3 (fix) -- Actual = ${{ env.NPM_PACKAGE_ORG }}; Expected = random-name
        if: false
        run: echo "I'll not be expanded"
      - name: Step 4 -- Actual = ${{ matrix.variation }}; Expected = aaa or bbb
        run: echo "am expanded as well"
        if: false

source: https://github.com/bbq-beets/jww3-2022a/blob/main/.github/workflows/verify-fix-2309.yml

Before

After

[AvaStancu]

Congrats on one of your very first PRs as part of the runtime team 👏 Very clear issue descriptions also 👏 👏 👏
Would it be possible to extract your implementation to a separate method and add some unit tests for it?

[jww3]

Congrats on one of your very first PRs as part of the runtime team 👏 Very clear issue descriptions also 👏 👏 👏 Would it be possible to extract your implementation to a separate method and add some unit tests for it?

Thanks! I spent a couple hours trying to devise a unit test for this given our unit testing framework and had to give up. Thomas and I both looked at it. As best as I can tell, there's no great way in our unit testing framework to exercise this code path without extensive mocking -- that is, mocking to the point where you're not really proving anything meaningful.

[jww3]

This is a subtle change in behavior, but fortunately there weren't many existing callers (mainly just unit tests) and even fewer existing callers that even bothered checking the return value

[fhammerl]

The return value is IsDisplayNameSet, and the out var says IsUpdated[JustNow].

Why do we need both? 🤔 Is it to avoid setting it again in StepsRunner.TryUpdateDisplayName()?

[jww3]

Consider code that invokes EvaluateDisplayName multiple times and updates the TimelineRecord based on whether the call "succeeded" or not.

Example:

if (EvaluateDisplayName(this.ExecutionContext.ExpressionValues, this.ExecutionContext)) 
{
   this.ExecutionContext.UpdateTimelineRecordDisplayName(this.DisplayName);
}
.
.
.
if (EvaluateDisplayName(this.ExecutionContext.ExpressionValues, this.ExecutionContext)) 
{
   this.ExecutionContext.UpdateTimelineRecordDisplayName(this.DisplayName);
}

Which TimelineRecord update is accurate and which one is (at best) redundant?

[jww3]

in this unit test, we're setting both DisplayName and DisplayNameToken.

[jww3]

Add a comment to clarify why we only attempt this on Stage == ActionRunStage.Main

[thboop]

Its been a while but I believe a job consists of the following stages

• Job started
  • We try to evaluate all display names here
• PreJobSteps
  • No contexts have changed from the above, so no need to retry if we couldn't fully expand
• Main Steps
  • Context may now have changed, lets update the display names before we start the steps
• Post Job Steps
  • Name should have already been fully expanded, no need to try again.
• Job complete
  • Nothing to do, job is done don't evaluate display names

[jww3]

Thank you!

[fhammerl]

LGTM

[jww3]

Fix verification with latest changes:

[fhammerl]

LGTM