github/actions-runner
1
0
Fork 0
mirror of https://github.com/actions/runner.git synced 2026-07-03 19:19:24 +08:00
Code Issues Packages Projects Releases Wiki Activity

Drop skipped-step annotation from execution view

Browse Source 
Marking placeholders as skipped was a special-case treatment for
predicted Post placeholders whose Main step never executed. In practice
this is no different from any step that doesn't run: the debugger
simply never pauses on it, and the IStep→line mapping is only
populated when a placeholder is claimed.

Drop `IsSkipped` from `JobExecutionViewEntry`, the inline
`# (skipped — ...)` rendering, and `JobExecutionView.TryMarkSkipped`.
Placeholders that are never claimed simply stay in the view as
informational entries — the IStep→line dictionary excludes them,
so the debugger never tries to pause on them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Francesco Renzi
2026-05-13 03:50:32 -07:00
committed by GitHub
parent fedd9a3089
commit 79e0e5cbe6
4 changed files with 15 additions and 130 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
src/Runner.Worker/Dap/JobExecutionView.cs
View File

@@ -10,9 +10,21 @@ namespace GitHub.Runner.Worker.Dap
/// and provides O(1) lookup from <see cref="IStep"/> identity to the current line
/// in the rendered YAML where that step's <c>- step:</c> key appears.
///
/// Append-only growth model: post-steps are discovered lazily during execution
/// and appended. Setup/pre/main entry line numbers are stable across appends —
/// only the synthetic Cleanup boundary (which is not tracked here) shifts.
/// Each <see cref="Append"/> can register the entry in one of three modes:
/// - With a non-null <c>stepIdentity</c>: registers the IStep→line mapping
/// immediately. Used for entries whose real <see cref="IStep"/> is already
/// known at append time.
/// - With a non-null <c>matchKey</c>: registers an unclaimed placeholder
/// that a later <see cref="TryClaim"/> binds to a real <see cref="IStep"/>.
/// Used for entries whose <see cref="IStep"/> is materialized later. A
/// placeholder that is never claimed simply stays in the view and is never
/// paused on — the IStep→line mapping is only populated on claim.
/// - With neither: a static entry that needs no line lookup.
///
/// <see cref="Append"/> and <see cref="AppendRange"/> never remove or reorder
/// existing entries. <see cref="TryClaim"/> does not re-render. The IStep→line
/// mapping is rebuilt on every render, so lookups stay accurate even if a later
/// Append happens to shift previously-emitted entries.
/// </summary>
internal sealed class JobExecutionView
{
@@ -193,46 +205,6 @@ namespace GitHub.Runner.Worker.Dap
}
}
/// <summary>
/// Mark a previously-appended unclaimed placeholder as skipped. Used
/// when the predicting Main step never runs (skipped by <c>if:</c>),
/// so its predicted Post-step placeholder should not appear as a
/// step that will execute. Re-renders the view (inline comment only
/// — subsequent entry line numbers stay stable).
/// </summary>
/// <returns>
/// true if a matching unclaimed placeholder was marked; false when
/// no placeholder exists for <paramref name="matchKey"/>, or the
/// placeholder has already been claimed (claim wins).
/// </returns>
public bool TryMarkSkipped(string matchKey)
{
ArgUtil.NotNull(matchKey, nameof(matchKey));
lock (_lock)
{
if (!_unclaimedByKey.TryGetValue(matchKey, out int index))
{
return false;
}
// Defensive: only mark if it's still an unclaimed placeholder.
if (_stepIdentities[index] != null)
{
return false;
}
if (_entries[index].IsSkipped)
{
// Idempotent — already marked.
return true;
}
_entries[index].IsSkipped = true;
_unclaimedByKey.Remove(matchKey);
Render();
return true;
}
}
/// <summary>
/// Bulk-append for the initial population. Equivalent to calling
/// <see cref="Append"/> once per pair, but renders only once at the end.

14
src/Runner.Worker/Dap/JobExecutionViewRenderer.cs
View File

@@ -88,15 +88,6 @@ namespace GitHub.Runner.Worker.Dap
public string WithYaml { get; }
public string Shell { get; }
public string WorkingDirectory { get; }
/// <summary>
/// Set when the corresponding step was skipped (e.g. predicted Post
/// placeholder for a Main step that never executed because its
/// <c>if:</c> evaluated false). Rendered as an inline YAML comment
/// on the entry's <c>- step:</c> line so subsequent entry line
/// numbers stay stable.
/// </summary>
public bool IsSkipped { get; internal set; }
}
/// <summary>
@@ -213,11 +204,6 @@ namespace GitHub.Runner.Worker.Dap
startLines[i] = newlinesEmitted + 1;
sb.Append(" - step: ").Append(FormatScalar(entry.DisplayName));
if (entry.IsSkipped)
{
// Inline comment — keeps following entry line numbers stable.
sb.Append(" # (skipped — main step did not execute)");
}
sb.Append('\n');
newlinesEmitted++;

41
src/Test/L0/Worker/JobExecutionViewL0.cs
View File

@@ -422,46 +422,5 @@ namespace GitHub.Runner.Common.Tests.Worker
Assert.Contains(line.Value, entryLines);
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void TryMarkSkipped_MarksUnclaimedPlaceholder()
{
var view = new JobExecutionView("j");
var postEntry = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post X", uses: "actions/x@v1");
view.Append(postEntry, stepIdentity: null, matchKey: "k1");
Assert.True(view.TryMarkSkipped("k1"));
Assert.True(postEntry.IsSkipped);
Assert.Contains("(skipped — main step did not execute)", view.Yaml);
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void TryMarkSkipped_ReturnsFalseForUnknownKey()
{
var view = new JobExecutionView("j");
Assert.False(view.TryMarkSkipped("nope"));
Assert.DoesNotContain("(skipped", view.Yaml);
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void TryMarkSkipped_ReturnsFalseForClaimedPlaceholder()
{
var view = new JobExecutionView("j");
var postEntry = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post X", uses: "actions/x@v1");
view.Append(postEntry, stepIdentity: null, matchKey: "k1");
var step = NewStep("real-post");
Assert.NotNull(view.TryClaim("k1", step));
// Already claimed — must not mark as skipped.
Assert.False(view.TryMarkSkipped("k1"));
Assert.False(postEntry.IsSkipped);
}
}
}

32
src/Test/L0/Worker/JobExecutionViewRendererL0.cs
View File

@@ -582,38 +582,6 @@ namespace GitHub.Runner.Common.Tests.Worker
sourceLine: 0));
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void Render_EmitsSkippedAnnotationForMarkedEntry()
{
var entry = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post X", uses: "actions/x@v1");
entry.IsSkipped = true;
var result = JobExecutionViewRenderer.Render("j", new[] { entry });
// Annotation is inline on the `- step:` line so subsequent
// entry line numbers stay stable.
Assert.Contains("- step: Post X # (skipped — main step did not execute)\n", result.Yaml);
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void Render_SkippedAnnotation_DoesNotShiftSubsequentLines()
{
var skipped = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post A", uses: "actions/a@v1");
var following = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post B", uses: "actions/b@v1");
var unmarked = JobExecutionViewRenderer.Render("j", new[] { skipped, following });
skipped.IsSkipped = true;
var marked = JobExecutionViewRenderer.Render("j", new[] { skipped, following });
// Following entry's start line must not move when the prior
// entry gets an inline skipped annotation.
Assert.Equal(unmarked.EntryStartLines[1], marked.EntryStartLines[1]);
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]