Attach retry artifacts to Azure DevOps sub-results

[Amaury Levé (Evangelink)]

Summary

• Upload retry-attempt artifacts to the Azure DevOps sub-result that produced them.
• Use server-assigned sub-result IDs returned by result create/update operations.
• Preserve original attachment names and keep ordinary result attachments on the parent.
• Avoid replaying accepted result writes when Azure DevOps response bodies cannot be read or parsed.

Fixes #10558

Testing

• Built Microsoft.Testing.Extensions.UnitTests across all configured target frameworks.
• Ran all 122 AzureDevOpsLivePublishingTests on net8.0.

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Expert Review Summary — PR #10723

Attach retry artifacts to Azure DevOps sub-results

This PR correctly switches from renaming attachments per-attempt to uploading them against the AzDO sub-result that produced them, using server-assigned sub-result IDs. The design is clean: response parsing is extracted into a shared ParsePublishedResults helper, the WithFileName/RenameForAttempt workaround is removed, and failure to resolve sub-result IDs is handled gracefully (count attachments as failed, log warning, continue).

#	Dimension	Verdict
1	Algorithmic Correctness	✅ LGTM — validation logic in ParsePublishedResults handles mismatched counts, bad IDs, duplicate sequence IDs
2	Threading & Concurrency	✅ LGTM — Interlocked.Add on _failedAttachmentCount is correct
3	Security & IPC Contract Safety	✅ N/A
4	Public API & Binary Compatibility	✅ LGTM — all types are internal, changes tracked in InternalAPI.Unshipped.txt
5	Performance & Allocations	✅ LGTM — HttpCompletionOption.ResponseHeadersRead is a nice improvement; Dictionary<int,int> per result is proportional to sub-result count
6	Cross-TFM Compatibility	✅ N/A — no TFM-specific APIs introduced
7	Resource & IDisposable Management	✅ LGTM — CancellationTokenSource properly using-scoped in new code paths
8	Defensive Coding at Boundaries	✅ LGTM — response parsing failures return null without retrying the accepted write; OperationCanceledException, JsonException, HttpRequestException, IOException all caught
9	Localization & Resources	✅ LGTM — warning message uses existing resource string
10	Test Isolation	✅ LGTM
11	Assertion Quality	✅ LGTM — assertions verify the right things (sub-result IDs, URI query params, attachment names)
12	Flakiness Patterns	✅ LGTM
13	Test Completeness & Coverage	✅ LGTM — 6 new/updated tests covering create path, update path, missing sub-results, response read failure, attachment URI with/without sub-result
14	Data-Driven Test Coverage	✅ N/A
15	Code Structure & Simplification	📝 NIT — one inline comment posted (collapsible double _resultIdStore null check)
16	Naming & Conventions	✅ LGTM
17	Documentation Accuracy	✅ LGTM — obsolete <remarks> on WithFileName and UpdateTestResultsAsync properly removed
18	Analyzer & Code Fix Quality	✅ N/A
19	IPC Wire Compatibility	✅ N/A — this is HTTP REST, not IPC wire format
20	Build Infrastructure & Dependencies	✅ N/A
21	Scope & PR Discipline	✅ LGTM — single-concern PR, references #10558
22	PowerShell Scripting Hygiene	✅ N/A

Overall: Clean PR. One NIT-level style suggestion posted inline. No blocking or major issues found.

[github-actions]

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 105.7 AIC · ⌖ 2.97 AIC · ⊞ 16.9K · ◷

[Copilot]

Pull request overview

Routes retry artifacts to their Azure DevOps sub-results using server-assigned IDs while preserving parent attachment behavior.

Changes:

• Captures sub-result IDs from create/update responses.
• Uploads retry attachments with testSubResultId.
• Adds API tracking and tests for attachment routing and response failures.

Show a summary per file

File	Description
AzureDevOpsLivePublishingTests.cs	Tests sub-result attachment routing and response handling.
InternalAPI.Unshipped.txt	Tracks internal API changes.
IAzureDevOpsTestResultsClient.cs	Extends result and attachment contracts.
AzureDevOpsTestResultsPublisher.Flush.cs	Routes attachments to resolved sub-results.
AzureDevOpsTestResultsClient.cs	Parses IDs and builds attachment requests.
AzureDevOpsResultIdStore.cs	Exposes first-attempt construction.
AzureDevOpsLivePublishingModels.cs	Adds the published-result ID model.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (1)

src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsTestResultsClient.cs:166

• The PATCH path has the same accepted-response gap: an invalid/unsupported response charset throws InvalidOperationException or ArgumentException outside this filter. TryUpdateResultsAsync then forgets the accepted mapping and requeues the attempt, which can turn the next pass into a duplicate create. Return null for these decoding failures as well.

        catch (Exception ex) when (ex is JsonException or HttpRequestException or IOException)

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

🧪 Expert test review — PR #10723

No new or modified test methods were identified in the changed regions of this PR. Nothing to review.

_{Re-run with /review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 40.9 AIC · ⌖ 2.41 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsTestResultsClient.cs:296

• PATCH responses are still paired with submitted updates by array position. Each update already has a stable parent Id, so if Azure DevOps returns the same results in a different order, this comparison rejects the otherwise valid response; TryUpdateResultsAsync then cannot resolve any sub-result IDs and drops the batch's retry attachments. Map PATCH response entries by parent ID instead (while retaining the existing create validation), and cover a batch whose parent results are returned in reverse order.

            PublishedTestResult published = parsed.Value[i];
            AzureDevOpsTestCaseResult submitted = submittedResults[i];
            if (published.Id <= 0
                || (validateAutomatedTestName
                    ? !string.Equals(published.AutomatedTestName, submitted.AutomatedTestName, StringComparison.Ordinal)
                    : submitted.Id != published.Id))

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

🧵 Parallel-safety audit — PR #10723

Parallelization — audited assembly for this PR:

Test assembly	Scope	Workers	Analyzer coverage
Microsoft.Testing.Extensions.UnitTests	MethodLevel ([assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)] in Program.cs, unchanged by this PR)	CPU count	coverable once the parallel-safety analyzers ship (attribute opt-in)

Nothing audited here touches process-global state, shared filesystem paths, or [ResourceLock] / [DoNotParallelize] declarations. Nothing to flag for parallel-safety.

What was reviewed: the PR only changes test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureDevOpsLivePublishingTests.cs (493 additions / 21 deletions), plus five src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/* files. The test-file diff:

• Renames/extends several [TestMethod]s and adds new ones (sub-result ID plumbing, cancellation-during-response-read, charset/response-read-failure edge cases, HttpContent disposal, attachment targeting).
• Adds two new private nested test-doubles (ThrowingHttpContent, BlockingHttpContent/BlockingReadStream) used only as per-invocation HttpContent implementations passed to a fresh HttpResponseMessage per test — no shared/static state.
• Continues to use the existing CreateTestDirectory() helper, which allocates a Guid.NewGuid()-named directory under the temp path per call (pre-existing, unchanged) — no path collisions.
• Environment access goes through mocked IEnvironment (Moq), not the real Environment.SetEnvironmentVariable/CurrentDirectory; CultureInfo.InvariantCulture is only read as a format-provider value, never mutated.
• No [ResourceLock], [DoNotParallelize], [TestInitialize]/[TestCleanup]/[ClassInitialize] changes in the diff; the existing [TestCleanup] only deletes per-test tracked directories (_directoriesToDelete, an instance field).
• No .runsettings, testconfig.json, .csproj/.props/.targets, or assembly-level [Parallelize]/[DoNotParallelize] files were touched by this PR — parallelization configuration is unchanged.

No under-declared global-state mutations, no shared/relative-path collisions, no [ResourceLock] key mismatches, and no unnecessary serialization were introduced.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 107.7 AIC · ⌖ 3.32 AIC · ⊞ 24.8K · [◷]( · ◷)

[Copilot]

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced