Show test pass/fail status in CLI E2E recording PR comments#16247
Open
Show test pass/fail status in CLI E2E recording PR comments#16247
Conversation
- Include TRX files alongside cast recordings in artifacts - Parse TRX test results to determine pass/fail per test - Show failed tests prominently at the top of the PR comment - Add ✅/❌/❔ emojis in the recordings table Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/25f46efc-7502-49ca-9918-b8dccb04c8ad Co-authored-by: radical <1472+radical@users.noreply.github.com>
Address code review feedback: - Use single element-level regex then extract attributes independently - Add clarifying comments about .cast filename to test name mapping Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/25f46efc-7502-49ca-9918-b8dccb04c8ad Co-authored-by: radical <1472+radical@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
radical
April 16, 2026 21:00
View session
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16247Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16247" |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the CI → PR-comment pipeline for CLI E2E asciinema recordings so the posted PR comment can indicate which recorded tests passed/failed by attaching and parsing TRX test results.
Changes:
- Upload
.trxfiles alongside.castrecordings in thecli-e2e-recordings-*artifact. - Extend the PR comment workflow to extract TRX files, parse per-test outcomes, and display pass/fail status + a “Failed Tests” section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/run-tests.yml | Adds TRX files to the recordings artifact so downstream comment workflow can infer outcomes. |
| .github/workflows/cli-e2e-recording-comment.yml | Extracts/parses TRX results and enriches the PR comment with statuses and a failed-tests summary. |
Only copy TRX files alongside recordings when .cast files exist, so non-recording test jobs don't produce cli-e2e-recordings artifacts containing only TRX data. Also gate the TRX parsing step in the comment workflow on cast_files presence. Addresses review feedback about artifact bloat and unnecessary processing for non-CLI-E2E test jobs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add continue-on-error: true so comment still posts if parsing crashes - Add per-file try/catch so one bad TRX doesn't block the rest - Surface parse errors as workflow annotations via core.warning() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
radical
requested review from
JamesNK,
eerhardt,
joperezr,
mitchdenny and
sebastienros
danegsta
reviewed
Apr 20, 2026
|
|
||
| // Match each <UnitTestResult ...> opening tag, then extract attributes independently. | ||
| // This handles attributes in any order within the element. | ||
| const elementRegex = /<UnitTestResult\s[^>]*>/g; |
Member
There was a problem hiding this comment.
Can we use xmllint or xq (jq like tool for XML) to read the XML from the command line (or take a dependency on a node XML parsing library)? Trying to parse the TRX files using regex feels like it'd be fairly fragile and prone to obscure failures if the format output changes in the future (even just dealing with subtle XML output differences like multiple line tags, self-closing tags, etc.).
JamesNK
reviewed
Apr 21, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
2 issues found: 1 bug (duplicate outcomes masking failures), 1 correctness/reliability concern (regex XML parsing).
Comment on lines
+200
to
+203
| const methodName = testName.includes('.') ? testName.split('.').pop() : testName; | ||
| outcomes[methodName] = outcome; | ||
| // Also store the full display name for exact matching | ||
| outcomes[testName] = outcome; |
Member
There was a problem hiding this comment.
Bug — Last-writer-wins on duplicate test method outcomes can mask failures
When the same test method name appears in multiple TRX files (e.g. from retries, or multiple test artifacts), the outcomes map is unconditionally overwritten. Since readdirSync order is filesystem-dependent, a "Passed" result from a later file could silently overwrite a "Failed" result from an earlier file — directly undermining the purpose of this PR.
Fix: prefer "Failed" over other outcomes:
if (!outcomes[methodName] || outcome === 'Failed') {
outcomes[methodName] = outcome;
}
if (!outcomes[testName] || outcome === 'Failed') {
outcomes[testName] = outcome;
}
Comment on lines
+183
to
+187
| const content = fs.readFileSync(path.join(trxDir, trxFile), 'utf8'); | ||
|
|
||
| // Match each <UnitTestResult ...> opening tag, then extract attributes independently. | ||
| // This handles attributes in any order within the element. | ||
| const elementRegex = /<UnitTestResult\s[^>]*>/g; |
Member
There was a problem hiding this comment.
Echoing @danegsta's point — regex-based XML parsing is fragile here.
yq (the Go-based one by Mike Farah) is pre-installed on ubuntu-latest runners and supports XML natively, so this could be replaced with zero extra install steps. For example:
# Convert TRX XML → JSON, extract testName→outcome map
yq -p xml -o json '.TestRun.Results.UnitTestResult' trx_files/*.trx \
| jq -s 'map({(."+@testName"): ."+@outcome"}) | add' > test_outcomes.jsonThis avoids the risk of the regex breaking on attribute reordering, self-closing tags, multi-line elements, or namespace prefix changes in future TRX format updates.
…enames - Replace regex-based TRX XML parsing with yq (pre-installed on ubuntu-latest), addressing fragility concerns raised by @danegsta and @JamesNK - Fix last-writer-wins bug: prefer 'Failed' over other outcomes when same test appears in multiple TRX files (e.g. retries), per @JamesNK's review - Sanitize .cast filenames before rendering in markdown to prevent injection in this privileged workflow_run context (artifacts may come from fork PRs) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🎬 CLI E2E Test Recordings — 72 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24814000326 |
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
Enhances the CLI E2E recording comment posted on PRs to surface test failures prominently. Previously the comment only listed recording links with no indication of which tests passed or failed.
Changes:
run-tests.yml: Bundle.trxfiles alongside.castrecordings in thecli-e2e-recordings-*artifact (only when.castfiles exist) so the comment workflow has access to test resultscli-e2e-recording-comment.yml:.castfiles are named via[CallerMemberName], matching TRXtestName)Example with failures
Checklist