Mirror deployment E2E summary in CLI E2E recording comment#17154
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 -- 17154Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17154" |
Update the CLI E2E recording PR comment to surface a passed/failed breakdown in the same style as the Deployment E2E Tests comment, e.g. "✅ **CLI E2E Tests passed** — 12 passed, 0 failed". Counts are derived from the TRX outcomes that are already parsed, adding explicit pass and unknown tallies alongside the existing fail count. Recording upload failures stay surfaced but no longer drive the headline status, since they are unrelated to test correctness. Co-authored-by: Copilot <[email protected]>
Surface the test outcome on the recording link itself, not only in the table's Status column, so a URL pulled out of the table still tells a reader whether it represents a pass or a failure. Failed recordings get "❌▶️ View failure recording", passes get "✅▶️ View recording", and unknowns keep "❔". Co-authored-by: Copilot <[email protected]>
Adds a pr_number input to the manual trigger so the workflow can be exercised against a chosen PR's artifacts without depending on the open head-SHA lookup, which is needed to test changes to this workflow before they reach main. Co-authored-by: Copilot <[email protected]>
The previous yq query used jq-style 'if type == "!!seq"' which the Go yq lexer rejects, so test_outcomes.json was always empty and every recording showed as Unknown. The step had continue-on-error: true, so the failure stayed silent until the new pass/fail summary surfaced it. Convert TRX to JSON with yq -p xml -o json, then do all reshaping in jq where it handles single-vs-array UnitTestResult shapes correctly. yq also emits attributes as '+@<attr>' (newer) or '@<attr>' (older), so the lookup tries both forms. Co-authored-by: Copilot <[email protected]>
mitchdenny
force-pushed
the
mitchdenny/cli-e2e-test-summary
branch
from
f05275d to
a82dc76
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns the CLI E2E recording comment headline with the deployment E2E comment style (explicit pass/fail counts) and adds a more reliable test-outcome attribution mechanism via a .cast.outcome sidecar file written by a new EmitRecordingOutcomeAttribute, removing reliance on fragile TRX testName matching for theory tests.
Changes:
- New
EmitRecordingOutcomeAttribute(xUnit v3BeforeAfterTestAttribute) that writes a<recording>.cast.outcomesidecar; applied to all CLI E2E and Deployment E2E test classes, and recording path is published intoTestContext.KeyValueStoragefromHex1bTestHelpers.CreateTestTerminalandCliE2ETestHelpers.RegisterCaptureFile. run-tests.ymlcollects.cast.outcomesidecars alongside.castand.trxartifacts.cli-e2e-recording-comment.ymlreads the sidecar first (falling back to a hardened yq+jq TRX parser), tallies Passed/Failed/Unknown, formats a deployment-style headline, and adds apr_numberworkflow_dispatch override for dry runs.
Show a summary per file
| File | Description |
|---|---|
| tests/Shared/EmitRecordingOutcomeAttribute.cs | New attribute writing outcome sidecar after each test. |
| tests/Shared/Hex1bTestHelpers.cs | Publishes RecordingPath into TestContext.KeyValueStorage. |
| tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2ETestHelpers.cs | Also publishes recording path when the capture file is recording.cast. |
| tests/Aspire.Cli.EndToEnd.Tests/*.cs (many) | [EmitRecordingOutcome] attribute applied to all CLI E2E test classes. |
| tests/Aspire.Deployment.EndToEnd.Tests/*.cs (many) | Same attribute applied to deployment E2E test classes. |
| tests/Aspire.Cli.EndToEnd.Tests/Aspire.Cli.EndToEnd.Tests.csproj, tests/Aspire.Deployment.EndToEnd.Tests/Aspire.Deployment.EndToEnd.Tests.csproj | Link the shared attribute file. |
| .github/workflows/run-tests.yml | Copy *.cast.outcome sidecars and include them in the artifact. |
| .github/workflows/cli-e2e-recording-comment.yml | Add PR override, hardened TRX parsing, sidecar-first outcome lookup, and new deployment-style summary headline. |
Copilot's findings
- Files reviewed: 111/111 changed files
- Comments generated: 1
Theory test recordings (e.g. recordings produced from [Theory] methods) are saved with their [CallerMemberName] bare method name, while TRX reports them with parameter data appended: 'Namespace.Class.Method(toolchain: "pnpm")'. The previous parser only keyed by the last dotted segment, which preserved those parens and so never matched the .cast filename, leaving theory recordings reported as 'Unknown'. Strip the parameter suffix and also key the outcome map by the bare method name and the no-parameter FQN. 'Failed' still wins over other outcomes when the same key is seen multiple times (retries, multiple theory cases), so a single failing case still surfaces as ❌ in the comment. Co-authored-by: Copilot <[email protected]>
mitchdenny
force-pushed
the
mitchdenny/cli-e2e-test-summary
branch
from
a82dc76 to
8aef4a3
Compare
Both [Fact]s delegate to a private helper that calls CreateDockerTestTerminal, so [CallerMemberName] resolved to the helper name. That produced a single .cast file named after the private method with no matching TRX entry, which the comment workflow reported as 'Unknown'. Pass the public test name through explicitly so each Fact gets its own recording and matches TRX. Co-authored-by: Copilot <[email protected]>
When no TRX outcomes can be matched to any recording the headline used to render '0 passed, 0 failed', which reads as 'no tests ran' rather than 'outcome data unavailable'. Switch the fallback wording to '<N> recording(s), outcomes unavailable' so the headline reflects what actually happened. Co-authored-by: Copilot <[email protected]>
JamesNK
approved these changes
May 16, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
LGTM. The summary logic correctly handles all outcome combinations, the TRX parsing refactor is robust (null-guarded, handles both yq attribute naming conventions), and the [CallerMemberName] propagation fix in OtelLogsTests is correct.
Contributor
|
🎬 CLI E2E Test Recordings — 86 recordings uploaded (commit View all recordings
📹 Recordings uploaded automatically from CI run #25951116505 |
Contributor
|
✅ No documentation update needed. docs_optional → No signals triggered ( |
nellshamrell
pushed a commit
to nellshamrell/aspire
that referenced
this pull request
May 18, 2026
…#17154) * Mirror deployment E2E summary in CLI E2E recording comment Update the CLI E2E recording PR comment to surface a passed/failed breakdown in the same style as the Deployment E2E Tests comment, e.g. "✅ **CLI E2E Tests passed** — 12 passed, 0 failed". Counts are derived from the TRX outcomes that are already parsed, adding explicit pass and unknown tallies alongside the existing fail count. Recording upload failures stay surfaced but no longer drive the headline status, since they are unrelated to test correctness. Co-authored-by: Copilot <[email protected]> * Embed per-row outcome in CLI E2E recording link label Surface the test outcome on the recording link itself, not only in the table's Status column, so a URL pulled out of the table still tells a reader whether it represents a pass or a failure. Failed recordings get "❌▶️ View failure recording", passes get "✅▶️ View recording", and unknowns keep "❔". Co-authored-by: Copilot <[email protected]> * Allow workflow_dispatch to override target PR for testing Adds a pr_number input to the manual trigger so the workflow can be exercised against a chosen PR's artifacts without depending on the open head-SHA lookup, which is needed to test changes to this workflow before they reach main. Co-authored-by: Copilot <[email protected]> * Fix TRX outcome parser The previous yq query used jq-style 'if type == "!!seq"' which the Go yq lexer rejects, so test_outcomes.json was always empty and every recording showed as Unknown. The step had continue-on-error: true, so the failure stayed silent until the new pass/fail summary surfaced it. Convert TRX to JSON with yq -p xml -o json, then do all reshaping in jq where it handles single-vs-array UnitTestResult shapes correctly. yq also emits attributes as '+@<attr>' (newer) or '@<attr>' (older), so the lookup tries both forms. Co-authored-by: Copilot <[email protected]> * Match cast files against TRX theory tests by bare method name Theory test recordings (e.g. recordings produced from [Theory] methods) are saved with their [CallerMemberName] bare method name, while TRX reports them with parameter data appended: 'Namespace.Class.Method(toolchain: "pnpm")'. The previous parser only keyed by the last dotted segment, which preserved those parens and so never matched the .cast filename, leaving theory recordings reported as 'Unknown'. Strip the parameter suffix and also key the outcome map by the bare method name and the no-parameter FQN. 'Failed' still wins over other outcomes when the same key is seen multiple times (retries, multiple theory cases), so a single failing case still surfaces as ❌ in the comment. Co-authored-by: Copilot <[email protected]> * Thread CallerMemberName through OtelLogs core helper Both [Fact]s delegate to a private helper that calls CreateDockerTestTerminal, so [CallerMemberName] resolved to the helper name. That produced a single .cast file named after the private method with no matching TRX entry, which the comment workflow reported as 'Unknown'. Pass the public test name through explicitly so each Fact gets its own recording and matches TRX. Co-authored-by: Copilot <[email protected]> * Replace zero pass/fail counts with recording count in fallback summary When no TRX outcomes can be matched to any recording the headline used to render '0 passed, 0 failed', which reads as 'no tests ran' rather than 'outcome data unavailable'. Switch the fallback wording to '<N> recording(s), outcomes unavailable' so the headline reflects what actually happened. Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Mitch Denny <[email protected]>
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.
Description
Update the CLI E2E recording PR comment so the headline mirrors the Deployment E2E Tests comment style, surfacing an explicit pass/fail breakdown instead of just a recording count.
Before:
❌ **CLI E2E Test Recordings** — 2 test(s) failed, 12 recordings uploadedAfter:
❌ **CLI E2E Tests failed** — 10 passed, 2 failedThe TRX-derived outcome map is already being computed; this change just tallies the
PassedandUnknownbuckets alongside the existingFailedcount and uses them to build a summary in the same shape asdeployment-tests.yml. Recording-upload failures continue to be flagged in the table and are now appended in parentheses to the headline (e.g.... (11/12 recordings uploaded, 1 upload(s) failed)) rather than overriding the test status, since upload glitches are unrelated to test correctness. When no TRX outcomes are available the comment falls back to the existing🎬 ... completedwording.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?