[perf-improver] perf: single-pass PropertyBag walk in AzureDevOps extension

[Evangelink]

Goal and Rationale

The AzureDevOps reporting extension was performing multiple independent O(n) linked-list traversals over PropertyBag per test result, plus heap-allocating LINQ iterators. This adds up at scale — every test result in a large run triggers these allocations.

Approach

Replace per-property SingleOrDefault<T>() / OfType<T>().FirstOrDefault() calls with a single GetStructEnumerator() pass (zero-allocation struct enumerator already used throughout the platform). Two hot paths updated:

AzureDevOpsSummaryReporter.ConsumeAsync (called for every terminal test result):

• Before: 1× GetFullyQualifiedName() → OfType<SerializableKeyValuePairStringProperty>().FirstOrDefault() (O(n) + LINQ heap alloc) + 1× GetDuration() → SingleOrDefault<TimingProperty>() (O(n))
• After: single GetStructEnumerator() loop; both private helper methods removed

AzureDevOpsTestResultsPublisher.BuildAttachmentsFromTestNode (called for every failing test):

• Before: OfType<FileArtifactProperty>() (O(n) + LINQ heap alloc) + SingleOrDefault<StandardOutputProperty>() (O(n)) + SingleOrDefault<StandardErrorProperty>() (O(n))
• After: single GetStructEnumerator() loop

Performance Evidence

PropertyBag uses a singly-linked list as its backing store; each SingleOrDefault<T> call walks the entire list. In a 1000-test run (all passed/failed) with a typical PropertyBag of ~5–10 entries:

Location	Before	After	Savings
ConsumeAsync per-result	2 O(n) walks + 1 LINQ alloc	1 O(n) walk	~50% traversal work, 1 GC alloc
BuildAttachmentsFromTestNode per-failure	3 O(n) walks + 1 LINQ alloc	1 O(n) walk	~67% traversal work, 1 GC alloc

Micro-benchmarks are not included here; the pattern is well-established and the savings are proportional to test count. The biggest gains come from eliminating the OfType<> allocations which show up as GC pressure in heap profiles.

Trade-offs

• Slightly more verbose code (switch statement instead of named helpers), but follows the existing pattern used in TrxTestResultExtractor, CtrfReport, JUnitReport, HtmlReport, DotnetTestDataConsumer, and OpenTelemetryResultHandler.
• No behavioral change — same properties are read, same outputs produced.

Reproducibility

# Build
./build.sh -build -c Debug

# Test (511 pass, 11 skipped Windows-only)
artifacts/bin/Microsoft.Testing.Extensions.UnitTests/Debug/net8.0/Microsoft.Testing.Extensions.UnitTests

Test Status

✅ Build succeeded (0 warnings, 0 errors)
✅ Microsoft.Testing.Extensions.UnitTests: 500 passed, 11 skipped (Windows-only), 0 failed

🤖 Automated content 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 Perf Improver workflow. · 2K AIC · ⌖ 25.1 AIC · [◷]( · ◷)

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/perf-improver.md@main

[Copilot]

Pull request overview

This PR optimizes hot paths in the Azure DevOps reporting extension by replacing multiple per-result PropertyBag LINQ queries / repeated SingleOrDefault<T>() traversals with a single GetStructEnumerator() pass, reducing linked-list walks and avoiding iterator allocations.

Changes:

• Update AzureDevOpsSummaryReporter.ConsumeAsync to extract TimingProperty and the fully-qualified-name property via a single PropertyBag enumeration.
• Update AzureDevOpsTestResultsPublisher.BuildAttachmentsFromTestNode to collect file attachments, stdout, and stderr in one pass over the PropertyBag.

Show a summary per file

File	Description
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsTestResultsPublisher.ResultFactory.cs	Switch from LINQ-based property extraction to a single struct-enumerator walk when building attachments.
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsSummaryReporter.cs	Switch from helper methods (LINQ + SingleOrDefault<T>()) to a single struct-enumerator walk for duration and FQN extraction.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

[Evangelink]

Fixed in def518c -- TimingProperty case now throws on duplicates (matching PropertyBag.SingleOrDefault<T>), and the FQN SerializableKeyValuePairStringProperty lookup now keeps first-wins semantics (matching the prior OfType<>().FirstOrDefault() behaviour) instead of silently overwriting earlier values. Review reply handled.

[Evangelink]

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.

Review Summary

The optimization direction is sound — replacing multiple LINQ/OfType<T>() traversals with a single GetStructEnumerator() pass aligns with the established pattern already used in TrxReport, CtrfReport, JUnitReport, and HtmlReport. However, both hot paths are missing the GetSingleOrDefaultValue helper that every prior conversion in this codebase explicitly includes to preserve the throw-on-duplicate contract of SingleOrDefault<T>().

---

MAJOR: Missing GetSingleOrDefaultValue pattern (both files)

The four existing GetStructEnumerator() conversions in this repo each contain an identical local helper and a comment documenting exactly why it exists:

// "Singleton-typed properties use the local GetSingleOrDefaultValue helper to preserve
// the throw-on-duplicate invariant that SingleOrDefault<T>() provided."
static TProperty GetSingleOrDefaultValue<TProperty>(TProperty? existingProperty, TProperty property)
    where TProperty : class, IProperty
    => existingProperty is not null
        ? throw new InvalidOperationException($"Found multiple properties of type '{typeof(TProperty)}'.")
        : property;

This PR omits the helper for TimingProperty (in ConsumeAsync) and for StandardOutputProperty / StandardErrorProperty (in BuildAttachmentsFromTestNode). PropertyBag.Add() only rejects duplicate references, not duplicate types, so two distinct TimingProperty (or StandardOutputProperty) objects can coexist in a bag. When that happens, the old code surfaced an InvalidOperationException; the new code silently uses whichever match was enumerated last — a hard-to-diagnose behavioral regression. See inline comments for specific lines and the recommended one-liner fix.

---

NIT: FQN lookup silently changes first-match → last-match

GetFullyQualifiedName() previously used OfType<SerializableKeyValuePairStringProperty>().FirstOrDefault(predicate), which returns the first match in enumeration order (head of the linked list = most recently added). The new switch overwrites on every match and therefore keeps the last match (first added / tail). If a node ever carries multiple SerializableKeyValuePairStringProperty entries with the same FQN key, the result silently flips. This property was not guarded by SingleOrDefault, so the risk is lower, but the behavioral change is worth noting for completeness.

---

Dimension Scorecard (22 of 22 assessed)

#	Dimension	Status
1	Algorithmic Correctness	❌ MAJOR — missing GetSingleOrDefaultValue in both hot paths
2	Threading & Concurrency	✅ LGTM — enumerator is a stack-local struct; _stateLock guards _records correctly
3	Security & IPC Contract Safety	✅ LGTM
4	Public API & Binary Compatibility	✅ LGTM — all changed members are private/internal
5	Performance & Allocations	✅ LGTM — single-pass avoids one array allocation and one linked-list traversal per call
6	Cross-TFM Compatibility	✅ LGTM — GetStructEnumerator() and PropertyBagEnumerator are available on all TFMs
7	Resource & IDisposable Management	✅ LGTM — PropertyBagEnumerator.Dispose() only nulls references; not disposing follows the codebase-wide pattern for this type
8	Defensive Coding at Boundaries	✅ LGTM — exception filter in FileArtifactProperty case is correctly narrowed
9	Localization & Resources	N/A
10	Test Isolation	N/A
11	Assertion Quality	N/A
12	Flakiness Patterns	N/A
13	Test Completeness & Coverage	✅ LGTM — pure refactor; existing unit tests cover the changed paths
14	Data-Driven Test Coverage	N/A
15	Code Structure & Simplification	✅ LGTM — switch-over-enumerator is idiomatic and matches the platform convention
16	Naming & Conventions	✅ LGTM
17	Documentation Accuracy	⚠️ NIT — comments say "1 LINQ allocation" but the actual allocation is the TProperty[] array returned by PropertyBag.OfType<T>(); LINQ's FirstOrDefault on the resulting array is not separately allocating
18	Analyzer & Code Fix Quality	N/A
19	IPC Wire Compatibility	N/A
20	Build Infrastructure & Dependencies	✅ LGTM
21	Scope & PR Discipline	✅ LGTM — focused change, good PR description
22	PowerShell Scripting Hygiene	N/A

15/22 dimensions clean · 5 N/A · 1 MAJOR · 1 NIT

The fix is mechanical: add the four-line GetSingleOrDefaultValue local function and route timing, stdout, and stderr through it — exactly as the four sibling extension files already do.

🤖 Automated content 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 (on PR ready) workflow. · 544.5 AIC · ⌖ 12.7 AIC · ◷

[Evangelink]

[MAJOR] Algorithmic Correctness — same GetSingleOrDefaultValue gap for stdout / stderr

Same issue as in ConsumeAsync: stdout = so and stderr = se silently overwrite if multiple instances exist in the bag. The prior SingleOrDefault<StandardOutputProperty>() and SingleOrDefault<StandardErrorProperty>() calls would have thrown. CTRF, JUnit, and HTML all guard these exact properties with the helper:

case StandardOutputProperty so: stdout = GetSingleOrDefaultValue(stdout, so); break;
case StandardErrorProperty se:  stderr = GetSingleOrDefaultValue(stderr, se); break;

Please apply the same fix here.

[Evangelink]

Refactored both files to use the GetSingleOrDefaultValue local helper pattern that matches TRX/CTRF/JUnit/HTML extensions. Same throw-on-duplicate invariant, more consistent code. Pushed in 07acf07.

[Evangelink]

[MAJOR] Algorithmic Correctness — silent last-wins silently replaces throw-on-duplicate

Every other GetStructEnumerator() migration in this codebase (TRX, CTRF, JUnit, HTML) explicitly adds a local GetSingleOrDefaultValue helper to preserve the InvalidOperationException that SingleOrDefault<T>() raised on duplicate instances. The pattern and its rationale are even spelled out in the doc comments of each of those files:

// From TrxTestResultExtractor.cs, CtrfReport/TestResultCapture.cs, JUnitReport/TestResultCapture.cs, HtmlReport/TestResultCapture.cs:
// "Singleton-typed properties use the local GetSingleOrDefaultValue helper to preserve
// the throw-on-duplicate invariant that SingleOrDefault<T>() provided."
static TProperty GetSingleOrDefaultValue<TProperty>(TProperty? existingProperty, TProperty property)
    where TProperty : class, IProperty
    => existingProperty is not null
        ? throw new InvalidOperationException($"Found multiple properties of type '{typeof(TProperty)}'.")
        : property;

// usage:
case TimingProperty t: timing = GetSingleOrDefaultValue(timing, t); break;

As written, timing = t silently overwrites on each TimingProperty match. PropertyBag.Add() only rejects duplicate references, not duplicate types, so two distinct TimingProperty instances can coexist in a bag. If that happens, the old code raised a visible InvalidOperationException; the new code silently uses the last value found — a diagnostic regression that makes malformed property bags very difficult to debug.

Please add the GetSingleOrDefaultValue local function and route timing through it.

[Evangelink]

Same refactor applied here — GetSingleOrDefaultValue helper now used for StandardOutputProperty and StandardErrorProperty to match the convention. Pushed in 07acf07.

[Copilot]

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 0 new