Cache generated acceptance test builds

[Amaury Levé (Evangelink)]

Most acceptance tests regenerate and rebuild the same temporary projects on every PR. This extends the existing MSBuildCache rollout so those generated assemblies can be reused across builds instead of being compiled repeatedly.

Approach

• Give fixture-generated assets stable content-based workspace identities so cache fingerprints remain reusable.
• Build reflection and source-generation variants through full Visual Studio MSBuild with isolated cache roots and portable NuGet package paths.
• Seed generated-asset entries from trusted main builds and consume them read-only during PR validation.
• Keep fork/tokenless runs on the existing path, and clean partial cache outputs before a non-incremental dotnet build fallback if cache infrastructure fails.
• Share the MSBuildCache package configuration between the repository graph and generated projects.

Validation

• Rebuilt MSTest.Acceptance.IntegrationTests in Release with no warnings or errors.
• Ran AssemblyCleanupShouldRunAfterAllClassCleanupsHaveCompleted through cache seed and read-only consumption.
• Confirmed 4/4 project-cache hits for both reflection and source-generated variants.
• Reduced the sampled acceptance run from 42.6 seconds to 23.3 seconds.
• Verified the forced cache-failure path still passes through the clean fallback build.

[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.

Review Summary — Cache generated acceptance test builds

Clean infrastructure PR that extends MSBuildCache to acceptance test fixtures, giving them stable content-based identities and a seed/consume pipeline between main and PR builds.

Verdict Table

#	Dimension	Verdict
1	Algorithmic Correctness	✅ N/A — refactor + new infrastructure; logic is straightforward. Fallback path correctly cleans partial outputs before --no-incremental rebuild.
2	Threading & Concurrency	✅ s_cacheBinlogCounter correctly uses Interlocked.Increment. _tempDirectory is written once in InitializeAsync before any concurrent read.
3	Security & IPC Contract Safety	✅ No deserialization, no user-facing IPC changes. Cache paths are constructed from controlled env vars and SHA256 hashes.
4	Public API & Binary Compatibility	✅ No public API changes. CreateEnvironmentVariables and CreateStable are internal.
5	Performance & Allocations	✅ N/A — test infrastructure, not hot path. The whole purpose is to improve build performance.
6	Cross-TFM Compatibility	✅ Test infra targets $(NetCurrent) only; OperatingSystem.IsWindows() is available.
7	Resource & IDisposable Management	✅ _tempDirectory?.Dispose() handles the nullable change. CommandLine is using-disposed.
8	Defensive Coding at Boundaries	✅ GetRequiredEnvironmentVariable throws clearly. Fork/tokenless runs gracefully fall back to null (no caching). Cache failures fall back to dotnet build.
9	Error Handling & Diagnostics	✅ Cache failure logs stdout/stderr to Console.WriteLine before fallback.
10	Naming & Code Clarity	✅ Clear names: CacheConfiguration, AssetKey, CleanCacheOutputs, ResolveEntryProject.
11	Style & Formatting	✅ Consistent with codebase conventions.
12	Documentation & Comments	✅ Inline comments explain why (e.g., NUGET_PACKAGES override rationale).
13	Test Coverage	✅ N/A — this is test infrastructure itself; validation described in PR body.
14	Localization	✅ N/A — no user-facing strings.
15	Dependency Management	✅ MSBuildCache props extracted to shared file, reused by both repo and generated projects.
16	CI/CD & Pipeline	✅ New acceptanceTestCacheMode parameter wired correctly (write on seed, read on PR).
17	Configuration & Build	✅ eng/MSBuildCache.props is clean, conditions preserved from original.
18	Logging & Telemetry	✅ N/A
19	Backward Compatibility	✅ Default acceptanceTestCacheMode: disabled preserves existing behavior.
20	Edge Cases	✅ Handles: no token (fork PRs), non-Windows, cache failure, multiple/zero entry projects.
21	Scope Discipline	✅ Single concern: acceptance build caching.
22	PowerShell & Scripting	✅ N/A — no .ps1 changes.

Overall: No actionable findings. The change is well-structured with appropriate fallbacks and guard rails.

[Copilot]

Pull request overview

Adds MSBuildCache support for generated acceptance-test assets.

Changes:

• Introduces stable asset identities and cached MSBuild execution with fallback cleanup.
• Shares MSBuildCache package configuration.
• Seeds caches on main and reads them during PR validation.

Show a summary per file

File	Description
TestAssetFixtureBase.cs	Adds cached asset builds and fallback logic.
TempDirectory.cs	Adds stable workspaces and cache dependencies.
DotnetCli.cs	Extracts environment preparation.
test-windows-configuration-tests.yml	Configures acceptance cache environment.
eng/MSBuildCache.props	Centralizes cache package properties.
Directory.Packages.props	Imports shared cache configuration.
azure-pipelines.yml	Enables cache seeding and consumption.

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: 1
• Review effort level: Balanced

[github-actions]

🧪 Expert test review — PR #10811

No new or modified test methods were identified in the changed regions of this PR. The changed files (Directory.Packages.props, azure-pipelines.yml, eng/MSBuildCache.props, eng/pipelines/steps/test-windows-configuration-tests.yml, and three files under test/Utilities/Microsoft.Testing.TestInfrastructure/) are build/CI configuration and shared test-infrastructure utilities (DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs) — none contain [TestMethod]/[Fact]/[Theory]/etc. test methods. 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 · 50.2 AIC · ⌖ 1.08 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• 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 #10811

No new or modified test methods were identified in the changed regions
of this PR. The changed files (Directory.Packages.props, azure-pipelines.yml,
eng/MSBuildCache.props, eng/pipelines/steps/test-windows-configuration-tests.yml,
and helper classes DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs
under test/Utilities/Microsoft.Testing.TestInfrastructure/) are build-caching
infrastructure and test-asset helper utilities, not [TestMethod]/[Fact]-style
test methods. 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 · 53.4 AIC · ⌖ 1.12 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Review details

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

[github-actions]

🧪 Expert test review — PR #10811

No new or modified test methods were identified in the changed regions of this PR. The changes touch test infrastructure/helpers (DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs, and the AssemblyInitialize/AssemblyCleanup methods in AcceptanceFixture.cs) and CI pipeline/props files — none are [TestMethod]/[Fact]/etc. test methods. 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 · 68.8 AIC · ⌖ 1.07 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: None

Suppressed comments (1)

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

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/Helpers/AcceptanceFixture.cs:25

• The pipeline sets the cache root even when caching is disabled and on tokenless/fork runs, while GetCacheConfiguration falls back to the original build path in those cases. This check still switches both acceptance suites to a stable NuGet folder, defeating the randomized same-version-package protection described above and the PR's tokenless fallback contract. Gate the stable package root on the same active mode and token conditions.

        string? acceptanceCacheRoot = Environment.GetEnvironmentVariable("TESTFX_ACCEPTANCE_MSBUILD_CACHE_ROOT");
        string nugetCache = !string.IsNullOrEmpty(acceptanceCacheRoot)
                ? Path.Combine(acceptanceCacheRoot, "NuGetPackages")
                : Path.Combine(s_directoryToCleanup, ".packages");

[github-actions]

🧪 Expert test review — PR #10811

No new or modified test methods were identified in the changed regions of this PR. The four changed files under test/ (AcceptanceFixture.cs, DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs) are all test infrastructure/fixture code — DotnetCli/TempDirectory/TestAssetFixtureBase are shared helper utilities, and the only attributed members touched in AcceptanceFixture.cs are [AssemblyInitialize]/[AssemblyCleanup], which are excluded from grading per the review rubric. 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 · 125.8 AIC · ⌖ 1.08 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	test/​Utilities/​Microsoft.Testing.TestInfrastructure/​TestAssetFixtureBase.cs — This using declaration holds the DotnetCli slot until BuildAssetAsync exits. On a cache…

[github-actions]

🧪 Expert test review — PR #10811

No new or modified test methods were identified in the changed regions of this PR. All changed files are test infrastructure/helper utilities (DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs, AcceptanceFixture.cs) and build/pipeline configuration files (Directory.Packages.props, azure-pipelines.yml, eng/MSBuildCache.props, eng/pipelines/steps/test-windows-configuration-tests.yml) — none contain [TestMethod], [Fact], or other test-attributed methods. 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 · 41.6 AIC · ⌖ 1.08 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: None

Issues resolved since last review (1)

Severity	Finding
	test/​Utilities/​Microsoft.Testing.TestInfrastructure/​TestAssetFixtureBase.cs — This using declaration holds the DotnetCli slot until BuildAssetAsync exits. On a cache… View resolved comment

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: None

[github-actions]

🧵 Parallel-safety audit — PR #10811

Parallelization — assemblies whose test infrastructure this PR touched:

Test assembly	Scope	Workers	Analyzer coverage
Microsoft.Testing.Platform.Acceptance.IntegrationTests	MethodLevel ([assembly: Parallelize] in Program.cs)	CPU count (Workers = 0)	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
MSTest.Acceptance.IntegrationTests	MethodLevel ([assembly: Parallelize] in Program.cs)	CPU count (Workers = 0)	coverable once the parallel-safety analyzers ship (attribute-based opt-in)

Both assemblies consume the shared TestAssetFixtureBase / TempDirectory / DotnetCli infrastructure this PR changes, so both were audited. No src/ files or .runsettings/testconfig.json/Directory.Build.* parallelization state changed in this PR — this diff is confined to test/Utilities/Microsoft.Testing.TestInfrastructure (TempDirectory.cs, TestAssetFixtureBase.cs, DotnetCli.cs) plus one [AssemblyInitialize] in AcceptanceFixture.cs.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 2.

Summary: this PR adds opt-in MSBuild-cache-backed asset builds for acceptance tests. It introduces a genuinely new shared, stable, named temp directory per test-asset fixture (TempDirectory.CreateStable, keyed by a SHA-256 hash of assembly/type/assetId/assetCode) instead of the previous always-unique random directory — that is exactly the kind of change that normally creates a category-B cross-test path collision. The PR correctly pairs it with a process-wide exclusive FileStream lock (AcquireStableDirectoryLock, FileShare.None, acquired before the directory is deleted/recreated and released in Dispose/on construction failure) scoped to that same key, so two fixtures that legitimately race for the same stable directory serialize instead of colliding. I could not find a code path that reads or mutates that stable directory without going through the lock-holding constructor, so I am not raising this as a finding — it reads as a deliberate fix, not a gap.

---

Info

• [A · Medium confidence] test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/Helpers/AcceptanceFixture.cs:29-31 — [AssemblyInitialize] now conditionally points the process-global NUGET_PACKAGES env var at a stable, cache-mode-shared path instead of always deriving it from a fresh per-run random directory. [AssemblyInitialize] is already semaphore-serialized one-time setup (not a concurrent race per this audit's rules), and the mutation pattern (Environment.SetEnvironmentVariable("NUGET_PACKAGES", ...)) is unchanged from before this PR — only the computed value differs. Flagging for visibility only: when TESTFX_ACCEPTANCE_MSBUILD_CACHE_MODE is read/write, every test class in the assembly now restores into one shared NuGet global-packages folder concurrently. NuGet's global-packages-folder restore already tolerates concurrent multi-process access, so no fix is required; call out only if a future change adds writes to that folder outside NuGet's own restore path.
• [D · Low confidence] test/Utilities/Microsoft.Testing.TestInfrastructure/TempDirectory.cs:299-322 (AcquireStableDirectoryLock) — the lock is held for the fixture's full lifetime (constructor to Dispose), not just around the delete/recreate step, so two fixtures that hash to the same stable key (extremely unlikely — different assetId/assetCode/type per test class) would serialize their entire build+test lifetime rather than just directory setup. Unquantified: no evidence this key collision occurs in practice, so no throughput action recommended.

_{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 · 133.6 AIC · ⌖ 0.89 AIC · ⊞ 24.8K · [◷]( · ◷)

[github-actions]

🧪 Expert test review — PR #10811

No new or modified test methods were identified in the changed regions of this PR. The changed files under test/ (AcceptanceFixture.cs, DotnetCli.cs, TempDirectory.cs, TestAssetFixtureBase.cs) are test infrastructure/helper classes, not test methods, and the rest of the PR touches build configuration (Directory.Packages.props, azure-pipelines.yml, eng/MSBuildCache.props, eng/pipelines/steps/test-windows-configuration-tests.yml). 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 · 68.1 AIC · ⌖ 1.11 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

Review tier: Balanced
Findings: None