[perf-improver] perf: skip allocation in CaptureLifecycleProperties when no user properties set

[Evangelink]

Goal and rationale

CaptureLifecycleProperties() is called once per [AssemblyInitialize] / [ClassInitialize] execution to snapshot any TestContext.Properties entries set during init, so they can be propagated to each test's context. In the common case — where neither init method sets any properties — the method unconditionally allocated two objects:

1. new Dictionary<string, object?>(capacity) — the snapshot buffer
2. new ReadOnlyDictionary<string, object?>(snapshot) — the immutable wrapper

Those two objects are then stored in PostAssemblyInitProperties / PostClassInitProperties and passed to every test via MergeProperties. MergeProperties already returned early for null, but not for an empty dictionary, so it also acquired a lock per call before doing nothing.

Approach

• CaptureLifecycleProperties: change to lazy allocation. The Dictionary is allocated only on the first non-label entry. If the loop finds nothing to snapshot, the method returns null (return type changed from IReadOnlyDictionary<string,object?> to IReadOnlyDictionary<string,object?>?). This is a single-pass implementation — no extra iteration.
• MergeProperties: extend the null guard to null or { Count: 0 }, so an empty dict is also short-circuited cheaply (defensive, and matches the new null-return convention).
• No caller changes: PostAssemblyInitProperties and PostClassInitProperties are already declared as nullable; MergeProperties(null) already short-circuits.

Performance evidence

Every test class that has [AssemblyInitialize] or [ClassInitialize] (but doesn't write to TestContext.Properties in it) benefits:

Call site	Before	After
CaptureLifecycleProperties (empty)	2 heap allocs	0 allocs
MergeProperties(emptySnapshot) per test	lock acquire + empty loop	1 null-check, return
MergeProperties(null) per test	1 null-check, return	1 null/count-check, return

For a test suite with an [AssemblyInitialize] / [ClassInitialize] that sets no properties:

• −2 heap allocations at init time
• −2 lock acquisitions per test (one for PostAssemblyInitProperties, one for PostClassInitProperties merge in RunSingleTestAsync)

In a 1000-test suite, that's ~2000 lock acquisitions avoided.

Trade-offs

None significant. The change is backward-compatible: callers that store the snapshot in nullable fields already handle null; MergeProperties(null) was already a no-op.

Reproducibility

# Build and run unit tests for MSTestAdapter.PlatformServices
./build.sh -test -projects test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/MSTestAdapter.PlatformServices.UnitTests.csproj

# Or for just the new tests, after build:
dotnet run --project test/UnitTests/MSTestAdapter.PlatformServices.UnitTests -f net8.0 --no-build -- \
  --treenode-filter "/*/*/*/TestContextImplementationTests/CaptureLifecyclePropertiesShouldReturnNullWhenNoNonLabelPropertiesExist"

Test status

No SDK is available in this CI environment, so local build/test could not be run. The change is confined to the MSTestAdapter.PlatformServices project and its unit tests; all callers already handle null snapshots.

🤖 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. · 2.6K AIC · ⌖ 26.5 AIC · ⊞ 57.7K · [◷]( · ◷)

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

Optimizes MSTestAdapter.PlatformServices’ TestContextImplementation lifecycle property flow by avoiding allocations and locking when no user properties are set during AssemblyInitialize / ClassInitialize.

Changes:

• Make CaptureLifecycleProperties() lazily allocate and return null when there are no non-label properties to snapshot.
• Make MergeProperties() return early for null or empty dictionaries to avoid unnecessary locking.
• Add/adjust unit tests to validate null snapshot behavior and empty-merge short-circuit behavior.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs	Implements lazy snapshot allocation + nullable return, and adds empty-dictionary fast path in MergeProperties.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.cs	Updates snapshot tests for nullable return and adds a test for merging an empty dictionary.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestClassInfoTests.cs	Adds coverage that PostClassInitProperties stays null when class init sets no properties.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestAssemblyInfoTests.cs	Adds coverage that PostAssemblyInitProperties stays null when assembly init sets no properties.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Low

[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

Dimension	Verdict
Algorithmic Correctness	✅ LGTM
Threading & Concurrency	✅ LGTM
Security & IPC Contract Safety	✅ N/A
Public API & Binary Compatibility	✅ LGTM (CaptureLifecycleProperties is internal; both assignment sites already declare the field as nullable)
Performance & Allocations	✅ LGTM — core goal of the PR
Cross-TFM Compatibility	✅ LGTM (is null or { Count: 0 } is C# 9 OR-pattern; IReadOnlyDictionary<K,V> exposes Count via IReadOnlyCollection<T>)
Resource & IDisposable Management	✅ N/A
Defensive Coding	✅ LGTM
Localization & Resources	✅ N/A
Test Isolation	✅ LGTM (each test sets its own AssemblyInitializeMethodBody; TestClassInfoTests resets all statics in its constructor)
Assertion Quality	✅ LGTM (AwesomeAssertions .Should() style — correct for MSTestAdapter.PlatformServices.UnitTests)
Flakiness Patterns	✅ None detected
Test Completeness	✅ LGTM
Data-Driven Coverage	✅ N/A
Code Structure & Simplification	✅ LGTM
Naming & Conventions	✅ LGTM
Documentation Accuracy	⚠️ NIT — see inline comment
Analyzer & Code Fix Quality	✅ N/A
IPC Wire Compatibility	✅ N/A
Build Infrastructure	✅ N/A
Scope & PR Discipline	✅ Single concern, clean delta
PowerShell Scripting Hygiene	✅ N/A

---

Correctness walkthrough

CaptureLifecycleProperties lazy-init path

• The snapshot ??= new Dictionary<string, object?>(_properties.Count) fires only on the first non-label entry, inside the existing lock (_propertiesLock). Thread-safe ✅.
• The capacity hint _properties.Count is the total count including labels, so it may over-allocate by up to 2 slots when labels are present. This is the same trade-off as the original code (which also used _properties.Count before the loop), and the #pragma disable IDE0028 comment correctly documents the intent. ✅
• If no non-label entries are found, snapshot stays null and the method returns null — zero allocations, as advertised. ✅

MergeProperties empty-dict short-circuit

IReadOnlyDictionary<K,V> extends IReadOnlyCollection<KeyValuePair<K,V>>, which defines int Count { get; }, so the { Count: 0 } property pattern resolves correctly. Merging an empty dictionary had no observable side effect before (just a lock-acquire and a no-op loop), so short-circuiting is safe. ✅

Callers of CaptureLifecycleProperties

Both callers assign to nullable-typed properties:

• TestAssemblyInfo.PostAssemblyInitProperties: IReadOnlyDictionary<string, object?>? ✅
• TestClassInfo.PostClassInitProperties: IReadOnlyDictionary<string, object?>? ✅

MergeProperties(null) was already a no-op, so no call-site changes are required. ✅

Test discovery

All four new test files extend TestContainer (using TestFramework.ForTestingMSTest; public class Foo : TestContainer). The TestFramework.ForTestingMSTest framework discovers any public parameterless instance method as a test — no [TestMethod] attribute is needed. The new methods are all public and are correctly discoverable. ✅

---

One inline NIT on a misleading comment in CaptureLifecyclePropertiesShouldReturnNullWhenNoNonLabelPropertiesExist; no blocking issues found.

[Evangelink]

🧪 Test quality grade — PR #9478

7 tests graded (4 new, 3 modified) across 3 files. All receive A (90–100): clear AAA structure, fluent AwesomeAssertions with equality, collection-membership, negative, and explicit null-expectation assertions, and tightly focused single-behavior coverage. The three modified tests add NotBeNull() guard assertions ahead of value assertions — these are meaningful preconditions, not trivial null checks. No improvements are needed.

Δ	Test	Grade	Band	Notes
new	TestAssemblyInfoTests. RunAssemblyInitializeShouldLeavePostAssemblyInitPropertiesNullWhenAssemblyInitSetsNoProperties	A	90–100	Clear AAA; outcome + null-property assertions verify the new lazy-init contract.
new	TestClassInfoTests. GetResultOrRunClassInitializeAsyncShouldLeavePostClassInitPropertiesNullWhenClassInitSetsNoProperties	A	90–100	Mirror of the assembly-init test; uses nameof over a string literal for type safety.
mod	TestContextImplementationTests. CaptureLifecyclePropertiesShouldAliasReferenceTypeValues	A	90–100	No issues found.
mod	TestContextImplementationTests. CaptureLifecyclePropertiesShouldReturnAllPropertiesExceptPerContextLabels	A	90–100	Rich assertion set: equality, collection membership, and negative checks cover the full contract.
new	TestContextImplementationTests. CaptureLifecyclePropertiesShouldReturnNullWhenNoNonLabelPropertiesExist	A	90–100	BeNull() is the meaningful assertion: null is the explicitly expected return per the new contract.
mod	TestContextImplementationTests. CaptureLifecyclePropertiesShouldReturnSnapshotIndependentOfTheLiveBag	A	90–100	No issues found.
new	TestContextImplementationTests. MergePropertiesShouldIgnoreEmptyDictionary	A	90–100	Concise AAA; equality assertion verifies the no-op contract for empty-dictionary input.

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.}

🤖 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 Grade Tests on PR (on open / sync) workflow. · 262.5 AIC · ⌖ 13.5 AIC · ⊞ 43.7K · [◷]( · ◷)