Uh oh!
There was an error while loading. Please reload this page.
Cover superseded retry slow-test tracking - #10840
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Verdict: Clean — no issues found.
This PR adds a focused unit test (ConsumeAsync_SupersededRetryAttempt_KeepsTrackingAsync) that exercises the IsSupersededRetryAttempt() guard in SlowTestReporterBase.ConsumeAsync (line 147 of SlowTestReporterBase.cs). I verified the production code: when a terminal update carries a superseded retry attempt, the test is intentionally not removed from _inProgress, so the slow-test scan continues to surface it. The test correctly validates this by:
- Sending an
InProgressmessage to start tracking. - Sending a
Failedmessage withRetryAttemptProperty(1, isSuperseded: true)— this should not stop tracking. - Running
ScanOnceAsyncpast the threshold and asserting a notice was still emitted.
| Dimension | Finding |
|---|---|
| Algorithmic Correctness | ✅ Test logic matches production behavior at the IsSupersededRetryAttempt() branch. |
| Test Quality | ✅ Follows existing test patterns (ConsumeAsync_TerminalState_StopsTrackingAsync, ConsumeAsync_ExecutionCompleted_StopsTrackingAsync). Naming is clear and consistent. |
| API Surface | ✅ No public API changes — CreateMessage helper gains an optional RetryAttemptProperty? parameter, internal to the test file. |
| Style & Conventions | ✅ Consistent with codebase style. |
| All other dimensions | N/A — this is a test-only change with no production code, localization, public API, or cross-TFM impact. |
No inline comments needed — the diff is clean and well-scoped.
🧪 Expert test review — PR #10840
Only one production-facing test method changed in this PR: This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
What changed in this PR
Adds focused regression coverage ensuring superseded retry attempts remain tracked by the slow-test reporter.
Changes:
- Tests superseded terminal retry handling.
- Extends the message helper with optional retry metadata.
| File | Description |
|---|---|
GitHubActionsSlowTestReporterTests.cs | Adds retry-tracking coverage and helper support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🧵 Parallel-safety audit — PR #10840Parallelization — test assembly audited:
Only one file changed: Findings: A (global-state) Nothing in the new test or the extended helper touches process-global state, shared filesystem paths, or
This is a clean, additive unit test change with no parallel-safety impact under the detected Advisory only — heuristic, non-blocking. Re-run with
|
SlowTestReporterBasemust keep tracking a test node when a terminal update represents a superseded retry attempt, because a later attempt is still running. This adds focused coverage so that behavior cannot regress unnoticed.The test sends an in-progress update followed by a failed superseded retry update, then verifies the slow-test notice still appears after the threshold. The shared test message helper now accepts optional retry metadata.
Testing
GitHubActionsSlowTestReporterTests: 12 passedFixes#10825