Simplify GitHubActionsAnnotationReporter: extract testName and DisplayAnnotationLineAsync helper#9678
Conversation
…yAnnotationLineAsync helper Fixes #9658 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/backport to rel/4.3 |
|
Started backporting to rel/4.3: https://github.com/microsoft/testfx/actions/runs/28841770334 |
There was a problem hiding this comment.
Pull request overview
This PR applies two small, mechanical simplifications to GitHubActionsAnnotationReporter.cs (the MTP extension that emits GitHub Actions ::error/::warning workflow-command annotations for failing and skipped tests). It follows up on the reporter introduced in #9641 and is tracked by #9658. The observable output is unchanged.
Changes:
- Hoists
GetTestName(...)into a singletestNamelocal inConsumeAsyncinstead of calling it in each branch. - Extracts the identical trailing
_outputDisplay.DisplayAsync(..., $"\n{line}")call (plus its explanatory comment) fromWriteAnnotationAsyncandWriteSkippedAnnotationAsyncinto a sharedDisplayAnnotationLineAsynchelper that returnsTaskdirectly.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAnnotationReporter.cs | Deduplicates the GetTestName call into a testName local and consolidates the shared annotation-writing logic/comment into DisplayAnnotationLineAsync. |
The DisplayAnnotationLineAsync extraction is correct and semantically equivalent — the non-async method returns the DisplayAsync task directly and callers keep .ConfigureAwait(false), so no synchronization context is captured. One minor trade-off: computing testName before the early return now evaluates it for every message (including passing tests), which I flagged as an optional nit since this reporter only needs the name when it writes an annotation.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
✅ 22/22 dimensions clean — no findings.
Clean mechanical refactoring: hoisting testName eliminates a duplicate call, and DisplayAnnotationLineAsync consolidates identical display logic with an improved unified comment. The slightly broader scope of GetTestName (now called for all node states) is negligible in this CI-only reporter and safely caught by the existing try/catch.
…yAnnotationLineAsync helper by @Evangelink in #9678 (backport to rel/4.3) (#9679) Co-authored-by: Amaury Levé <amauryleve@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The two GetTestName calls were in mutually exclusive branches (never a runtime duplicate), so hoisting them evaluated the name for every message including passing tests on the consumer hot path. Keep the calls in the skip/failure branches; retain the DisplayAnnotationLineAsync helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes #9658
Two small, purely mechanical simplifications to
GitHubActionsAnnotationReporter.cs.Improvements
Eliminated duplicate
GetTestNamecall inConsumeAsync—GetTestName(nodeUpdateMessage.TestNode)was called in both the skipped and failure branches. It's now computed once into atestNamelocal before the branch.Extracted shared
DisplayAnnotationLineAsynchelper —WriteAnnotationAsyncandWriteSkippedAnnotationAsyncboth ended with an identical_outputDisplay.DisplayAsync(this, new FormattedTextOutputDeviceData($"\n{line}"), ...)call plus a comment explaining why a leading newline is required. This is now centralized in one method (returningTaskdirectly), consolidating the explanation in a single place. Callers still apply.ConfigureAwait(false)to the returnedTask.Testing
Microsoft.Testing.Extensions.GitHubActionsReportbuilds with 0 warnings / 0 errors across all target frameworksGitHubActionsAnnotationReporterTestspass unchangedCo-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com