[fix] Add filesystem integration tests for DotnetTestHostManager deps.json/runtimeconfig.dev.json parsing

[nohwnd]

🤖 This is an automated fix for issue #15589.

Root Cause

During the STJ migration, DependencyContextJsonReader was replaced with a custom DepsJsonParser (on .NET Framework) and JsonDocument.Parse was replaced with Jsonite-based parsing for runtimeconfig.dev.json. When these replacements were made, no integration tests failed, proving the actual file-reading code paths were not exercised by any test — only mocked.

What the fix does

Adds DotnetTestHostManagerFilesystemIntegrationTests.cs with 4 tests that use real FileHelper (no mocking of the file system) and actual files on disk:

1. FindsTestHostDllViaRealDepsJsonAndRuntimeConfigDevJson — Creates a real deps.json pointing to testhost.dll in a nuget-like layout and a real runtimeconfig.dev.json with additionalProbingPaths. Verifies GetTestHostProcessStartInfo correctly parses both files and resolves the testhost path. This test would fail if DepsJsonParser or the Jsonite runtimeconfig.dev.json parser is broken.

2. PassesDepsFileArgWhenDepsJsonExists — Verifies --depsfile is included in testhost launch args when a real .deps.json exists on disk.

3. PassesRuntimeConfigArgWhenRuntimeConfigExists — Verifies --runtimeconfig is included when a real .runtimeconfig.json exists on disk.

4. FindsTestHostDllNextToSourceWhenNoRuntimeConfigDev — Verifies the fallback path (testhost.dll next to source dll) works when no runtimeconfig.dev.json is present.

Tests

All 4 new tests pass on both net11.0 and net481 target frameworks.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

Adds filesystem-backed tests for DotnetTestHostManager to cover real .deps.json, .runtimeconfig.json, and .runtimeconfig.dev.json handling paths that were previously only mocked.

Changes:

• Adds four tests that create temporary test assemblies/config files and use the real FileHelper.
• Verifies testhost discovery through deps/runtimeconfig.dev probing and fallback discovery next to the source DLL.
• Verifies launch arguments include --depsfile and --runtimeconfig when corresponding files exist.

[nohwnd]

Review: filesystem integration tests for DotnetTestHostManager

Activated dimensions (test/TestHostProvider path): Testhost Assembly Loading & Resolution, Process Architecture & Host Resolution, Cross-TFM & Framework Resolution, Acceptance Test Coverage Design.

What looks good

• Real FileHelper used (no filesystem mock) — correctly exercises DepsJsonParser / DependencyContextJsonReader code paths.
• TearDown cleans up via Directory.Delete(recursive: true) — no leaked temp dirs.
• Guid.NewGuid() temp dir per run — tests are isolated and parallelism-safe.
• Assert.Contains(needle, haystack) calls are all correctly ordered (needle first).
• JSON escaping of packagesDir on Windows (Replace("\\", "\\\\")) is correct.

One issue

_mockDotnetHostHelper (line 43) — declared but never consumed. CreateManager instantiates a real DotnetHostHelper and this mock is never passed in. See inline comment.

Coverage note (non-blocking)

All four tests unconditionally mock PlatformOperatingSystem.Unix, so the Windows-specific testhost resolution path (.exe selection, Windows registry lookup) is not exercised. This is within the stated scope of the PR (guarding DepsJsonParser / runtimeconfig parsing), but a follow-up test covering the Windows platform branch would give fuller coverage.

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Dead field — _mockDotnetHostHelper is declared here but never referenced. CreateManager constructs a real DotnetHostHelper directly and this mock is never passed in. Either remove the field or wire it in if it was intended to allow mocking the dotnet host helper in future tests.

// Remove this line — it is unused:
private readonly Mock<IDotnetHostHelper> _mockDotnetHostHelper = new();

[nohwnd]

Done — removed the unused _mockDotnetHostHelper field.

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Commit pushed: 4f8995e

🔧 Iterated by PR Iteration Agent 🔧

[Evangelink]

🤖 Expert vstest review (automated)

Test-only PR that adds 4 integration tests in test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerFilesystemIntegrationTests.cs. The change is additive, no production code touched, and the gap it fills is real — the STJ migration silently replaced DependencyContextJsonReader with DepsJsonParser and JsonDocument.Parse with Jsonite for runtimeconfig.dev.json and nothing caught it because every existing test mocked IFileHelper. I built and ran the tests on net11.0 and net481; all 4 pass on both TFMs, exercising the DependencyContextJsonReader/JsonDocument branch on .NET and the DepsJsonParser/Jsonite branch on .NET Framework. No blockers — looks good to me with a couple of small suggestions below.

🛑 Blockers

None.

⚠️ Issues

None that affect correctness or compatibility.

💡 Suggestions

1. Test 1 assertion is weaker than it should be — DotnetTestHostManagerFilesystemIntegrationTests.cs:140

Assert.Contains("microsoft.testplatform.testhost", startInfo.Arguments);

Substring search for the package name is fine for guarding against a total parse-failure regression, but it does not prove that the probing-path resolution actually picked the file you placed under packagesDir. If the resolver returned some other path that happened to contain the package name, this still passes. Since you already computed testhostFullPath in the arrange, assert on that directly:

Assert.Contains(testhostFullPath, startInfo.Arguments);

That turns this into a real end-to-end check of DepsJsonParser + additionalProbingPaths rather than a substring check.

2. Test 4 name overstates what it covers — DotnetTestHostManagerFilesystemIntegrationTests.cs:213

public void GetTestHostProcessStartInfo_FindsTestHostDllNextToSourceWhenNoRuntimeConfigDev()

The arrange writes only TestProject.dll and testhost.dll. There is no deps.json either, so GetTestHostPath short-circuits at the _fileHelper.Exists(depsFilePath) check on DotnetTestHostManager.cs:898 and never reaches the runtimeconfig.dev.json branch. The test does exercise the "testhost next to source" fallback, but the name implies a deps.json-present-but-no-runtimeconfig.dev.json scenario which is actually the more interesting branch (else at line 1012). Consider either renaming to FindsTestHostDllNextToSourceWhenNoDepsJson or writing the deps.json-only variant.

3. TearDown can flake on Windows — DotnetTestHostManagerFilesystemIntegrationTests.cs:62-68

[TestCleanup]
public void TearDown()
{
    if (Directory.Exists(_tempDir))
    {
        Directory.Delete(_tempDir, recursive: true);
    }
}

Directory.Delete(..., true) on Windows intermittently throws IOException/UnauthorizedAccessException due to AV/indexer/file-lock races and would mask the actual test result. Wrap in try/catch (or use a _ = Try(() => Directory.Delete(...)) helper) so cleanup failures don't fail the test:

[TestCleanup]
public void TearDown()
{
    try
    {
        if (Directory.Exists(_tempDir))
        {
            Directory.Delete(_tempDir, recursive: true);
        }
    }
    catch
    {
        // Best-effort cleanup; do not fail the test on a leaked temp dir.
    }
}

4. Asymmetry vs. existing test class — naming convention in DotnetTestHostManagerTests.cs is MethodShouldDoX (e.g. GetTestHostProcessStartInfoShouldInvokeDotnetCommandline); these new tests use Method_Scenario. Not worth blocking on, but consistency helps grep. Nit.

5. Replace("/", "/") no-op — DotnetTestHostManagerFilesystemIntegrationTests.cs:120

"{{packagesDir.Replace("\\", "\\\\").Replace("/", "/")}}"

The second Replace does nothing. If the intent was to JSON-escape forward slashes (\/), JSON doesn't require it; if the intent was a placeholder, drop it. Cosmetic.

6. Consider asserting parse correctness directly on DepsJsonParser — there is already DepsJsonParserTests.cs next door; the deps.json fixture used here is also a candidate for a parser-level unit test that would localize regressions to the parser vs. the manager. Out of scope for this PR but worth a follow-up if DepsJsonParser grows.

✅ Notes

• Verified locally: built Microsoft.TestPlatform.TestHostProvider.UnitTests for net11.0 and net481, both succeed; all 4 new tests pass on both TFMs.
• Correctly mocks PlatformOperatingSystem.Unix to skip the Windows-only testhost.exe branch, ensuring the deps.json/runtimeconfig.dev.json code path is the one exercised regardless of CI OS.
• Hits both parsing branches in DotnetTestHostManager.GetTestHostPath (#if NETCOREAPP DependencyContextJsonReader + JsonDocument.Parse and the #else DepsJsonParser + Jsonite.Json.Deserialize), which is exactly what the linked issue (DotnetTestHostManager runtimeconfig.dev.json and deps.json parsing not covered by integration tests #15589) asks for.
• Pure test additions — zero risk to production code, no public API surface change, no protocol/binding-redirect/dependency impact.

Verdict

APPROVE — addresses a real coverage gap with passing tests on all targeted TFMs. Suggestion (1) would meaningfully strengthen the assertion; the rest are nits.

[nohwnd]

Commit pushed: bba48a8

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Thanks for the review! I've addressed all the actionable suggestions:

1. Strengthened assertion — Test 1 now asserts Assert.Contains(testhostFullPath, startInfo.Arguments) instead of the substring check, making it a real end-to-end verification of DepsJsonParser + additionalProbingPaths resolution.
2. Renamed Test 4 — FindsTestHostDllNextToSourceWhenNoRuntimeConfigDev → FindsTestHostDllNextToSourceWhenNoDepsJson (more accurately describes what the arrange sets up).
3. Hardened TearDown — wrapped Directory.Delete in try/catch to avoid flaky failures from Windows AV/indexer locks.
4. Removed no-op .Replace("/", "/") from the runtimeconfig.dev.json template string.

🔧 Iterated by PR Iteration Agent 🔧

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.

[nohwnd]

Re-review after iteration

The previous finding (_mockDotnetHostHelper dead field) has been resolved — the field is no longer present in the current diff.

The code is clean:

• Assert.Contains(needle, haystack) ordering is correct in all four assertions
• Real FileHelper is used — the parsing code paths are genuinely exercised
• JSON escaping in the inline strings is correct (/ for deps.json library paths, \\\\ for the probing path in runtimeconfig.dev.json)
• TearDown cleans up temp dirs with best-effort catch — no leaks

No further issues found.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠