Eliminate Unknown CLI E2E test results in PR comment#17484
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17484Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17484" |
Resolves bug #1 of the two root causes that left CLI E2E recordings tagged as Unknown in the PR recording-comment. CliE2ETestHelpers.CreateTestTerminal / CreateDockerTestTerminal / CreatePodmanDockerTestTerminal (and the shared Hex1bTestHelpers.CreateTestTerminal) use [CallerMemberName] to pick the .cast filename. When a public [Fact] is a thin wrapper that delegates into a private helper — e.g. DashboardRunWithAgentMcpListTracesReturnsNoTraces => DashboardRunWithAgentMcpCore in DashboardRunTests, or the *Core helper in AgentMcpLogsTests — [CallerMemberName] captures the helper. The .cast file ends up named after the helper, the TRX has no entry for that name, and the recording-comment workflow's lookup falls through to Unknown on every PR. Fix: prefer TestContext.Current?.TestCase?.TestMethodName when running inside a live xUnit test context, fall back to the [CallerMemberName] default otherwise. The public API surface is unchanged (no caller passes testName explicitly), so the recording filenames quietly flip from 'DashboardRunWithAgentMcpCore' to the public test name with no test-side edits required. The companion workflow-side fix for the second root cause (jq splitting on '.' inside theory parameters before stripping the param suffix) ships in a follow-up commit on the same PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
mitchdenny/investigate-unknown-cli-e2e-results
branch
from
9b3f6af to
9dfee9a
Compare
mitchdenny
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the C# side of two bugs that caused 5 CLI E2E tests to be tagged "Unknown" in the recording-comment PR bot. The recording-comment workflow joins .cast filenames to TRX test outcomes by method name, but when a [Fact] delegates into a private helper, [CallerMemberName] captures the helper's name and no TRX entry matches. This PR makes the recording helpers prefer xUnit's reported TestContext.Current.TestCase.TestMethodName over the caller-member-name fallback so recordings are always named after the public test.
Changes:
- Add
Hex1bTestHelpers.ResolveTestMethodNamethat prefersXunit.TestContext.Current?.TestCase?.TestMethodNameand falls back to the[CallerMemberName]value. - Apply the resolved name in
Hex1bTestHelpers.CreateTestTerminaland in all three CLI E2E factories (CreateTestTerminal,CreateDockerTestTerminal,CreatePodmanDockerTestTerminal). - Bug #2 (jq
bare_methodincli-e2e-recording-comment.yml) is described in the PR body as a patch to be applied by a maintainer and is not part of the pushed diff.
Show a summary per file
| File | Description |
|---|---|
tests/Shared/Hex1bTestHelpers.cs |
Adds ResolveTestMethodName and uses it in the shared CreateTestTerminal so deployment E2E gets the fix too. |
tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2ETestHelpers.cs |
Resolves the xUnit test method name in all three CLI terminal factories so .cast files are named after the public [Fact]/[Theory] rather than a *Core helper. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
danegsta
approved these changes
May 26, 2026
Contributor
|
❓ CLI E2E Tests unknown — 105 passed, 0 failed, 1 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26429822821 |
Contributor
|
✅ No documentation update needed. docs_required → already documented by name (false-positive signal) Triggered signals (1): The signal fired because the PR body contains All changed files are internal test helpers (
This PR fixes how |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eliminate Unknown CLI E2E test results
Across recent PRs (#17474, #17463, #17249, …) the recording-comment bot consistently tagged the same five tests as Unknown (❔):
AgentMcpListStructuredLogsFromStarterAppCore*Corehelper called by 3 public[Fact]sDashboardRunWithAgentMcpCore*Corehelper called by 2 public[Fact]sDashboardRunWithOtelTracesReturnsNoTracesCore*Corehelper called by 2 public[Fact]sAspireInitWithSolutionFileGeneratesAppHostThatBuildsAgainstChannelHive[Theory]withInlineData("Test.sln")/("Test.slnx")Root causes (two independent bugs)
Bug 1 —
[CallerMemberName]captures the helper, not the public testCliE2ETestHelpers.CreateTestTerminal/CreateDockerTestTerminal/CreatePodmanDockerTestTerminal(and the sharedHex1bTestHelpers.CreateTestTerminal) name the.castfile using[CallerMemberName]. When a[Fact]is a thin wrapper delegating into a private helper:… the cast file ends up named after the helper. The TRX has no entry for
DashboardRunWithAgentMcpCore, so the workflow'sjq -r '.[$name] // "Unknown"'falls through to Unknown.Bug 2 — jq
bare_methodsplits on.inside theory parametersThe workflow's
bare_methodruns in the wrong order:For
...Hive(solutionFileName: "Test.sln"),split(".")chops inside the quoted param andlastreturnssln"), not the method name. Verified locally:Hits every theory whose parameters contain
.(URLs, file extensions, version strings).Fixes in this PR
CreateTestTerminalfamily + shared helper: preferTestContext.Current?.TestCase?.TestMethodNameover[CallerMemberName], fall back when no test context is active.9dfee9a2ccli-e2e-recording-comment.yml: swap order inbare_method(strip params first, then split on.)workflowscopecli-e2e-recording-comment.yml: emit a::warning::annotation listing any unmatched recordings so future regressions show up in the workflow log itselfBoth Bug 1 and Bug 2 are needed to clear all five Unknowns: Bug 1 fixes the
*Corehelpers (4 tests), Bug 2 fixes the dotted-theory case (AspireInit…ChannelHive).Workflow fix patch (apply with
workflowscope)Copilot's OAuth token doesn't have
workflowscope, so the workflow change has to be applied by a maintainer. Save the block below toworkflow-fix.patchand rungit am workflow-fix.patch, or just edit the YAML by hand:Patch for
.github/workflows/cli-e2e-recording-comment.ymlFrom 8ed659e806273845ddee8d3aec6849cdf77d0695 Mon Sep 17 00:00:00 2001 From: Mitch Denny <midenn@microsoft.com> Date: Tue, 26 May 2026 13:03:02 +1000 Subject: [PATCH] Fix Unknown CLI E2E results from jq splitting theory params on '.' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves bug #2 of the two root causes that left CLI E2E recordings tagged as Unknown in the PR recording-comment. The 'bare_method' jq function used to match TRX testNames against .cast recording filenames split on '.' before stripping the theory parameter suffix: def bare_method(name): (name | split(".") | last) | sub("\\(.*$"; ""); For a TRX testName like Aspire.Cli.EndToEnd.Tests.CSharpProjectModeInitTests.AspireInitWithSolutionFileGeneratesAppHostThatBuildsAgainstChannelHive(solutionFileName: "Test.sln") split(".") chops inside the quoted parameter and 'last' returns 'sln")', not the method name. So the bare-name key in the lookup map becomes 'sln")' / 'slnx")' instead of the actual test method name, and the .cast file matches neither — Unknown. This is the same pattern for any [Theory] parameter that contains a '.' (URLs, file extensions, version strings). Fix: strip the '(<params>)' suffix first, then split on '.'. Also adds a defensive ::warning:: annotation when any .cast file fails to match a TRX outcome, so a future regression of the matching logic is visible in the recording-comment workflow's own log instead of only showing up as a degraded count in the PR comment that few people read the workflow logs for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/cli-e2e-recording-comment.yml | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cli-e2e-recording-comment.yml b/.github/workflows/cli-e2e-recording-comment.yml index 5c76728b0..9abd81e2c 100644 --- a/.github/workflows/cli-e2e-recording-comment.yml +++ b/.github/workflows/cli-e2e-recording-comment.yml @@ -195,8 +195,15 @@ jobs: # parameter data stripped) so a .cast file named after the # CallerMemberName matches a TRX entry like # "Namespace.Class.Method(toolchain: "pnpm")". + # + # IMPORTANT: strip the "(<params>)" suffix BEFORE splitting on + # ".". Theory parameters frequently contain "." (URLs, file + # extensions like "Test.sln", version strings). If split runs + # first, `last` returns garbage like `sln")` instead of the + # actual method name and the test reports as "Unknown" on every + # PR. See https://github.com/microsoft/aspire/pull/17484. def bare_method(name): - (name | split(".") | last) | sub("\\(.*$"; ""); + (name | sub("\\(.*$"; "")) | split(".") | last; def fqn_no_params(name): name | sub("\\(.*$"; ""); def merge(map; key; outcome): @@ -276,6 +283,7 @@ jobs: # Arrays to track failed test recordings separately FAILED_TESTS_BODY="" TABLE_BODY="" + UNKNOWN_FILENAMES="" for castfile in "$RECORDINGS_DIR"/*.cast; do if [ -f "$castfile" ]; then @@ -307,6 +315,12 @@ jobs: STATUS_EMOJI="❔" LINK_LABEL="❔ ▶️ View recording" TEST_UNKNOWN_COUNT=$((TEST_UNKNOWN_COUNT + 1)) + # Track the filename so we can emit a workflow warning below. + # "Unknown" almost always means the recording name didn't match any TRX + # entry (e.g. recording named after a private helper rather than the + # public [Fact]). Surfacing it here makes the regression obvious in the + # workflow logs instead of only in the PR comment. + UNKNOWN_FILENAMES="${UNKNOWN_FILENAMES} ${safe_filename}" fi # Upload to asciinema with retry logic for transient failures @@ -351,6 +365,15 @@ jobs: echo "Uploaded $UPLOAD_COUNT recordings, $FAIL_COUNT upload failures, $TEST_PASS_COUNT passed, $TEST_FAIL_COUNT failed, $TEST_UNKNOWN_COUNT unknown" + # Emit a workflow annotation for unmatched recordings so the regression + # surfaces in the GitHub Actions UI of this workflow run, not only in the + # eventual PR comment. The PR comment summarises by count, which is easy + # to overlook; a `::warning::` annotation is hard to miss when triaging + # the recording-comment workflow itself. + if [ "$TEST_UNKNOWN_COUNT" -gt 0 ]; then + echo "::warning title=CLI E2E recordings without a matching TRX outcome::${TEST_UNKNOWN_COUNT} recording(s) could not be matched to a test result and will be tagged 'Unknown' in the PR comment.${UNKNOWN_FILENAMES} See https://github.com/microsoft/aspire/blob/main/.github/workflows/cli-e2e-recording-comment.yml for the matching logic." + fi + # Build the summary line in the same style as the deployment E2E comment: # "<emoji> **CLI E2E Tests <status>** — X passed, Y failed[, Z unknown]" # Status reflects test outcomes; recording-upload failures are a secondary concern -- 2.50.1 (Apple Git-155)Verification
Both
Aspire.Cli.EndToEnd.TestsandAspire.Deployment.EndToEnd.Testsbuild clean against the C# changes.The new jq
bare_methodwas tested locally against a synthetic TRX containing the failing theory shape and a normal test:(Verified that "Failed" wins over "Passed" across theory rows via the existing
mergerule.)Out of scope
One
.castfile per theory case: today[Theory]invocations share a single recording path and the last case wins. Worth doing eventually but orthogonal to making the matching correct.