From d96e64ccfdaa4d82a2a44e90c46ce9d14c350fb3 Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 11:57:45 +0300 Subject: [PATCH 1/4] Follow NuGet package references in the affected-test fallback Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 10 + README.md | 4 +- .../AffectedTestFinder.cs | 158 ++++++++++++ .../Mcp/Tools/TestingTools.cs | 146 ++++++++++- .../NuGetPackageEdgeTests.cs | 244 ++++++++++++++++++ 5 files changed, 558 insertions(+), 4 deletions(-) create mode 100644 tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c17088..caee602 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ All notable changes to DotNetDevMCP are documented here. The format follows that forwards the original `Origin`, or a non-browser client that happens to set one. `--http` still has no authentication or TLS and still shouldn't be exposed beyond localhost. +### Added +- `dotnet_test_affected`'s project fallback now follows NuGet package references, not just `ProjectReference`s: when a + test project's restored `obj/project.assets.json` references another solution project's package id (a literal + ``, one from the nearest `Directory.Build.props`, or the assembly name), that test project is selected + even with no `ProjectReference` between them. A change to only `Directory.Packages.props` is narrowed, via the same + assets data, to the test projects that use the package ids whose version actually moved (diffed against git), + instead of running the whole solution; the note explains why when it can't (git unavailable, the file doesn't + parse, or no restored test project uses those ids). Test projects with no `obj/project.assets.json` (not restored) + are called out in the note when the run falls back to whole test projects. + ## [0.3.3] - 2026-09-24 Prompted by an external evaluation; each claim was checked against the code first. diff --git a/README.md b/README.md index 624828c..704389d 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ Measured with BenchmarkDotNet on an i7-10750H, .NET 10.0.9. The orchestration be After an edit, the agent usually reruns the whole suite. `dotnet_test_affected` asks Roslyn instead: take the symbols declared in the changed files, follow references (up to `maxDepth` hops, default 8) until you land in a method with `[Fact]`, `[Theory]`, `[Test]`, `[TestCase]` or `[TestMethod]`, then run exactly those. Changed files default to the git working tree, or `gitBase: "main"` for a branch. `dryRun: true` lists the tests without running them; `framework: "net10.0"` runs one target framework of multi-targeted test projects. -The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). +The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). The project fallback also follows restored NuGet package references (a test project whose `obj/project.assets.json` references another solution project's package id, with no `ProjectReference` between them, is still selected), and a change to only `Directory.Packages.props` is narrowed, via that same assets data, to the test projects that actually use the package ids whose version moved, instead of running the whole solution. On this repository, editing `ConcurrentExecutor.cs` selects 22 of 44 tests (the `ConcurrentExecutorTests` plus the `OrchestrationServiceTests` that reach it through `OrchestrationService`). Measured through the MCP tool, build included, i7-10750H: @@ -195,7 +195,7 @@ Built on the official [MCP C# SDK](https://github.com/modelcontextprotocol/cshar 0.3.3. The Roslyn tools are mature (they come from SharpTools). Testing, build, git and orchestration are newer and have been exercised on this repository and a few others; expect rough edges on unusual project layouts. Issues and PRs welcome, see [CONTRIBUTING](https://github.com/csa7mdm/DotNetDevMCP/blob/main/CONTRIBUTING.md). -Known gaps: `dotnet_test_affected` follows C# references only (no reflection, no DI-by-convention, no string-keyed lookups), so a change reached only through those paths will not select the test; use `dryRun` to check what it picks. Calls through an interface or base class are followed. The project fallback follows `ProjectReference`s only: a test project that uses the changed code through a NuGet package is not found. Builds and tests are not sandboxed (see Security). Tests that hang instead of failing are only caught by a run that finishes. Test attribute detection covers xUnit, NUnit and MSTest by attribute name. Past the command-line length limit the filter widens from methods to classes, then to the whole project (more tests, never fewer). +Known gaps: `dotnet_test_affected` follows C# references only (no reflection, no DI-by-convention, no string-keyed lookups), so a change reached only through those paths will not select the test; use `dryRun` to check what it picks. Calls through an interface or base class are followed. The project fallback follows `ProjectReference`s and NuGet package references: package references are now followed when the test project has been restored; cross-repo consumers are not. Builds and tests are not sandboxed (see Security). Tests that hang instead of failing are only caught by a run that finishes. Test attribute detection covers xUnit, NUnit and MSTest by attribute name. Past the command-line length limit the filter widens from methods to classes, then to the whole project (more tests, never fewer). ## Credits and license diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index 1771886..ec4a3fe 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -1,6 +1,8 @@ // Copyright (c) 2025 Ahmed Mustafa using System.Collections.Immutable; +using System.Text.Json; +using System.Xml.Linq; using DotNetDevMCP.CodeIntelligence.Interfaces; using DotNetDevMCP.Core; using DotNetDevMCP.Core.Models; @@ -229,6 +231,7 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, list.Add(project.Id); } } + AddPackageEdges(solution, dependents); var reachable = new HashSet(changedProjectIds); var frontier = new Queue(changedProjectIds); @@ -242,6 +245,161 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); } + /// + /// Reverse edges from a solution project P to a test project that consumes P only through a restored NuGet + /// PackageReference - no Roslyn ProjectReference at all - discovered from the test project's + /// obj/project.assets.json. Added into the same reverse-edge map already + /// builds from ProjectReferences, so its BFS covers both kinds of edge without any change to the walk itself. + /// ponytail: this only follows package ids that match another SOLUTION project's resolved packageId. A test + /// project that depends on a third-party (non-solution) package is unaffected either way, so it's out of scope. + /// + private static void AddPackageEdges(Solution solution, Dictionary> dependents) + { + var solutionDir = solution.FilePath is { } solutionPath ? Path.GetDirectoryName(solutionPath) : null; + + var packageIdToProjectIds = new Dictionary>(StringComparer.OrdinalIgnoreCase); + foreach (var project in solution.Projects) + { + var packageId = GetPackageId(project, solutionDir); + if (!packageIdToProjectIds.TryGetValue(packageId, out var producers)) packageIdToProjectIds[packageId] = producers = []; + producers.Add(project.Id); + } + + foreach (var group in solution.Projects.Where(IsTestProject).GroupBy(p => p.FilePath)) + { + if (group.Key is not { } testProjectPath) continue; // no file on disk: no obj folder to read assets from. + var projectDir = Path.GetDirectoryName(testProjectPath); + if (projectDir is null) continue; + var packageIds = ReadAssetsPackageIds(Path.Combine(projectDir, "obj", "project.assets.json")); + if (packageIds.Count == 0) continue; + + var testProjectIds = group.Select(p => p.Id).ToList(); + foreach (var packageId in packageIds) + { + if (!packageIdToProjectIds.TryGetValue(packageId, out var producers)) continue; + foreach (var producerId in producers) + { + if (!dependents.TryGetValue(producerId, out var consumers)) dependents[producerId] = consumers = []; + foreach (var testProjectId in testProjectIds) if (!consumers.Contains(testProjectId)) consumers.Add(testProjectId); + } + } + } + } + + /// + /// Test projects (dedupe by file path across TFM variants) whose obj/project.assets.json "targets" section + /// references any of the given package ids. The targets section is already transitive, so this also finds a + /// project that depends on a changed package only via another package in its own graph. Used by the Central + /// Package Management precision path in TestingTools.RunAffected. + /// + public static IReadOnlyList FindTestProjectsUsingPackages(Solution solution, IReadOnlySet packageIds) + { + var results = new List(); + foreach (var group in solution.Projects.Where(IsTestProject).GroupBy(p => p.FilePath)) + { + if (group.Key is not { } testProjectPath) continue; + var projectDir = Path.GetDirectoryName(testProjectPath); + if (projectDir is null) continue; + var referenced = ReadAssetsPackageIds(Path.Combine(projectDir, "obj", "project.assets.json")); + if (referenced.Overlaps(packageIds)) results.Add(testProjectPath); + } + return results; + } + + /// + /// Test projects (dedupe by file path across TFM variants) with no obj/project.assets.json at all - i.e. never + /// restored - for whom and cannot see + /// any package reference. Surfaced in the affected-test note so a package-mediated edge that was missed reads as + /// "not restored", not as "this project doesn't depend on the change". + /// + public static int CountUnrestoredTestProjects(Solution solution) => + solution.Projects.Where(IsTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase) + .Count(path => !File.Exists(Path.Combine(Path.GetDirectoryName(path)!, "obj", "project.assets.json"))); + + /// + /// The package id a solution project would publish as, for matching against project.assets.json package + /// entries: a literal <PackageId> in the project file, else in the nearest Directory.Build.props walking up + /// from the project's folder (stopping at the solution folder), else the project's AssemblyName. A file that + /// doesn't exist on disk (AdhocWorkspace tests build projects with no real file) is silently skipped rather than + /// thrown on, falling through to AssemblyName. + /// + private static string GetPackageId(Project project, string? solutionDir) + { + if (project.FilePath is not { } projectPath) return project.AssemblyName; + if (TryReadPackageIdLiteral(projectPath) is { } fromProject) return fromProject; + + var boundary = solutionDir is null ? null : NormalizeDir(solutionDir); + var dir = Path.GetDirectoryName(projectPath); + while (dir is not null) + { + if (TryReadPackageIdLiteral(Path.Combine(dir, "Directory.Build.props")) is { } fromProps) return fromProps; + if (boundary is not null && NormalizeDir(dir) == boundary) break; + + var parent = Path.GetDirectoryName(dir); + if (parent is null || parent == dir) break; + dir = parent; + } + return project.AssemblyName; + + static string NormalizeDir(string d) => Path.GetFullPath(d).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + } + + /// + /// The literal text of a <PackageId> element in a project or Directory.Build.props file, or null when the + /// file is missing, unreadable, malformed, has no such element, or the element's value contains "$(" - + /// ponytail: an MSBuild property reference (e.g. "$(AssemblyName).Extra") this walk doesn't evaluate, since doing + /// so would need the MSBuild engine rather than a plain XML read. Namespace-agnostic: SDK-style project files + /// declare no xmlns, but this also tolerates one if present. + /// + private static string? TryReadPackageIdLiteral(string filePath) + { + if (!File.Exists(filePath)) return null; + try + { + var value = XDocument.Load(filePath).Descendants().FirstOrDefault(e => e.Name.LocalName == "PackageId")?.Value.Trim(); + return string.IsNullOrEmpty(value) || value.Contains("$(", StringComparison.Ordinal) ? null : value; + } + catch (Exception ex) when (ex is System.Xml.XmlException or IOException or UnauthorizedAccessException) + { + return null; + } + } + + /// + /// Package ids referenced anywhere in a project.assets.json's "targets" section - every TFM key, since one + /// assets file already covers every TFM of a multi-targeted project: "targets" -> <tfm> -> + /// "<id>/<version>" entries whose "type" is "package" (as opposed to "project", a ProjectReference + /// restored into the same graph, which is already covered separately). Empty - never throwing - when the file is + /// missing or the JSON is malformed: a corrupted or absent lock file just means this edge source contributes + /// nothing, not a hard failure for the whole affected-test walk. + /// + private static HashSet ReadAssetsPackageIds(string assetsJsonPath) + { + var ids = new HashSet(StringComparer.OrdinalIgnoreCase); + if (!File.Exists(assetsJsonPath)) return ids; + try + { + using var stream = File.OpenRead(assetsJsonPath); + using var document = JsonDocument.Parse(stream); + if (!document.RootElement.TryGetProperty("targets", out var targets)) return ids; + foreach (var tfm in targets.EnumerateObject()) + { + foreach (var entry in tfm.Value.EnumerateObject()) + { + if (entry.Value.ValueKind != JsonValueKind.Object) continue; + if (!entry.Value.TryGetProperty("type", out var type) || !type.ValueEquals("package")) continue; + var slash = entry.Name.IndexOf('/'); + ids.Add(slash > 0 ? entry.Name[..slash] : entry.Name); + } + } + } + catch (Exception ex) when (ex is JsonException or IOException) + { + return new HashSet(StringComparer.OrdinalIgnoreCase); + } + return ids; + } + /// /// The projects a changed file belongs to: those that compile it, or else those whose folder holds it (the deepest such /// folder, for nested projects). That covers files the reference walk can't see: .csproj, .razor, .json, resources, and diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index 2e90d2d..3eaf1c4 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -1,9 +1,11 @@ // Copyright (c) 2025 Ahmed Mustafa using System.ComponentModel; +using System.Xml.Linq; using DotNetDevMCP.CodeIntelligence.Interfaces; using DotNetDevMCP.Core; using DotNetDevMCP.Core.Models; +using Microsoft.CodeAnalysis; using Microsoft.Extensions.Logging; using ModelContextProtocol.Server; @@ -86,6 +88,8 @@ public static async Task RunAffected( logger.LogDebug("dotnet_test_affected: resolved {Count} changed files in {Ms} ms", files.Length, sw.ElapsedMilliseconds); var solution = solutions.CurrentSolution; var changed = files.Select(f => Path.GetFullPath(Path.Combine(solutionDir, f))).Where(f => !DocumentationExtensions.Contains(Path.GetExtension(f))).ToArray(); + // Never traced at any layer (not a document, not a project-owned file, not build-wide): noted, not analyzed. + var dllChanged = changed.Where(f => f.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)).ToArray(); // The walk traces only C# the solution compiles. Anything else (a .csproj, .razor, appsettings.json, a deleted file) // can still break tests, so it switches to the project fallback below instead of being dropped. files = changed.Where(f => f.EndsWith(".cs", StringComparison.OrdinalIgnoreCase) && !solution.GetDocumentIdsWithFilePath(f).IsEmpty).ToArray(); @@ -127,13 +131,30 @@ public static async Task RunAffected( ? $"Selection stopped after {maxSelectionSeconds}s and {selection.SymbolsSearched} symbols: this change reaches too much code to trace cheaply." : $"Selected {affected.Count} of {selection.TotalTestMethods} test methods ({selectedFraction:P0}), above the {maxSelectedFraction:P0} threshold: a selection this large is likely slower filtered than running the whole solution."; + // Central Package Management precision: a Directory.Packages.props change is build-wide in general (it can + // affect any project), but when it's the ONLY build-wide file that changed, diffing its previous and current + // XML tells us exactly which package ids moved - and the assets-based edge above can then tell us exactly + // which test projects use them, instead of falling back to the whole solution. + var buildWideFiles = untraced.Where(IsBuildWideFile).ToArray(); + var cpmOnly = buildWideChange && buildWideFiles.Length == 1 + && Path.GetFileName(buildWideFiles[0]).Equals("Directory.Packages.props", StringComparison.OrdinalIgnoreCase); + var cpm = cpmOnly ? await TryCentralPackageManagementScopeAsync(solution, solutionDir, buildWideFiles[0], gitBase, cancellationToken) : null; + var reachableProjects = AffectedTestFinder.FindAffectedTestProjects(solution, files.Concat(untraced)); allTestProjects = AffectedTestFinder.AllTestProjectFilePaths(solution); - if (buildWideChange || reachableProjects.Count == 0 || reachableProjects.Count >= allTestProjects.Count) + if (cpm is { Projects.Count: > 0 } cpmHit) + { + scope = AffectedRunScope.Projects; + projectsToRun = cpmHit.Projects; + note = cpmHit.Note; + } + else if (buildWideChange || reachableProjects.Count == 0 || reachableProjects.Count >= allTestProjects.Count) { scope = AffectedRunScope.Solution; - note = buildWideChange + note = cpm is { } cpmMiss + ? $"{reason} {cpmMiss.Note}" + : buildWideChange ? $"{reason} A build-wide file changed, which can affect every project; running the whole solution instead." : $"{reason} Every test project in the solution is reachable from the change (or none could be resolved to a project); running the whole solution instead."; } @@ -148,6 +169,19 @@ public static async Task RunAffected( "detected by this fallback and may be missed."; } } + + if (scope == AffectedRunScope.Projects) + { + var unrestoredCount = AffectedTestFinder.CountUnrestoredTestProjects(solution); + if (unrestoredCount > 0) + { + note = $"{note} {unrestoredCount} test project(s) have no obj/project.assets.json (not restored), so package references for them are unknown."; + } + } + if (dllChanged.Length > 0) + { + note = note is null ? "Binary references (.dll) are not traced." : $"{note} Binary references (.dll) are not traced."; + } var ranWholeSolution = scope == AffectedRunScope.Solution; // kept for compatibility; true only when the whole solution ran. if (dryRun || (scope == AffectedRunScope.Selection && affected.Count == 0)) @@ -266,4 +300,112 @@ private static async Task GitChangedFilesAsync(string repoDir, string? .Select(rel => Path.Combine(root, rel)) .ToArray(); } + + /// Result of : which test projects to run and the note + /// to report for it. Projects is empty when precision wasn't possible - Note then explains why, and the caller + /// falls back to running the whole solution exactly as it did before this precision existed. + private sealed record CpmScope(IReadOnlyList Projects, string Note); + + /// + /// Central Package Management precision for a Directory.Packages.props change: diffs its previous and current + /// version (via git) to the package ids that were actually added, removed, or changed version, then selects only + /// the test projects whose restored project.assets.json references one of them (see + /// - its "targets" section is already transitive). + /// Falls back to an empty result - explained in its Note - when git isn't available, either version of the file + /// can't be read or parsed, or no restored test project uses the changed ids; the caller then runs the whole + /// solution exactly as it did before this precision existed. + /// + private static async Task TryCentralPackageManagementScopeAsync( + Solution solution, string solutionDir, string propsFilePath, string? gitBase, CancellationToken ct) + { + const string fallbackSuffix = "running the whole solution instead."; + string newXml; + try + { + newXml = await File.ReadAllTextAsync(propsFilePath, ct); + } + catch (IOException) + { + return new CpmScope([], $"Directory.Packages.props changed but could not be read; {fallbackSuffix}"); + } + + // Same argument-list git invocation the file already uses (GitChangedFilesAsync): an ArgumentList, not a + // concatenated string, so a ref that starts with "-" can't be parsed as an option. + var (rootExit, rootOut, _, _) = await TestRunner.RunProcessAsync("git", ["rev-parse", "--show-toplevel"], ct, solutionDir); + if (rootExit != 0) + { + return new CpmScope([], $"Directory.Packages.props changed but git is not available to diff it; {fallbackSuffix}"); + } + var root = rootOut.Trim(); + var relative = Path.GetRelativePath(root, propsFilePath).Replace('\\', '/'); + var gitRef = gitBase ?? "HEAD"; + + var (showExit, showOut, showErr, _) = await TestRunner.RunProcessAsync("git", ["show", $"{gitRef}:{relative}"], ct, root); + if (showExit != 0) + { + return new CpmScope([], $"Directory.Packages.props changed but its previous version ({gitRef}:{relative}) could not be read from git ({showErr.Trim()}); {fallbackSuffix}"); + } + + var changedIds = DiffPackageVersions(showOut, newXml); + if (changedIds is null) + { + return new CpmScope([], $"Directory.Packages.props changed but its XML could not be diffed; {fallbackSuffix}"); + } + if (changedIds.Count == 0) + { + return new CpmScope([], $"Directory.Packages.props changed but no package version actually differs between {gitRef} and the working tree; {fallbackSuffix}"); + } + + var selected = AffectedTestFinder.FindTestProjectsUsingPackages(solution, changedIds); + if (selected.Count == 0) + { + return new CpmScope([], $"Directory.Packages.props changed ({string.Join(", ", changedIds.OrderBy(x => x, StringComparer.OrdinalIgnoreCase))}) but no restored test project references those packages; {fallbackSuffix}"); + } + + return new CpmScope(selected, $"Directory.Packages.props changed: {string.Join(", ", changedIds.OrderBy(x => x, StringComparer.OrdinalIgnoreCase))}; running the test projects that use them."); + } + + /// + /// Package ids whose <PackageVersion Include="X" Version="V" /> entry was added, removed, or changed + /// version between two versions of a Directory.Packages.props file's XML (namespace-agnostic: an SDK-style props + /// file declares no xmlns, but this tolerates one if present). Pure and synchronous, so it's unit-testable + /// without git or the filesystem. Null - not throwing - when either document fails to parse as XML: the caller + /// falls back to today's whole-solution behavior for a props file it can't safely diff. + /// + public static IReadOnlySet? DiffPackageVersions(string oldXml, string newXml) + { + var oldVersions = TryParsePackageVersions(oldXml); + var newVersions = TryParsePackageVersions(newXml); + if (oldVersions is null || newVersions is null) return null; + + var changed = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var (id, version) in newVersions) + { + if (!oldVersions.TryGetValue(id, out var oldVersion) || !string.Equals(oldVersion, version, StringComparison.Ordinal)) changed.Add(id); + } + foreach (var id in oldVersions.Keys) + { + if (!newVersions.ContainsKey(id)) changed.Add(id); + } + return changed; + } + + private static Dictionary? TryParsePackageVersions(string xml) + { + try + { + var result = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var element in XDocument.Parse(xml).Descendants().Where(e => e.Name.LocalName == "PackageVersion")) + { + var include = element.Attribute("Include")?.Value; + var version = element.Attribute("Version")?.Value; + if (!string.IsNullOrEmpty(include) && version is not null) result[include] = version; + } + return result; + } + catch (System.Xml.XmlException) + { + return null; + } + } } diff --git a/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs new file mode 100644 index 0000000..8555d01 --- /dev/null +++ b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs @@ -0,0 +1,244 @@ +// Copyright (c) 2025 Ahmed Mustafa + +using DotNetDevMCP.Testing; +using DotNetDevMCP.Testing.Mcp.Tools; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Text; + +namespace DotNetDevMCP.Testing.Tests; + +/// +/// The affected-test project fallback () following NuGet +/// package references, not just Roslyn ProjectReferences: a test project that consumes a solution library only +/// through a restored PackageReference (its obj/project.assets.json), with no ProjectReference at all, should still +/// be selected when that library changes. Unlike , these need real files on +/// disk (a project.assets.json fixture, and for one case a real .csproj with a literal <PackageId>), so each +/// test builds its own temp directory tree and points an AdhocWorkspace's project FilePaths at it. +/// +public class NuGetPackageEdgeTests : IDisposable +{ + private readonly string _root = Path.Combine(Path.GetTempPath(), "dotnetdevmcp-nuget-edge-tests", Guid.NewGuid().ToString("N")); + + public void Dispose() + { + if (Directory.Exists(_root)) + { + try { Directory.Delete(_root, recursive: true); } catch (IOException) { /* best effort */ } + } + } + + [Fact] + public void Test_project_referencing_a_package_with_no_ProjectReference_is_selected_when_the_library_changes() + { + var (solution, libACs) = Build( + libAProjectXml: DefaultCsproj, + assetsJson: AssetsJson(("LibA", "package"))); + + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [libACs]); + + Assert.Equal([Path.Combine(_root, "test", "LibA.Tests.csproj")], affected); + } + + [Fact] + public void An_unrelated_package_id_in_the_assets_file_does_not_create_an_edge() + { + var (solution, libACs) = Build( + libAProjectXml: DefaultCsproj, + assetsJson: AssetsJson(("Some.Other.Package", "package"))); + + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [libACs]); + + Assert.Empty(affected); + } + + [Fact] + public void A_literal_PackageId_in_the_project_file_is_used_instead_of_the_assembly_name() + { + const string customPackageId = """ + + + net10.0 + Custom.Id + + + """; + + var (solution, libACs) = Build( + libAProjectXml: customPackageId, + assetsJson: AssetsJson(("Custom.Id", "package"))); + + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [libACs]); + + Assert.Equal([Path.Combine(_root, "test", "LibA.Tests.csproj")], affected); + } + + [Fact] + public void Malformed_assets_json_does_not_throw_and_creates_no_edge() + { + var (solution, libACs) = Build(libAProjectXml: DefaultCsproj, assetsJson: "{ this is not valid json"); + + var affected = Record.Exception(() => AffectedTestFinder.FindAffectedTestProjects(solution, [libACs])); + + Assert.Null(affected); + Assert.Empty(AffectedTestFinder.FindAffectedTestProjects(solution, [libACs])); + } + + [Fact] + public void CountUnrestoredTestProjects_counts_test_projects_with_no_assets_file() + { + var (solution, _) = Build(libAProjectXml: DefaultCsproj, assetsJson: null); + + Assert.Equal(1, AffectedTestFinder.CountUnrestoredTestProjects(solution)); + } + + [Fact] + public void CountUnrestoredTestProjects_is_zero_once_the_assets_file_exists() + { + var (solution, _) = Build(libAProjectXml: DefaultCsproj, assetsJson: AssetsJson(("LibA", "package"))); + + Assert.Equal(0, AffectedTestFinder.CountUnrestoredTestProjects(solution)); + } + + private const string DefaultCsproj = """ + + + net10.0 + + + """; + + private static string AssetsJson(params (string Id, string Type)[] entries) + { + var targets = string.Join(",\n", entries.Select(e => $$""" + "{{e.Id}}/1.0.0": { "type": "{{e.Type}}" } + """)); + return $$""" + { + "targets": { + "net10.0": { + {{targets}} + } + } + } + """; + } + + /// + /// LibA (a library with the given project XML) and LibA.Tests (an xUnit project with NO ProjectReference to + /// LibA - the whole point of these tests - but with the given project.assets.json, or none) on real temp-folder + /// paths, wired into an AdhocWorkspace the same way does it. + /// + private (Solution Solution, string LibACsprojPath) Build(string libAProjectXml, string? assetsJson) + { + var srcDir = Path.Combine(_root, "src"); + var testDir = Path.Combine(_root, "test"); + Directory.CreateDirectory(srcDir); + Directory.CreateDirectory(testDir); + + var libACsprojPath = Path.Combine(srcDir, "LibA.csproj"); + File.WriteAllText(libACsprojPath, libAProjectXml); + + var testsCsprojPath = Path.Combine(testDir, "LibA.Tests.csproj"); + File.WriteAllText(testsCsprojPath, DefaultCsproj); + + if (assetsJson is not null) + { + var objDir = Path.Combine(testDir, "obj"); + Directory.CreateDirectory(objDir); + File.WriteAllText(Path.Combine(objDir, "project.assets.json"), assetsJson); + } + + var workspace = new AdhocWorkspace(); + var corlib = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); + var runtime = MetadataReference.CreateFromFile(Path.Combine(Path.GetDirectoryName(typeof(object).Assembly.Location)!, "System.Runtime.dll")); + var xunit = MetadataReference.CreateFromFile(typeof(FactAttribute).Assembly.Location); + + var solution = workspace.AddSolution(SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Default, Path.Combine(_root, "Test.sln"), [])); + + var libAId = ProjectId.CreateNewId(); + var libASourcePath = Path.Combine(srcDir, "A.cs"); + solution = solution + .AddProject(ProjectInfo.Create(libAId, VersionStamp.Default, "LibA", "LibA", LanguageNames.CSharp, + filePath: libACsprojPath, metadataReferences: [corlib, runtime])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(libAId), "A.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From("namespace LibA; public class A { public int M() => 1; }"), VersionStamp.Default)), + filePath: libASourcePath)); + + var testsId = ProjectId.CreateNewId(); + solution = solution + // No AddProjectReference here: the edge under test comes only from the package reference below. + .AddProject(ProjectInfo.Create(testsId, VersionStamp.Default, "LibA.Tests", "LibA.Tests", LanguageNames.CSharp, + filePath: testsCsprojPath, metadataReferences: [corlib, runtime, xunit])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(testsId), "T.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From( + "using Xunit; namespace LibA.Tests; public class T { [Fact] public void T1() { } }"), VersionStamp.Default)), + filePath: Path.Combine(testDir, "T.cs"))); + + return (solution, libACsprojPath); + } +} + +/// +/// The pure, unit-testable half of the Central Package Management precision in +/// : diffing two versions of a Directory.Packages.props file's XML to the set +/// of package ids that were added, removed, or changed version. +/// +public class CentralPackageManagementDiffTests +{ + [Fact] + public void Detects_a_version_bump() + { + var oldXml = Props(("Foo", "1.0.0"), ("Bar", "2.0.0")); + var newXml = Props(("Foo", "1.1.0"), ("Bar", "2.0.0")); + + var changed = TestingTools.DiffPackageVersions(oldXml, newXml); + + Assert.Equal(["Foo"], changed); + } + + [Fact] + public void Detects_an_added_package() + { + var oldXml = Props(("Foo", "1.0.0")); + var newXml = Props(("Foo", "1.0.0"), ("Bar", "2.0.0")); + + var changed = TestingTools.DiffPackageVersions(oldXml, newXml); + + Assert.Equal(["Bar"], changed); + } + + [Fact] + public void Detects_a_removed_package() + { + var oldXml = Props(("Foo", "1.0.0"), ("Bar", "2.0.0")); + var newXml = Props(("Foo", "1.0.0")); + + var changed = TestingTools.DiffPackageVersions(oldXml, newXml); + + Assert.Equal(["Bar"], changed); + } + + [Fact] + public void Unchanged_packages_are_not_reported() + { + var oldXml = Props(("Foo", "1.0.0"), ("Bar", "2.0.0")); + var newXml = Props(("Foo", "1.0.0"), ("Bar", "2.0.0")); + + var changed = TestingTools.DiffPackageVersions(oldXml, newXml); + + Assert.Empty(changed!); + } + + [Fact] + public void Malformed_xml_returns_null_instead_of_throwing() + { + var changed = TestingTools.DiffPackageVersions("", Props(("Foo", "1.0.0"))); + + Assert.Null(changed); + } + + private static string Props(params (string Id, string Version)[] entries) => + "\n \n" + + string.Join("\n", entries.Select(e => $""" """)) + + "\n \n"; +} From ca97dfe09ce1bd2a4b02e3af974c4b3921ece9ab Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 17:20:22 +0300 Subject: [PATCH 2/4] NuGet edges: review fixes Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 17 +- README.md | 2 +- .../AffectedTestFinder.cs | 154 ++++++-- .../Mcp/Tools/TestingTools.cs | 154 ++++++-- .../NuGetPackageEdgeTests.cs | 34 ++ .../RunAffectedCpmReviewFixTests.cs | 358 ++++++++++++++++++ 6 files changed, 638 insertions(+), 81 deletions(-) create mode 100644 tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index caee602..f9de21a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,18 @@ All notable changes to DotNetDevMCP are documented here. The format follows - `dotnet_test_affected`'s project fallback now follows NuGet package references, not just `ProjectReference`s: when a test project's restored `obj/project.assets.json` references another solution project's package id (a literal ``, one from the nearest `Directory.Build.props`, or the assembly name), that test project is selected - even with no `ProjectReference` between them. A change to only `Directory.Packages.props` is narrowed, via the same - assets data, to the test projects that use the package ids whose version actually moved (diffed against git), - instead of running the whole solution; the note explains why when it can't (git unavailable, the file doesn't - parse, or no restored test project uses those ids). Test projects with no `obj/project.assets.json` (not restored) - are called out in the note when the run falls back to whole test projects. + even with no `ProjectReference` between them. A change to `Directory.Packages.props` is narrowed, via the same + assets data, to the test projects that use the package ids whose version actually moved (diffed against git) - + walking from EVERY solution project whose restored assets reference a changed id, not just test projects', so a + `PrivateAssets="all"` package (an analyzer or source generator) a source project consumes directly is still + tracked to the test projects that reference that source project - but only when `Directory.Packages.props` is the + *only* changed file, only `Version` attributes actually changed (any other edit to the file, even alongside a + version bump, is treated as "beyond package versions" and not narrowed), and every solution project has been + restored; helper libraries with no test method are never selected, and a narrowed set that turns out to cover + every runnable test project runs the whole solution in one invocation instead. Any other case runs the whole + solution, with a note explaining why (git unavailable, the file doesn't parse, something beyond package versions + changed, an unrestored project, or no restored project uses those ids). Test projects with no + `obj/project.assets.json` (not restored) are called out in the note when the run falls back to whole test projects. ## [0.3.3] - 2026-09-24 diff --git a/README.md b/README.md index 704389d..12f1d3e 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ Measured with BenchmarkDotNet on an i7-10750H, .NET 10.0.9. The orchestration be After an edit, the agent usually reruns the whole suite. `dotnet_test_affected` asks Roslyn instead: take the symbols declared in the changed files, follow references (up to `maxDepth` hops, default 8) until you land in a method with `[Fact]`, `[Theory]`, `[Test]`, `[TestCase]` or `[TestMethod]`, then run exactly those. Changed files default to the git working tree, or `gitBase: "main"` for a branch. `dryRun: true` lists the tests without running them; `framework: "net10.0"` runs one target framework of multi-targeted test projects. -The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). The project fallback also follows restored NuGet package references (a test project whose `obj/project.assets.json` references another solution project's package id, with no `ProjectReference` between them, is still selected), and a change to only `Directory.Packages.props` is narrowed, via that same assets data, to the test projects that actually use the package ids whose version moved, instead of running the whole solution. +The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). The project fallback also follows restored NuGet package references (a test project whose `obj/project.assets.json` references another solution project's package id, with no `ProjectReference` between them, is still selected). A change to `Directory.Packages.props` is narrowed to the test projects that actually use the package ids whose version moved only when it is the *only* changed file, only the `Version` attributes actually changed (not a `Condition`, a `GlobalPackageReference`, or any other edit riding along), and every solution project has been restored; any other change to that file, a change alongside another file, or an unrestored project anywhere in the solution runs the whole solution instead, with a note explaining which of those applied. On this repository, editing `ConcurrentExecutor.cs` selects 22 of 44 tests (the `ConcurrentExecutorTests` plus the `OrchestrationServiceTests` that reach it through `OrchestrationService`). Measured through the MCP tool, build included, i7-10750H: diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index ec4a3fe..226ae4a 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -216,22 +216,12 @@ private static bool IsTestProject(Project p) => p.MetadataReferences.Any(r => /// edge in this graph and will not be found. That's a real gap, not a bug - documenting it here and in the tool's /// response note is the fix, since detecting package-mediated dependencies would need a much heavier analysis. /// - public static IReadOnlyList FindAffectedTestProjects(Solution solution, IEnumerable changedFiles) + public static IReadOnlyList FindAffectedTestProjects(Solution solution, IEnumerable changedFiles, AssetsCache? assetsCache = null) { var changedProjectIds = changedFiles.SelectMany(f => OwningProjects(solution, f)).ToHashSet(); if (changedProjectIds.Count == 0) return []; - // Reverse ProjectReference edges (referenced -> referencing projects), across all TFM variants. - var dependents = new Dictionary>(); - foreach (var project in solution.Projects) - { - foreach (var reference in project.ProjectReferences) - { - if (!dependents.TryGetValue(reference.ProjectId, out var list)) dependents[reference.ProjectId] = list = []; - list.Add(project.Id); - } - } - AddPackageEdges(solution, dependents); + var dependents = BuildDependentsGraph(solution, assetsCache ?? new AssetsCache()); var reachable = new HashSet(changedProjectIds); var frontier = new Queue(changedProjectIds); @@ -245,6 +235,24 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); } + /// Reverse ProjectReference edges (referenced -> referencing projects, across all TFM variants) plus + /// 's package-mediated edges - the same graph + /// and both walk, built once so both share one assets cache. + private static Dictionary> BuildDependentsGraph(Solution solution, AssetsCache assetsCache) + { + var dependents = new Dictionary>(); + foreach (var project in solution.Projects) + { + foreach (var reference in project.ProjectReferences) + { + if (!dependents.TryGetValue(reference.ProjectId, out var list)) dependents[reference.ProjectId] = list = []; + list.Add(project.Id); + } + } + AddPackageEdges(solution, dependents, assetsCache); + return dependents; + } + /// /// Reverse edges from a solution project P to a test project that consumes P only through a restored NuGet /// PackageReference - no Roslyn ProjectReference at all - discovered from the test project's @@ -253,7 +261,7 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, /// ponytail: this only follows package ids that match another SOLUTION project's resolved packageId. A test /// project that depends on a third-party (non-solution) package is unaffected either way, so it's out of scope. /// - private static void AddPackageEdges(Solution solution, Dictionary> dependents) + private static void AddPackageEdges(Solution solution, Dictionary> dependents, AssetsCache assetsCache) { var solutionDir = solution.FilePath is { } solutionPath ? Path.GetDirectoryName(solutionPath) : null; @@ -268,10 +276,8 @@ private static void AddPackageEdges(Solution solution, Dictionary p.FilePath)) { if (group.Key is not { } testProjectPath) continue; // no file on disk: no obj folder to read assets from. - var projectDir = Path.GetDirectoryName(testProjectPath); - if (projectDir is null) continue; - var packageIds = ReadAssetsPackageIds(Path.Combine(projectDir, "obj", "project.assets.json")); - if (packageIds.Count == 0) continue; + var packageIds = assetsCache.Get(testProjectPath); + if (packageIds is not { Count: > 0 }) continue; var testProjectIds = group.Select(p => p.Id).ToList(); foreach (var packageId in packageIds) @@ -286,35 +292,96 @@ private static void AddPackageEdges(Solution solution, DictionaryResult of : either the runnable test projects reachable + /// from the change, or - when UnrestoredProjectPath is set - a signal that narrowing isn't safe because that + /// project's restore state is unknown (TestProjects is then always empty). + public sealed record PackageChangeImpact(IReadOnlyList TestProjects, string? UnrestoredProjectPath); + /// - /// Test projects (dedupe by file path across TFM variants) whose obj/project.assets.json "targets" section - /// references any of the given package ids. The targets section is already transitive, so this also finds a - /// project that depends on a changed package only via another package in its own graph. Used by the Central - /// Package Management precision path in TestingTools.RunAffected. + /// Central Package Management precision support: the runnable test projects reachable from a change to the given + /// package ids, considering EVERY solution project's restored assets - not just test projects' own. A package + /// referenced with PrivateAssets="all" (an analyzer or source generator) never appears in a downstream test + /// project's own project.assets.json, only in the source project's that references it directly; scanning test + /// projects alone (the previous approach) made such a change invisible. A project whose assets show a match is + /// treated exactly like a directly-changed project: reachable test projects are found by walking the same + /// reverse ProjectReference + package-reference graph uses, then filtered + /// to (not just ) so a helper library with no test + /// method (referencing xUnit but declaring none, like Polly.TestUtils) is never selected to run. + /// UnrestoredProjectPath is set - and TestProjects then empty - the moment ANY solution project (test or not) has + /// no obj/project.assets.json at all: without every project's assets, "this project doesn't use the changed + /// package" can't be told apart from "restore state unknown", so the caller should not narrow. /// - public static IReadOnlyList FindTestProjectsUsingPackages(Solution solution, IReadOnlySet packageIds) + public static PackageChangeImpact FindTestProjectsForPackageChange(Solution solution, IReadOnlySet packageIds, AssetsCache assetsCache) { - var results = new List(); - foreach (var group in solution.Projects.Where(IsTestProject).GroupBy(p => p.FilePath)) + var projectsByPath = solution.Projects.Where(p => p.FilePath is not null) + .GroupBy(p => p.FilePath!, StringComparer.OrdinalIgnoreCase).ToList(); + + foreach (var group in projectsByPath) { - if (group.Key is not { } testProjectPath) continue; - var projectDir = Path.GetDirectoryName(testProjectPath); - if (projectDir is null) continue; - var referenced = ReadAssetsPackageIds(Path.Combine(projectDir, "obj", "project.assets.json")); - if (referenced.Overlaps(packageIds)) results.Add(testProjectPath); + if (assetsCache.Get(group.Key) is null) return new PackageChangeImpact([], group.Key); } - return results; + + var matched = new HashSet(); + foreach (var group in projectsByPath) + { + var ids = assetsCache.Get(group.Key); + if (ids is { Count: > 0 } && ids.Overlaps(packageIds)) + { + foreach (var project in group) matched.Add(project.Id); + } + } + if (matched.Count == 0) return new PackageChangeImpact([], null); + + var dependents = BuildDependentsGraph(solution, assetsCache); + var reachable = new HashSet(matched); + var frontier = new Queue(matched); + while (frontier.Count > 0) + { + if (!dependents.TryGetValue(frontier.Dequeue(), out var deps)) continue; + foreach (var dep in deps) if (reachable.Add(dep)) frontier.Enqueue(dep); + } + + var testProjects = reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) + .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + return new PackageChangeImpact(testProjects, null); } /// /// Test projects (dedupe by file path across TFM variants) with no obj/project.assets.json at all - i.e. never - /// restored - for whom and cannot see - /// any package reference. Surfaced in the affected-test note so a package-mediated edge that was missed reads as - /// "not restored", not as "this project doesn't depend on the change". + /// restored - for whom cannot see any package reference. Surfaced in the + /// affected-test note so a package-mediated edge that was missed reads as "not restored", not as "this project + /// doesn't depend on the change". + /// + public static int CountUnrestoredTestProjects(Solution solution, AssetsCache? assetsCache = null) + { + assetsCache ??= new AssetsCache(); + return solution.Projects.Where(IsTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase) + .Count(path => assetsCache.Get(path) is null); + } + + /// + /// Per-call cache of a project's project.assets.json, keyed by project file path: null means the file doesn't + /// exist (the project has never been restored); otherwise the package ids + /// found (possibly empty, for a restored project with no "type":"package" entries or a malformed file). Shared + /// across , and + /// within one `dotnet_test_affected` call so each project's assets file + /// is read and parsed at most once, even though all three ask about the same projects. /// - public static int CountUnrestoredTestProjects(Solution solution) => - solution.Projects.Where(IsTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase) - .Count(path => !File.Exists(Path.Combine(Path.GetDirectoryName(path)!, "obj", "project.assets.json"))); + public sealed class AssetsCache + { + private readonly Dictionary?> _byProjectPath = new(StringComparer.OrdinalIgnoreCase); + + /// A project's .csproj path; its obj/project.assets.json is read relative to it. + public HashSet? Get(string projectFilePath) + { + if (_byProjectPath.TryGetValue(projectFilePath, out var cached)) return cached; + var dir = Path.GetDirectoryName(projectFilePath); + var assetsPath = dir is null ? null : Path.Combine(dir, "obj", "project.assets.json"); + var result = assetsPath is not null && File.Exists(assetsPath) ? ReadAssetsPackageIds(assetsPath) : null; + _byProjectPath[projectFilePath] = result; + return result; + } + } /// /// The package id a solution project would publish as, for matching against project.assets.json package @@ -370,8 +437,11 @@ private static string GetPackageId(Project project, string? solutionDir) /// assets file already covers every TFM of a multi-targeted project: "targets" -> <tfm> -> /// "<id>/<version>" entries whose "type" is "package" (as opposed to "project", a ProjectReference /// restored into the same graph, which is already covered separately). Empty - never throwing - when the file is - /// missing or the JSON is malformed: a corrupted or absent lock file just means this edge source contributes - /// nothing, not a hard failure for the whole affected-test walk. + /// missing, unreadable, or the JSON is malformed OR simply not shaped like an assets file ("targets" absent, not + /// an object, or a TFM entry that isn't an object): a corrupted, permission-denied, or unexpected-shape lock file + /// just means this edge source contributes nothing, not a hard failure for the whole affected-test walk. Every + /// JsonElement access is guarded by a ValueKind check first, since JsonElement's Get/TryGetProperty and + /// EnumerateObject throw InvalidOperationException on the wrong kind rather than returning false. /// private static HashSet ReadAssetsPackageIds(string assetsJsonPath) { @@ -381,19 +451,21 @@ private static HashSet ReadAssetsPackageIds(string assetsJsonPath) { using var stream = File.OpenRead(assetsJsonPath); using var document = JsonDocument.Parse(stream); - if (!document.RootElement.TryGetProperty("targets", out var targets)) return ids; + if (document.RootElement.ValueKind != JsonValueKind.Object) return ids; + if (!document.RootElement.TryGetProperty("targets", out var targets) || targets.ValueKind != JsonValueKind.Object) return ids; foreach (var tfm in targets.EnumerateObject()) { + if (tfm.Value.ValueKind != JsonValueKind.Object) continue; foreach (var entry in tfm.Value.EnumerateObject()) { if (entry.Value.ValueKind != JsonValueKind.Object) continue; - if (!entry.Value.TryGetProperty("type", out var type) || !type.ValueEquals("package")) continue; + if (!entry.Value.TryGetProperty("type", out var type) || type.ValueKind != JsonValueKind.String || !type.ValueEquals("package")) continue; var slash = entry.Name.IndexOf('/'); ids.Add(slash > 0 ? entry.Name[..slash] : entry.Name); } } } - catch (Exception ex) when (ex is JsonException or IOException) + catch (Exception ex) when (ex is JsonException or IOException or UnauthorizedAccessException) { return new HashSet(StringComparer.OrdinalIgnoreCase); } diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index 3eaf1c4..c768b0f 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -97,7 +97,9 @@ public static async Task RunAffected( var buildWideChange = untraced.Any(IsBuildWideFile); if (files.Length == 0 && untraced.Length == 0) { - return new { Success = true, ChangedFiles = Array.Empty(), AffectedTests = Array.Empty(), Message = "No changed code or project files." }; + var message = "No changed code or project files."; + if (dllChanged.Length > 0) message += " Binary references (.dll) are not traced."; + return new { Success = true, ChangedFiles = Array.Empty(), AffectedTests = Array.Empty(), Message = message }; } sw.Restart(); @@ -118,6 +120,7 @@ public static async Task RunAffected( AffectedRunScope scope; IReadOnlyList projectsToRun = []; IReadOnlyList allTestProjects = []; + var assetsCache = new AffectedTestFinder.AssetsCache(); if (untraced.Length == 0 && selection.Complete && selectedFraction <= maxSelectedFraction) { scope = AffectedRunScope.Selection; @@ -132,23 +135,40 @@ public static async Task RunAffected( : $"Selected {affected.Count} of {selection.TotalTestMethods} test methods ({selectedFraction:P0}), above the {maxSelectedFraction:P0} threshold: a selection this large is likely slower filtered than running the whole solution."; // Central Package Management precision: a Directory.Packages.props change is build-wide in general (it can - // affect any project), but when it's the ONLY build-wide file that changed, diffing its previous and current - // XML tells us exactly which package ids moved - and the assets-based edge above can then tell us exactly - // which test projects use them, instead of falling back to the whole solution. - var buildWideFiles = untraced.Where(IsBuildWideFile).ToArray(); - var cpmOnly = buildWideChange && buildWideFiles.Length == 1 - && Path.GetFileName(buildWideFiles[0]).Equals("Directory.Packages.props", StringComparison.OrdinalIgnoreCase); - var cpm = cpmOnly ? await TryCentralPackageManagementScopeAsync(solution, solutionDir, buildWideFiles[0], gitBase, cancellationToken) : null; - - var reachableProjects = AffectedTestFinder.FindAffectedTestProjects(solution, files.Concat(untraced)); + // affect any project), so it's only narrowed when it is the ONLY changed file of any kind (a build-wide + // props change alongside a traced .cs change, or any other file, keeps the pre-existing build-wide -> + // whole-solution behavior below - merging a package-version diff with a symbol-level selection would be a + // different, riskier feature). Diffing the file's previous and current XML then tells us exactly which + // package ids moved, and the reverse dependency graph (ProjectReference + package edges, walked from every + // solution project - not just test projects - whose restored assets reference one of them) tells us + // exactly which runnable test projects to run instead of the whole solution. + var cpmOnly = changed.Length == 1 + && IsBuildWideFile(changed[0]) + && Path.GetFileName(changed[0]).Equals("Directory.Packages.props", StringComparison.OrdinalIgnoreCase); + var cpm = cpmOnly ? await TryCentralPackageManagementScopeAsync(solution, solutionDir, changed[0], gitBase, assetsCache, cancellationToken) : null; + + // Skipped entirely for the cpmOnly case: a lone build-wide change can never land in the reachableProjects + // branch below (buildWideChange always routes it to Solution or the CPM Projects scope first), so + // computing it would only cost an unused pass over the project graph and assets files. + var reachableProjects = cpmOnly + ? (IReadOnlyList)Array.Empty() + : AffectedTestFinder.FindAffectedTestProjects(solution, files.Concat(untraced), assetsCache); allTestProjects = AffectedTestFinder.AllTestProjectFilePaths(solution); - if (cpm is { Projects.Count: > 0 } cpmHit) + if (cpm is { Projects.Count: > 0 } cpmHit && cpmHit.Projects.Count < allTestProjects.Count) { scope = AffectedRunScope.Projects; projectsToRun = cpmHit.Projects; note = cpmHit.Note; } + else if (cpm is { Projects.Count: > 0 } cpmCoversAll) + { + // Narrowed set happens to be every runnable test project: one solution-wide run is simpler than + // filtered per-project runs that would add up to the same coverage, exactly like the non-CPM fallback + // below already prefers Solution once reachableProjects covers everything. + scope = AffectedRunScope.Solution; + note = $"Directory.Packages.props changed: {cpmCoversAll.IdsSummary}; every runnable test project uses at least one of them, so running the whole solution instead."; + } else if (buildWideChange || reachableProjects.Count == 0 || reachableProjects.Count >= allTestProjects.Count) { scope = AffectedRunScope.Solution; @@ -164,15 +184,15 @@ public static async Task RunAffected( projectsToRun = reachableProjects; var names = string.Join(", ", reachableProjects.Select(Path.GetFileName)); note = $"{reason} Running the test projects reachable from the change via project references instead: {names}. " + - "This is a superset of the affected tests (some of their other tests may also run); a test project that " + - "depends on the changed code only through a NuGet package reference, with no ProjectReference, is not " + - "detected by this fallback and may be missed."; + "This is a superset of the affected tests (some of their other tests may also run); a test project " + + "that uses the changed code through a NuGet package is found only if it has been restored " + + "(obj/project.assets.json); cross-repository consumers are not found."; } } if (scope == AffectedRunScope.Projects) { - var unrestoredCount = AffectedTestFinder.CountUnrestoredTestProjects(solution); + var unrestoredCount = AffectedTestFinder.CountUnrestoredTestProjects(solution, assetsCache); if (unrestoredCount > 0) { note = $"{note} {unrestoredCount} test project(s) have no obj/project.assets.json (not restored), so package references for them are unknown."; @@ -303,20 +323,24 @@ private static async Task GitChangedFilesAsync(string repoDir, string? /// Result of : which test projects to run and the note /// to report for it. Projects is empty when precision wasn't possible - Note then explains why, and the caller - /// falls back to running the whole solution exactly as it did before this precision existed. - private sealed record CpmScope(IReadOnlyList Projects, string Note); + /// falls back to running the whole solution exactly as it did before this precision existed. IdsSummary is the + /// comma-joined changed package ids (empty until they're known), reused by the caller to phrase its own note when + /// the narrowed set happens to cover every runnable test project. + private sealed record CpmScope(IReadOnlyList Projects, string IdsSummary, string Note); /// /// Central Package Management precision for a Directory.Packages.props change: diffs its previous and current - /// version (via git) to the package ids that were actually added, removed, or changed version, then selects only - /// the test projects whose restored project.assets.json references one of them (see - /// - its "targets" section is already transitive). - /// Falls back to an empty result - explained in its Note - when git isn't available, either version of the file - /// can't be read or parsed, or no restored test project uses the changed ids; the caller then runs the whole - /// solution exactly as it did before this precision existed. + /// version (via git) to the package ids that were actually added, removed, or changed version - refusing to + /// narrow at all if anything else in the file changed too (see ) - then + /// selects the runnable test projects reachable from every solution project whose restored assets reference one + /// of those ids (). Falls back to an empty + /// result - explained in its Note - when git isn't available, either version of the file can't be read or + /// parsed, something other than a package version changed, any solution project isn't restored, or no restored + /// project uses the changed ids; the caller then runs the whole solution exactly as it did before this precision + /// existed. /// private static async Task TryCentralPackageManagementScopeAsync( - Solution solution, string solutionDir, string propsFilePath, string? gitBase, CancellationToken ct) + Solution solution, string solutionDir, string propsFilePath, string? gitBase, AffectedTestFinder.AssetsCache assetsCache, CancellationToken ct) { const string fallbackSuffix = "running the whole solution instead."; string newXml; @@ -326,7 +350,7 @@ private static async Task TryCentralPackageManagementScopeAsync( } catch (IOException) { - return new CpmScope([], $"Directory.Packages.props changed but could not be read; {fallbackSuffix}"); + return new CpmScope([], "", $"Directory.Packages.props changed but could not be read; {fallbackSuffix}"); } // Same argument-list git invocation the file already uses (GitChangedFilesAsync): an ArgumentList, not a @@ -334,7 +358,7 @@ private static async Task TryCentralPackageManagementScopeAsync( var (rootExit, rootOut, _, _) = await TestRunner.RunProcessAsync("git", ["rev-parse", "--show-toplevel"], ct, solutionDir); if (rootExit != 0) { - return new CpmScope([], $"Directory.Packages.props changed but git is not available to diff it; {fallbackSuffix}"); + return new CpmScope([], "", $"Directory.Packages.props changed but git is not available to diff it; {fallbackSuffix}"); } var root = rootOut.Trim(); var relative = Path.GetRelativePath(root, propsFilePath).Replace('\\', '/'); @@ -343,26 +367,43 @@ private static async Task TryCentralPackageManagementScopeAsync( var (showExit, showOut, showErr, _) = await TestRunner.RunProcessAsync("git", ["show", $"{gitRef}:{relative}"], ct, root); if (showExit != 0) { - return new CpmScope([], $"Directory.Packages.props changed but its previous version ({gitRef}:{relative}) could not be read from git ({showErr.Trim()}); {fallbackSuffix}"); + return new CpmScope([], "", $"Directory.Packages.props changed but its previous version ({gitRef}:{relative}) could not be read from git ({showErr.Trim()}); {fallbackSuffix}"); + } + + var onlyVersionsChanged = IsOnlyPackageVersionChange(showOut, newXml); + if (onlyVersionsChanged is null) + { + return new CpmScope([], "", $"Directory.Packages.props changed but its XML could not be diffed; {fallbackSuffix}"); + } + if (onlyVersionsChanged == false) + { + return new CpmScope([], "", $"Directory.Packages.props changed beyond package versions; {fallbackSuffix}"); } var changedIds = DiffPackageVersions(showOut, newXml); if (changedIds is null) { - return new CpmScope([], $"Directory.Packages.props changed but its XML could not be diffed; {fallbackSuffix}"); + // Can't happen given onlyVersionsChanged was true above (both documents parsed), but keep the same safe + // fallback rather than assuming. + return new CpmScope([], "", $"Directory.Packages.props changed but its XML could not be diffed; {fallbackSuffix}"); } + var idsSummary = string.Join(", ", changedIds.OrderBy(x => x, StringComparer.OrdinalIgnoreCase)); if (changedIds.Count == 0) { - return new CpmScope([], $"Directory.Packages.props changed but no package version actually differs between {gitRef} and the working tree; {fallbackSuffix}"); + return new CpmScope([], idsSummary, $"Directory.Packages.props changed but no package version actually differs between {gitRef} and the working tree; {fallbackSuffix}"); } - var selected = AffectedTestFinder.FindTestProjectsUsingPackages(solution, changedIds); - if (selected.Count == 0) + var impact = AffectedTestFinder.FindTestProjectsForPackageChange(solution, changedIds, assetsCache); + if (impact.UnrestoredProjectPath is { } unrestored) + { + return new CpmScope([], idsSummary, $"Directory.Packages.props changed but {Path.GetFileName(unrestored)} has no obj/project.assets.json (not restored), so package usage can't be checked for the whole solution; {fallbackSuffix}"); + } + if (impact.TestProjects.Count == 0) { - return new CpmScope([], $"Directory.Packages.props changed ({string.Join(", ", changedIds.OrderBy(x => x, StringComparer.OrdinalIgnoreCase))}) but no restored test project references those packages; {fallbackSuffix}"); + return new CpmScope([], idsSummary, $"Directory.Packages.props changed ({idsSummary}) but no restored project references those packages; {fallbackSuffix}"); } - return new CpmScope(selected, $"Directory.Packages.props changed: {string.Join(", ", changedIds.OrderBy(x => x, StringComparer.OrdinalIgnoreCase))}; running the test projects that use them."); + return new CpmScope(impact.TestProjects, idsSummary, $"Directory.Packages.props changed: {idsSummary}; running the test projects that use them."); } /// @@ -370,7 +411,10 @@ private static async Task TryCentralPackageManagementScopeAsync( /// version between two versions of a Directory.Packages.props file's XML (namespace-agnostic: an SDK-style props /// file declares no xmlns, but this tolerates one if present). Pure and synchronous, so it's unit-testable /// without git or the filesystem. Null - not throwing - when either document fails to parse as XML: the caller - /// falls back to today's whole-solution behavior for a props file it can't safely diff. + /// falls back to today's whole-solution behavior for a props file it can't safely diff. Reports only the id + /// diff - whether anything ELSE in the file also changed is a separate question, answered by + /// , which the caller checks first before trusting this diff enough to + /// narrow on it. /// public static IReadOnlySet? DiffPackageVersions(string oldXml, string newXml) { @@ -390,6 +434,48 @@ private static async Task TryCentralPackageManagementScopeAsync( return changed; } + /// + /// Whether two Directory.Packages.props XML documents differ ONLY in the Version attribute values of their + /// <PackageVersion> elements - the only difference is safe to narrow on. + /// A <PackageReference>, <GlobalPackageReference>, a Condition, a property like + /// ManagePackageVersionsCentrally, or any other structural change riding along with a version bump makes this + /// false, since none of those are covered by the id diff and narrowing on it anyway could miss whatever that + /// other change affects. Comments and whitespace-only text nodes are stripped before comparing, so reformatting + /// or a comment edit alongside a version bump doesn't count as "beyond versions". Null - not throwing - when + /// either document fails to parse as XML. + /// + public static bool? IsOnlyPackageVersionChange(string oldXml, string newXml) + { + var oldNormalized = TryNormalizeIgnoringPackageVersions(oldXml); + var newNormalized = TryNormalizeIgnoringPackageVersions(newXml); + if (oldNormalized is null || newNormalized is null) return null; + return XNode.DeepEquals(oldNormalized, newNormalized); + } + + /// Parses XML and strips comments, whitespace-only text nodes, and the Version attribute of every + /// <PackageVersion> element (namespace-agnostic, matching ), leaving + /// only what should actually compare. Null on a parse failure. + private static XDocument? TryNormalizeIgnoringPackageVersions(string xml) + { + XDocument doc; + try + { + doc = XDocument.Parse(xml, LoadOptions.None); + } + catch (System.Xml.XmlException) + { + return null; + } + + doc.DescendantNodes().OfType().Remove(); + doc.DescendantNodes().OfType().Where(t => string.IsNullOrWhiteSpace(t.Value)).Remove(); + foreach (var element in doc.Descendants().Where(e => e.Name.LocalName == "PackageVersion")) + { + element.Attributes().FirstOrDefault(a => a.Name.LocalName == "Version")?.Remove(); + } + return doc; + } + private static Dictionary? TryParsePackageVersions(string xml) { try diff --git a/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs index 8555d01..9fe03f8 100644 --- a/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs +++ b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs @@ -83,6 +83,40 @@ public void Malformed_assets_json_does_not_throw_and_creates_no_edge() Assert.Empty(AffectedTestFinder.FindAffectedTestProjects(solution, [libACs])); } + [Theory] + [InlineData("""{ "targets": [] }""")] // "targets" is an array, not an object + [InlineData("""{ "targets": { "net10.0": [] } }""")] // a TFM entry is an array, not an object + [InlineData("[1,2]")] // the whole document isn't an object + public void Unexpectedly_shaped_but_valid_json_does_not_throw_and_creates_no_edge(string assetsJson) + { + var (solution, libACs) = Build(libAProjectXml: DefaultCsproj, assetsJson: assetsJson); + + var thrown = Record.Exception(() => AffectedTestFinder.FindAffectedTestProjects(solution, [libACs])); + + Assert.Null(thrown); + Assert.Empty(AffectedTestFinder.FindAffectedTestProjects(solution, [libACs])); + } + + [Fact] + public void AssetsCache_reads_and_parses_a_project_s_assets_file_at_most_once() + { + var (_, libACs) = Build(libAProjectXml: DefaultCsproj, assetsJson: AssetsJson(("LibA", "package"))); + var testsCsprojPath = Path.Combine(_root, "test", "LibA.Tests.csproj"); + File.WriteAllText(testsCsprojPath, DefaultCsproj); + var objDir = Path.Combine(_root, "test", "obj"); + Directory.CreateDirectory(objDir); + File.WriteAllText(Path.Combine(objDir, "project.assets.json"), AssetsJson(("Foo", "package"))); + + var cache = new AffectedTestFinder.AssetsCache(); + var first = cache.Get(testsCsprojPath); + var second = cache.Get(testsCsprojPath); + + // ReadAssetsPackageIds allocates a fresh HashSet on every real read; getting back the exact same instance + // proves the second Get() was served from the cache instead of re-reading and re-parsing the file. + Assert.NotNull(first); + Assert.Same(first, second); + } + [Fact] public void CountUnrestoredTestProjects_counts_test_projects_with_no_assets_file() { diff --git a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs new file mode 100644 index 0000000..6d73e91 --- /dev/null +++ b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs @@ -0,0 +1,358 @@ +// Copyright (c) 2025 Ahmed Mustafa + +using System.Diagnostics; +using DotNetDevMCP.CodeIntelligence.Interfaces; +using DotNetDevMCP.Testing.Mcp.Tools; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.MSBuild; +using Microsoft.CodeAnalysis.Text; +using Microsoft.Extensions.Logging.Abstractions; + +namespace DotNetDevMCP.Testing.Tests; + +/// +/// Code-review fixes for the Central Package Management precision in , +/// exercised end to end (dryRun) against a real throwaway git repository rather than through the pure XML-diff +/// helpers alone: the scope decision depends on `git show` against a committed baseline, and several of the bugs +/// found in review (BLOCKER 1, MAJOR 2, MAJOR 4, MAJOR 5) only show up once the whole method runs. The repo lives +/// under a temp folder whose name contains a space, since an argument-list git invocation is exactly what a +/// string-concatenation regression would break on such a path. +/// +public sealed class RunAffectedCpmReviewFixTests : IDisposable +{ + private readonly string _root = Path.Combine(Path.GetTempPath(), "dotnetdevmcp cpm tests", Guid.NewGuid().ToString("N")); + + public void Dispose() + { + if (!Directory.Exists(_root)) return; + try + { + // git marks its object files read-only on Windows; clear that before recursive delete or it throws + // UnauthorizedAccessException instead of removing them. + foreach (var file in Directory.EnumerateFiles(_root, "*", SearchOption.AllDirectories)) + { + try { File.SetAttributes(file, FileAttributes.Normal); } catch (IOException) { } + } + Directory.Delete(_root, recursive: true); + } + catch (IOException) { /* best effort */ } + catch (UnauthorizedAccessException) { /* best effort */ } + } + + [Fact] + public async Task Blocker1_props_change_alongside_a_traced_file_change_runs_the_whole_solution_not_just_the_package_scope() + { + var repo = BuildRepo(); + // Make LibA.Tests' OWN assets reference the changed package too (not just LibA's, via the source-project + // path MAJOR 2 adds): the point of this test is that a SECOND changed file must force the whole solution + // regardless of how the package is reachable, narrowly or broadly. + File.WriteAllText(Path.Combine(Path.GetDirectoryName(repo.LibATestsCsproj)!, "obj", "project.assets.json"), + AssetsJson(("Priv.Analyzer", "package"))); + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); // Priv.Analyzer bump + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath, repo.LibASourcePath]); + + // Before the fix: cpmOnly triggered on ANY build-wide-only untraced set, so this whole-solution-worthy change + // (a props bump AND a traced .cs edit) was narrowed to just the package-reachable projects, silently dropping + // whatever the .cs edit alone would have reached outside that package's reverse graph. + Assert.Equal("solution", Prop(result, "RanScope")); + var note = Prop(result, "Note")!; + Assert.DoesNotContain("Directory.Packages.props changed:", note); + var testProjectsRun = Prop>(result, "TestProjectsRun")!.ToList(); + Assert.Contains(Path.GetFileName(repo.LibATestsCsproj), testProjectsRun); + Assert.Contains(Path.GetFileName(repo.OtherTestsCsproj), testProjectsRun); + } + + [Fact] + public async Task Major2And4_a_package_only_a_source_projects_assets_reference_still_selects_its_test_project_but_skips_the_helper_project() + { + var repo = BuildRepo(); + // Priv.Analyzer appears only in LibA's own assets (simulating PrivateAssets="all": it never flows to a + // downstream test project's assets), reached from LibA.Tests and TestUtils only via ProjectReference. + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("projects", Prop(result, "RanScope")); + var testProjectsRun = Prop>(result, "TestProjectsRun")!.ToList(); + Assert.Equal([Path.GetFileName(repo.LibATestsCsproj)], testProjectsRun); + Assert.DoesNotContain(Path.GetFileName(repo.TestUtilsCsproj), testProjectsRun); // no [Fact]: not runnable + Assert.DoesNotContain(Path.GetFileName(repo.OtherTestsCsproj), testProjectsRun); // unrelated package + Assert.Contains("Priv.Analyzer", Prop(result, "Note")); + } + + [Fact] + public async Task Major4_a_narrowed_set_covering_every_runnable_test_project_runs_the_whole_solution_instead() + { + var repo = BuildRepo(includeOtherTests: false); // only LibA.Tests is a runnable test project now + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("solution", Prop(result, "RanScope")); + Assert.Contains("every runnable test project", Prop(result, "Note")); + } + + [Fact] + public async Task Major3_a_non_version_change_riding_along_with_a_version_bump_is_not_narrowed() + { + var repo = BuildRepo(); + var withCondition = PropsXml("1.1.0", "1.0.0") + .Replace("", + ""); + File.WriteAllText(repo.PropsPath, withCondition); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("solution", Prop(result, "RanScope")); + Assert.Contains("Directory.Packages.props changed beyond package versions", Prop(result, "Note")); + } + + [Fact] + public async Task Major3_comments_and_whitespace_riding_along_with_a_version_bump_do_not_block_narrowing() + { + var repo = BuildRepo(); + var reformatted = PropsXml("1.1.0", "1.0.0").Replace("", "\n \n\n "); + File.WriteAllText(repo.PropsPath, reformatted); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("projects", Prop(result, "RanScope")); + Assert.Contains("Priv.Analyzer", Prop(result, "Note")); + } + + [Fact] + public async Task Major5_a_solution_project_with_no_assets_file_anywhere_blocks_narrowing() + { + var repo = BuildRepo(); + File.Delete(Path.Combine(Path.GetDirectoryName(repo.OtherTestsCsproj)!, "obj", "project.assets.json")); + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("solution", Prop(result, "RanScope")); + var note = Prop(result, "Note")!; + Assert.Contains("not restored", note); + Assert.Contains("Other.Tests.csproj", note); + } + + [Fact] + public async Task Minor7_the_project_reachability_fallback_note_says_package_edges_need_a_restore() + { + var repo = BuildRepo(); + + // The .csproj file itself: untraced (not a document the Roslyn walk can trace) but not build-wide, so this + // takes the general project-reachability fallback, not the CPM path. + var result = await CallRunAffected(repo.Solution, [repo.LibACsproj]); + + Assert.Equal("projects", Prop(result, "RanScope")); + Assert.Contains( + "a test project that uses the changed code through a NuGet package is found only if it has been " + + "restored (obj/project.assets.json); cross-repository consumers are not found.", + Prop(result, "Note")); + var testProjectsRun = Prop>(result, "TestProjectsRun")!.ToList(); + Assert.Equal([Path.GetFileName(repo.LibATestsCsproj)], testProjectsRun); + } + + [Fact] + public async Task Minor8_a_lone_dll_outside_any_project_still_gets_the_binary_reference_sentence() + { + var repo = BuildRepo(); + var dllPath = Path.Combine(_root, "somewhere.dll"); // repo root: outside every project folder, not build-wide + + var result = await CallRunAffected(repo.Solution, [dllPath]); + + Assert.Equal("No changed code or project files. Binary references (.dll) are not traced.", Prop(result, "Message")); + } + + private static T? Prop(object obj, string name) + { + var value = obj.GetType().GetProperty(name)?.GetValue(obj) ?? throw new InvalidOperationException($"No property '{name}' on {obj.GetType()}"); + return (T)value; + } + + private static async Task CallRunAffected(Solution solution, string[] changedFiles, string? gitBase = null) + { + var manager = new FakeSolutionManager(solution); + var finder = new AffectedTestFinder(manager, NullLogger.Instance); + var runner = new TestRunner(); + return await TestingTools.RunAffected(runner, finder, manager, NullLogger.Instance, + changedFiles: changedFiles, gitBase: gitBase, dryRun: true); + } + + private const string DefaultCsproj = """ + + + net10.0 + + + """; + + private static string PropsXml(string privAnalyzerVersion, string unrelatedVersion) => $""" + + + true + + + + + + + """; + + private static string AssetsJson(params (string Id, string Type)[] entries) + { + var targets = string.Join(",\n", entries.Select(e => $$""" + "{{e.Id}}/1.0.0": { "type": "{{e.Type}}" } + """)); + return $$""" + { + "targets": { + "net10.0": { + {{targets}} + } + } + } + """; + } + + private static void Git(string dir, params string[] args) + { + var psi = new ProcessStartInfo("git") + { + WorkingDirectory = dir, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }; + foreach (var a in args) psi.ArgumentList.Add(a); + using var process = Process.Start(psi) ?? throw new InvalidOperationException("Failed to start git."); + var stderr = process.StandardError.ReadToEnd(); + process.StandardOutput.ReadToEnd(); + process.WaitForExit(15_000); + if (process.ExitCode != 0) throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {stderr}"); + } + + /// The on-disk + in-memory pieces of one throwaway repo: LibA (a library carrying the "private" package + /// directly in its own assets), LibA.Tests (references LibA via ProjectReference only - no direct package + /// reference of its own), TestUtils (also references LibA, but declares no [Fact] - a helper `dotnet test` can't + /// run), and optionally Other.Tests (unrelated, its own unrelated package, no path to LibA). + private sealed record Repo(Solution Solution, string PropsPath, string LibACsproj, string LibASourcePath, + string LibATestsCsproj, string TestUtilsCsproj, string OtherTestsCsproj); + + private Repo BuildRepo(bool includeOtherTests = true) + { + Directory.CreateDirectory(_root); + var propsPath = Path.Combine(_root, "Directory.Packages.props"); + File.WriteAllText(propsPath, PropsXml("1.0.0", "1.0.0")); + + var libADir = Path.Combine(_root, "LibA"); + Directory.CreateDirectory(Path.Combine(libADir, "obj")); + var libACsproj = Path.Combine(libADir, "LibA.csproj"); + File.WriteAllText(libACsproj, DefaultCsproj); + File.WriteAllText(Path.Combine(libADir, "obj", "project.assets.json"), AssetsJson(("Priv.Analyzer", "package"))); + var libASourcePath = Path.Combine(libADir, "A.cs"); + File.WriteAllText(libASourcePath, "namespace LibA; public class A { public int M() => 1; }"); + + var libATestsDir = Path.Combine(_root, "LibA.Tests"); + Directory.CreateDirectory(Path.Combine(libATestsDir, "obj")); + var libATestsCsproj = Path.Combine(libATestsDir, "LibA.Tests.csproj"); + File.WriteAllText(libATestsCsproj, DefaultCsproj); + File.WriteAllText(Path.Combine(libATestsDir, "obj", "project.assets.json"), AssetsJson()); // no direct package reference + + var testUtilsDir = Path.Combine(_root, "TestUtils"); + Directory.CreateDirectory(Path.Combine(testUtilsDir, "obj")); + var testUtilsCsproj = Path.Combine(testUtilsDir, "TestUtils.csproj"); + File.WriteAllText(testUtilsCsproj, DefaultCsproj); + File.WriteAllText(Path.Combine(testUtilsDir, "obj", "project.assets.json"), AssetsJson()); + + string otherTestsCsproj = ""; + string? otherTestsDir = null; + if (includeOtherTests) + { + otherTestsDir = Path.Combine(_root, "Other.Tests"); + Directory.CreateDirectory(Path.Combine(otherTestsDir, "obj")); + otherTestsCsproj = Path.Combine(otherTestsDir, "Other.Tests.csproj"); + File.WriteAllText(otherTestsCsproj, DefaultCsproj); + File.WriteAllText(Path.Combine(otherTestsDir, "obj", "project.assets.json"), AssetsJson(("Unrelated.Package", "package"))); + } + + Git(_root, "init", "--initial-branch=main"); + Git(_root, "config", "user.email", "test@example.com"); + Git(_root, "config", "user.name", "Test"); + Git(_root, "add", "-A"); + Git(_root, "commit", "-m", "init"); + + var workspace = new AdhocWorkspace(); + var corlib = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); + var runtime = MetadataReference.CreateFromFile(Path.Combine(Path.GetDirectoryName(typeof(object).Assembly.Location)!, "System.Runtime.dll")); + var xunit = MetadataReference.CreateFromFile(typeof(FactAttribute).Assembly.Location); + + var slnPath = Path.Combine(_root, "Test.sln"); + var solution = workspace.AddSolution(SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Default, slnPath, [])); + + var libAId = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(libAId, VersionStamp.Default, "LibA", "LibA", LanguageNames.CSharp, + filePath: libACsproj, metadataReferences: [corlib, runtime])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(libAId), "A.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From("namespace LibA; public class A { public int M() => 1; }"), VersionStamp.Default)), + filePath: libASourcePath)); + + var libATestsId = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(libATestsId, VersionStamp.Default, "LibA.Tests", "LibA.Tests", LanguageNames.CSharp, + filePath: libATestsCsproj, metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(libATestsId, new ProjectReference(libAId)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(libATestsId), "T.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From( + "using Xunit; namespace LibA.Tests; public class T { [Fact] public void T1() { _ = new LibA.A().M(); } }"), VersionStamp.Default)), + filePath: Path.Combine(libATestsDir, "T.cs"))); + + var testUtilsId = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(testUtilsId, VersionStamp.Default, "TestUtils", "TestUtils", LanguageNames.CSharp, + filePath: testUtilsCsproj, metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(testUtilsId, new ProjectReference(libAId)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(testUtilsId), "Fakes.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From( + "namespace TestUtils; public static class Fakes { public static int One() => 1; }"), VersionStamp.Default)), // no [Fact]: not runnable + filePath: Path.Combine(testUtilsDir, "Fakes.cs"))); + + if (includeOtherTests) + { + var otherId = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(otherId, VersionStamp.Default, "Other.Tests", "Other.Tests", LanguageNames.CSharp, + filePath: otherTestsCsproj, metadataReferences: [corlib, runtime, xunit])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(otherId), "T.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From( + "using Xunit; namespace Other.Tests; public class T { [Fact] public void T1() { } }"), VersionStamp.Default)), + filePath: Path.Combine(otherTestsDir!, "T.cs"))); + } + + return new Repo(solution, propsPath, libACsproj, libASourcePath, libATestsCsproj, testUtilsCsproj, otherTestsCsproj); + } + + /// Minimal ISolutionManager wrapping a hand-built Solution: RunAffected only ever reads CurrentSolution + /// and IsSolutionLoaded from it in the dryRun path these tests exercise. + private sealed class FakeSolutionManager(Solution solution) : ISolutionManager + { + public bool IsSolutionLoaded => true; + public MSBuildWorkspace? CurrentWorkspace => null; + public Solution? CurrentSolution => solution; + public Task LoadSolutionAsync(string solutionPath, CancellationToken cancellationToken) => throw new NotSupportedException(); + public void UnloadSolution() { } + public Task FindRoslynSymbolAsync(string fullyQualifiedName, CancellationToken cancellationToken) => throw new NotSupportedException(); + public Task FindRoslynNamedTypeSymbolAsync(string fullyQualifiedTypeName, CancellationToken cancellationToken) => throw new NotSupportedException(); + public Task FindReflectionTypeAsync(string fullyQualifiedTypeName, CancellationToken cancellationToken) => throw new NotSupportedException(); + public Task> SearchReflectionTypesAsync(string regexPattern, CancellationToken cancellationToken) => throw new NotSupportedException(); + public IEnumerable GetProjects() => solution.Projects; + public Project? GetProjectByName(string projectName) => solution.Projects.FirstOrDefault(p => p.Name == projectName); + public Task GetSemanticModelAsync(DocumentId documentId, CancellationToken cancellationToken) => throw new NotSupportedException(); + public Task GetCompilationAsync(ProjectId projectId, CancellationToken cancellationToken) => throw new NotSupportedException(); + public Task ReloadSolutionFromDiskAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public void RefreshCurrentSolution() { } + public void Dispose() { } + } +} From ecb6ed9140562075e8ce2a1a24f7b5c12c0551dd Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 18:15:15 +0300 Subject: [PATCH 3/4] NuGet edges: round-2 review fixes - An unreadable or unexpectedly shaped project.assets.json is 'unknown', not 'uses no packages', so it blocks Directory.Packages.props narrowing. - git show output is decoded as UTF-8 (non-ASCII props no longer defeat narrowing); BOM trimmed. - Note distinguishes 'used, but no runnable test project reaches it'. - Reading Directory.Packages.props: UnauthorizedAccessException falls back. - Documentation extensions are ignored only outside every project folder: test data inside a project (TestData/expected.txt) now selects its tests. - Doc comments and README wording match the behavior. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++ README.md | 2 +- .../AffectedTestFinder.cs | 20 +++--- .../Mcp/Tools/TestingTools.cs | 22 ++++--- src/DotNetDevMCP.Testing/TestRunner.cs | 4 +- .../RunAffectedCpmReviewFixTests.cs | 61 +++++++++++++++++++ 6 files changed, 97 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9de21a..4f167b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,11 @@ All notable changes to DotNetDevMCP are documented here. The format follows changed, an unrestored project, or no restored project uses those ids). Test projects with no `obj/project.assets.json` (not restored) are called out in the note when the run falls back to whole test projects. +### Fixed +- `dotnet_test_affected`: a changed `.txt`, `.png` or other "documentation" file inside a project folder (test data such as + `TestData/expected.txt`) was ignored, so nothing ran. Only such files outside every project are ignored now; inside a + project they select that project's tests. + ## [0.3.3] - 2026-09-24 Prompted by an external evaluation; each claim was checked against the code first. diff --git a/README.md b/README.md index 12f1d3e..3ca0ae2 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,7 @@ Measured with BenchmarkDotNet on an i7-10750H, .NET 10.0.9. The orchestration be After an edit, the agent usually reruns the whole suite. `dotnet_test_affected` asks Roslyn instead: take the symbols declared in the changed files, follow references (up to `maxDepth` hops, default 8) until you land in a method with `[Fact]`, `[Theory]`, `[Test]`, `[TestCase]` or `[TestMethod]`, then run exactly those. Changed files default to the git working tree, or `gitBase: "main"` for a branch. `dryRun: true` lists the tests without running them; `framework: "net10.0"` runs one target framework of multi-targeted test projects. -The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). The project fallback also follows restored NuGet package references (a test project whose `obj/project.assets.json` references another solution project's package id, with no `ProjectReference` between them, is still selected). A change to `Directory.Packages.props` is narrowed to the test projects that actually use the package ids whose version moved only when it is the *only* changed file, only the `Version` attributes actually changed (not a `Condition`, a `GlobalPackageReference`, or any other edit riding along), and every solution project has been restored; any other change to that file, a change alongside another file, or an unrestored project anywhere in the solution runs the whole solution instead, with a note explaining which of those applied. +The walk has a time budget (`maxSelectionSeconds`, default 10). A change to code that everything depends on reaches too much to trace cheaply; then the test projects that reference the changed projects run instead (the whole solution if that's all of them), and the response says so (`selectionComplete: false`, `ranScope`). The same happens when the selection is more than 20% of all tests (`maxSelectedFraction`), where a filtered run is no faster. Changed files the walk can't trace (a `.csproj`, `.razor`, `appsettings.json`, a deleted file) switch to the same project fallback and are listed in `untracedFiles`; a changed `Directory.Build.props`, `global.json` or `.editorconfig` runs the whole solution. You never get a silently partial selection. Runs are killed after `timeoutSeconds` (default 600) so a hanging test cannot hang the agent; the response names the test modules that never finished. `maxDepth: 3` narrows more changes but misses more tests. Works with VSTest and with Microsoft.Testing.Platform (`"test": { "runner": "Microsoft.Testing.Platform" }` in global.json). The project fallback also follows restored NuGet package references (a test project whose `obj/project.assets.json` references another solution project's package id, with no `ProjectReference` between them, is still selected). A change to `Directory.Packages.props` is narrowed to the test projects that actually use the package ids whose version moved only when it is the *only* changed file (documentation outside every project aside), only the `Version` attributes actually changed (not a `Condition`, a `GlobalPackageReference`, or any other edit riding along), and every solution project has been restored; any other change to that file, a change alongside another file, or an unrestored project anywhere in the solution runs the whole solution instead, with a note explaining which of those applied. On this repository, editing `ConcurrentExecutor.cs` selects 22 of 44 tests (the `ConcurrentExecutorTests` plus the `OrchestrationServiceTests` that reach it through `OrchestrationService`). Measured through the MCP tool, build included, i7-10750H: diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index 226ae4a..1c8eb09 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -295,7 +295,7 @@ private static void AddPackageEdges(Solution solution, DictionaryResult of : either the runnable test projects reachable /// from the change, or - when UnrestoredProjectPath is set - a signal that narrowing isn't safe because that /// project's restore state is unknown (TestProjects is then always empty). - public sealed record PackageChangeImpact(IReadOnlyList TestProjects, string? UnrestoredProjectPath); + public sealed record PackageChangeImpact(IReadOnlyList TestProjects, string? UnrestoredProjectPath, IReadOnlyList? UsingProjects = null); /// /// Central Package Management precision support: the runnable test projects reachable from a change to the given @@ -343,7 +343,9 @@ public static PackageChangeImpact FindTestProjectsForPackageChange(Solution solu var testProjects = reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); - return new PackageChangeImpact(testProjects, null); + var usingProjects = matched.Select(solution.GetProject).OfType().Select(p => p.FilePath).OfType() + .Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + return new PackageChangeImpact(testProjects, null, usingProjects); } /// @@ -443,19 +445,19 @@ private static string GetPackageId(Project project, string? solutionDir) /// JsonElement access is guarded by a ValueKind check first, since JsonElement's Get/TryGetProperty and /// EnumerateObject throw InvalidOperationException on the wrong kind rather than returning false. /// - private static HashSet ReadAssetsPackageIds(string assetsJsonPath) + private static HashSet? ReadAssetsPackageIds(string assetsJsonPath) { var ids = new HashSet(StringComparer.OrdinalIgnoreCase); - if (!File.Exists(assetsJsonPath)) return ids; + if (!File.Exists(assetsJsonPath)) return null; try { using var stream = File.OpenRead(assetsJsonPath); using var document = JsonDocument.Parse(stream); - if (document.RootElement.ValueKind != JsonValueKind.Object) return ids; - if (!document.RootElement.TryGetProperty("targets", out var targets) || targets.ValueKind != JsonValueKind.Object) return ids; + if (document.RootElement.ValueKind != JsonValueKind.Object) return null; + if (!document.RootElement.TryGetProperty("targets", out var targets) || targets.ValueKind != JsonValueKind.Object) return null; foreach (var tfm in targets.EnumerateObject()) { - if (tfm.Value.ValueKind != JsonValueKind.Object) continue; + if (tfm.Value.ValueKind != JsonValueKind.Object) return null; foreach (var entry in tfm.Value.EnumerateObject()) { if (entry.Value.ValueKind != JsonValueKind.Object) continue; @@ -467,7 +469,9 @@ private static HashSet ReadAssetsPackageIds(string assetsJsonPath) } catch (Exception ex) when (ex is JsonException or IOException or UnauthorizedAccessException) { - return new HashSet(StringComparer.OrdinalIgnoreCase); + // Unknown, not empty: a half-written or locked assets file must not read as "uses no packages", or the + // Directory.Packages.props narrowing could drop a test project that does use the changed package. + return null; } return ids; } diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index c768b0f..c993e39 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -87,7 +87,10 @@ public static async Task RunAffected( var files = changedFiles is { Length: > 0 } ? changedFiles : await GitChangedFilesAsync(solutionDir, gitBase, cancellationToken); logger.LogDebug("dotnet_test_affected: resolved {Count} changed files in {Ms} ms", files.Length, sw.ElapsedMilliseconds); var solution = solutions.CurrentSolution; - var changed = files.Select(f => Path.GetFullPath(Path.Combine(solutionDir, f))).Where(f => !DocumentationExtensions.Contains(Path.GetExtension(f))).ToArray(); + // A .txt/.png inside a project folder can be test data (TestData/expected.txt), so only files outside every + // project are dropped as documentation. ponytail: a README.md inside a project now triggers its project fallback. + var changed = files.Select(f => Path.GetFullPath(Path.Combine(solutionDir, f))) + .Where(f => !DocumentationExtensions.Contains(Path.GetExtension(f)) || AffectedTestFinder.OwningProjects(solution, f).Count > 0).ToArray(); // Never traced at any layer (not a document, not a project-owned file, not build-wide): noted, not analyzed. var dllChanged = changed.Where(f => f.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)).ToArray(); // The walk traces only C# the solution compiles. Anything else (a .csproj, .razor, appsettings.json, a deleted file) @@ -257,7 +260,7 @@ public static async Task RunAffected( return new { Success = summary.Success, ChangedFiles = files, UntracedFiles = untraced, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, RanScope = ScopeName(scope), TestProjectsRun = testProjectsRun.Select(Path.GetFileName), AffectedTests = list, Ran = true, Run = Shape(summary) }; } - /// Changes to these can't break a test. ponytail: extension list, not content sniffing. + /// Outside every project folder, changes to these can't break a test. ponytail: extension list, not content sniffing. private static readonly HashSet DocumentationExtensions = new(StringComparer.OrdinalIgnoreCase) { ".md", ".txt", ".png", ".jpg", ".jpeg", ".gif", ".svg" }; /// Files outside any project folder that still feed every build. @@ -348,7 +351,7 @@ private static async Task TryCentralPackageManagementScopeAsync( { newXml = await File.ReadAllTextAsync(propsFilePath, ct); } - catch (IOException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { return new CpmScope([], "", $"Directory.Packages.props changed but could not be read; {fallbackSuffix}"); } @@ -364,7 +367,8 @@ private static async Task TryCentralPackageManagementScopeAsync( var relative = Path.GetRelativePath(root, propsFilePath).Replace('\\', '/'); var gitRef = gitBase ?? "HEAD"; - var (showExit, showOut, showErr, _) = await TestRunner.RunProcessAsync("git", ["show", $"{gitRef}:{relative}"], ct, root); + var (showExit, showOut, showErr, _) = await TestRunner.RunProcessAsync("git", ["show", $"{gitRef}:{relative}"], ct, root, outputEncoding: System.Text.Encoding.UTF8); + showOut = showOut.TrimStart(''); if (showExit != 0) { return new CpmScope([], "", $"Directory.Packages.props changed but its previous version ({gitRef}:{relative}) could not be read from git ({showErr.Trim()}); {fallbackSuffix}"); @@ -396,11 +400,14 @@ private static async Task TryCentralPackageManagementScopeAsync( var impact = AffectedTestFinder.FindTestProjectsForPackageChange(solution, changedIds, assetsCache); if (impact.UnrestoredProjectPath is { } unrestored) { - return new CpmScope([], idsSummary, $"Directory.Packages.props changed but {Path.GetFileName(unrestored)} has no obj/project.assets.json (not restored), so package usage can't be checked for the whole solution; {fallbackSuffix}"); + return new CpmScope([], idsSummary, $"Directory.Packages.props changed but {Path.GetFileName(unrestored)} has no readable obj/project.assets.json (not restored or unreadable), so package usage can't be checked for the whole solution; {fallbackSuffix}"); } if (impact.TestProjects.Count == 0) { - return new CpmScope([], idsSummary, $"Directory.Packages.props changed ({idsSummary}) but no restored project references those packages; {fallbackSuffix}"); + var reason = impact.UsingProjects is { Count: > 0 } users + ? $"they are used by {string.Join(", ", users.Select(Path.GetFileName))}, but no runnable test project reaches those" + : "no restored project references those packages"; + return new CpmScope([], idsSummary, $"Directory.Packages.props changed ({idsSummary}) but {reason}; {fallbackSuffix}"); } return new CpmScope(impact.TestProjects, idsSummary, $"Directory.Packages.props changed: {idsSummary}; running the test projects that use them."); @@ -408,7 +415,8 @@ private static async Task TryCentralPackageManagementScopeAsync( /// /// Package ids whose <PackageVersion Include="X" Version="V" /> entry was added, removed, or changed - /// version between two versions of a Directory.Packages.props file's XML (namespace-agnostic: an SDK-style props + /// version (only a changed version can lead to narrowing: an added or removed entry already fails + /// , so the caller runs the whole solution) between two versions of a Directory.Packages.props file's XML (namespace-agnostic: an SDK-style props /// file declares no xmlns, but this tolerates one if present). Pure and synchronous, so it's unit-testable /// without git or the filesystem. Null - not throwing - when either document fails to parse as XML: the caller /// falls back to today's whole-solution behavior for a props file it can't safely diff. Reports only the id diff --git a/src/DotNetDevMCP.Testing/TestRunner.cs b/src/DotNetDevMCP.Testing/TestRunner.cs index 111b47a..159ba8b 100644 --- a/src/DotNetDevMCP.Testing/TestRunner.cs +++ b/src/DotNetDevMCP.Testing/TestRunner.cs @@ -372,12 +372,14 @@ private static string Tail(string s, int lines = 40) /// quoting and nothing can smuggle in extra arguments the way concatenating a single argument string would allow. /// internal static async Task<(int ExitCode, string Stdout, string Stderr, bool TimedOut)> RunProcessAsync( - string fileName, IReadOnlyList arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null) + string fileName, IReadOnlyList arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null, + System.Text.Encoding? outputEncoding = null) { var psi = new ProcessStartInfo(fileName) { RedirectStandardInput = true, RedirectStandardOutput = true, + StandardOutputEncoding = outputEncoding, RedirectStandardError = true, UseShellExecute = false, CreateNoWindow = true, diff --git a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs index 6d73e91..15110ea 100644 --- a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs +++ b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs @@ -165,6 +165,67 @@ public async Task Minor8_a_lone_dll_outside_any_project_still_gets_the_binary_re Assert.Equal("No changed code or project files. Binary references (.dll) are not traced.", Prop(result, "Message")); } + [Fact] + public async Task Round2_N1_an_unreadable_assets_file_blocks_narrowing_instead_of_reading_as_no_packages() + { + var repo = BuildRepo(); + // Half-written restore: Other.Tests' assets can't be parsed, so whether it uses Priv.Analyzer is unknown. + File.WriteAllText(Path.Combine(Path.GetDirectoryName(repo.OtherTestsCsproj)!, "obj", "project.assets.json"), "{\"targets\": {\"net10.0\": {"); + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("solution", Prop(result, "RanScope")); + var note = Prop(result, "Note")!; + Assert.Contains("Other.Tests.csproj", note); + Assert.Contains("not restored or unreadable", note); + } + + [Fact] + public async Task Round2_N2_non_ascii_text_outside_comments_still_narrows_a_version_only_bump() + { + var repo = BuildRepo(); + var withAuthors = PropsXml("1.0.0", "1.0.0").Replace("", " Müller"); + File.WriteAllText(repo.PropsPath, withAuthors); + Git(_root, "commit", "-am", "authors"); + File.WriteAllText(repo.PropsPath, withAuthors.Replace("Include=\"Priv.Analyzer\" Version=\"1.0.0\"", "Include=\"Priv.Analyzer\" Version=\"1.1.0\"")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("projects", Prop(result, "RanScope")); + Assert.Equal([Path.GetFileName(repo.LibATestsCsproj)], Prop>(result, "TestProjectsRun")!); + } + + [Fact] + public async Task Round2_N3_a_package_used_only_by_a_project_no_test_reaches_says_so() + { + var repo = BuildRepo(includeOtherTests: false); + File.WriteAllText(Path.Combine(Path.GetDirectoryName(repo.TestUtilsCsproj)!, "obj", "project.assets.json"), AssetsJson(("Unrelated.Package", "package"))); + File.WriteAllText(repo.PropsPath, PropsXml("1.0.0", "1.1.0")); + + var result = await CallRunAffected(repo.Solution, [repo.PropsPath]); + + Assert.Equal("solution", Prop(result, "RanScope")); + Assert.Contains("used by TestUtils.csproj, but no runnable test project reaches those", Prop(result, "Note")); + } + + [Fact] + public async Task Round2_N5_test_data_inside_a_project_counts_as_a_change() + { + var repo = BuildRepo(); + var testData = Path.Combine(Path.GetDirectoryName(repo.OtherTestsCsproj)!, "TestData", "expected.txt"); + + // Alone: a .txt inside a test project is test data, not documentation, so that project runs. + var alone = await CallRunAffected(repo.Solution, [testData]); + Assert.Equal("projects", Prop(alone, "RanScope")); + Assert.Equal([Path.GetFileName(repo.OtherTestsCsproj)], Prop>(alone, "TestProjectsRun")!); + + // With a props bump it's a second changed file, so the props narrowing doesn't apply. + File.WriteAllText(repo.PropsPath, PropsXml("1.1.0", "1.0.0")); + var both = await CallRunAffected(repo.Solution, [repo.PropsPath, testData]); + Assert.Equal("solution", Prop(both, "RanScope")); + } + private static T? Prop(object obj, string name) { var value = obj.GetType().GetProperty(name)?.GetValue(obj) ?? throw new InvalidOperationException($"No property '{name}' on {obj.GetType()}"); From 02f5bb111a5b9af27ea272035f44033daac157eb Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 18:50:57 +0300 Subject: [PATCH 4/4] NuGet edges: round-3 review fixes - .md files never count as changes; other documentation extensions count only inside a project that isn't at the solution root (a root-level project owns every file by folder). - Tests: root README.md, a .md inside a project, and docs/*.png under a root-level project all run nothing. - Doc comments and notes say 'not restored or unreadable'; BOM literal written as an escape; PackageChangeImpact.UsingProjects documented. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++--- .../AffectedTestFinder.cs | 22 +++++++++--------- .../Mcp/Tools/TestingTools.cs | 23 +++++++++++++++---- .../RunAffectedCpmReviewFixTests.cs | 17 ++++++++++++++ 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f167b8..05ba70d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,9 +37,9 @@ All notable changes to DotNetDevMCP are documented here. The format follows `obj/project.assets.json` (not restored) are called out in the note when the run falls back to whole test projects. ### Fixed -- `dotnet_test_affected`: a changed `.txt`, `.png` or other "documentation" file inside a project folder (test data such as - `TestData/expected.txt`) was ignored, so nothing ran. Only such files outside every project are ignored now; inside a - project they select that project's tests. +- `dotnet_test_affected`: a changed `.txt` or image inside a project folder (test data such as `TestData/expected.txt` or + Verify's `*.verified.txt`) was ignored, so nothing ran. It now selects that project's tests. `.md` files, and + documentation outside every project, are still ignored; a project at the solution root doesn't make docs count. ## [0.3.3] - 2026-09-24 diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index 1c8eb09..4d993a5 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -294,7 +294,8 @@ private static void AddPackageEdges(Solution solution, DictionaryResult of : either the runnable test projects reachable /// from the change, or - when UnrestoredProjectPath is set - a signal that narrowing isn't safe because that - /// project's restore state is unknown (TestProjects is then always empty). + /// project's restore state is unknown (TestProjects is then always empty). UsingProjects: the projects whose assets + /// reference a changed package (null when narrowing stopped early or nothing matched), for the caller's note. public sealed record PackageChangeImpact(IReadOnlyList TestProjects, string? UnrestoredProjectPath, IReadOnlyList? UsingProjects = null); /// @@ -308,7 +309,7 @@ public sealed record PackageChangeImpact(IReadOnlyList TestProjects, str /// to (not just ) so a helper library with no test /// method (referencing xUnit but declaring none, like Polly.TestUtils) is never selected to run. /// UnrestoredProjectPath is set - and TestProjects then empty - the moment ANY solution project (test or not) has - /// no obj/project.assets.json at all: without every project's assets, "this project doesn't use the changed + /// no readable obj/project.assets.json: without every project's assets, "this project doesn't use the changed /// package" can't be told apart from "restore state unknown", so the caller should not narrow. /// public static PackageChangeImpact FindTestProjectsForPackageChange(Solution solution, IReadOnlySet packageIds, AssetsCache assetsCache) @@ -349,8 +350,8 @@ public static PackageChangeImpact FindTestProjectsForPackageChange(Solution solu } /// - /// Test projects (dedupe by file path across TFM variants) with no obj/project.assets.json at all - i.e. never - /// restored - for whom cannot see any package reference. Surfaced in the + /// Test projects (dedupe by file path across TFM variants) with no readable obj/project.assets.json - never + /// restored, or unreadable / not shaped like an assets file - for whom cannot see any package reference. Surfaced in the /// affected-test note so a package-mediated edge that was missed reads as "not restored", not as "this project /// doesn't depend on the change". /// @@ -362,9 +363,9 @@ public static int CountUnrestoredTestProjects(Solution solution, AssetsCache? as } /// - /// Per-call cache of a project's project.assets.json, keyed by project file path: null means the file doesn't - /// exist (the project has never been restored); otherwise the package ids - /// found (possibly empty, for a restored project with no "type":"package" entries or a malformed file). Shared + /// Per-call cache of a project's project.assets.json, keyed by project file path: null means unknown (the file is + /// missing, unreadable, or not shaped like an assets file); otherwise the package ids + /// found (possibly empty, for a restored project with no "type":"package" entries). Shared /// across , and /// within one `dotnet_test_affected` call so each project's assets file /// is read and parsed at most once, even though all three ask about the same projects. @@ -438,10 +439,9 @@ private static string GetPackageId(Project project, string? solutionDir) /// Package ids referenced anywhere in a project.assets.json's "targets" section - every TFM key, since one /// assets file already covers every TFM of a multi-targeted project: "targets" -> <tfm> -> /// "<id>/<version>" entries whose "type" is "package" (as opposed to "project", a ProjectReference - /// restored into the same graph, which is already covered separately). Empty - never throwing - when the file is - /// missing, unreadable, or the JSON is malformed OR simply not shaped like an assets file ("targets" absent, not - /// an object, or a TFM entry that isn't an object): a corrupted, permission-denied, or unexpected-shape lock file - /// just means this edge source contributes nothing, not a hard failure for the whole affected-test walk. Every + /// restored into the same graph, which is already covered separately). Null - never throwing - when the file is + /// missing, unreadable, or the JSON is malformed OR not shaped like an assets file ("targets" absent, not an object, + /// or a TFM entry that isn't an object): unknown, which callers treat like "not restored" rather than "uses no packages". Every /// JsonElement access is guarded by a ValueKind check first, since JsonElement's Get/TryGetProperty and /// EnumerateObject throw InvalidOperationException on the wrong kind rather than returning false. /// diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index c993e39..e0234e0 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -87,10 +87,8 @@ public static async Task RunAffected( var files = changedFiles is { Length: > 0 } ? changedFiles : await GitChangedFilesAsync(solutionDir, gitBase, cancellationToken); logger.LogDebug("dotnet_test_affected: resolved {Count} changed files in {Ms} ms", files.Length, sw.ElapsedMilliseconds); var solution = solutions.CurrentSolution; - // A .txt/.png inside a project folder can be test data (TestData/expected.txt), so only files outside every - // project are dropped as documentation. ponytail: a README.md inside a project now triggers its project fallback. var changed = files.Select(f => Path.GetFullPath(Path.Combine(solutionDir, f))) - .Where(f => !DocumentationExtensions.Contains(Path.GetExtension(f)) || AffectedTestFinder.OwningProjects(solution, f).Count > 0).ToArray(); + .Where(f => !IsIgnoredDocumentation(solution, solutionDir, f)).ToArray(); // Never traced at any layer (not a document, not a project-owned file, not build-wide): noted, not analyzed. var dllChanged = changed.Where(f => f.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)).ToArray(); // The walk traces only C# the solution compiles. Anything else (a .csproj, .razor, appsettings.json, a deleted file) @@ -198,7 +196,7 @@ public static async Task RunAffected( var unrestoredCount = AffectedTestFinder.CountUnrestoredTestProjects(solution, assetsCache); if (unrestoredCount > 0) { - note = $"{note} {unrestoredCount} test project(s) have no obj/project.assets.json (not restored), so package references for them are unknown."; + note = $"{note} {unrestoredCount} test project(s) have no readable obj/project.assets.json (not restored or unreadable), so package references for them are unknown."; } } if (dllChanged.Length > 0) @@ -260,9 +258,24 @@ public static async Task RunAffected( return new { Success = summary.Success, ChangedFiles = files, UntracedFiles = untraced, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, RanScope = ScopeName(scope), TestProjectsRun = testProjectsRun.Select(Path.GetFileName), AffectedTests = list, Ran = true, Run = Shape(summary) }; } - /// Outside every project folder, changes to these can't break a test. ponytail: extension list, not content sniffing. + /// Documentation extensions; see . ponytail: extension list, not content sniffing. private static readonly HashSet DocumentationExtensions = new(StringComparer.OrdinalIgnoreCase) { ".md", ".txt", ".png", ".jpg", ".jpeg", ".gif", ".svg" }; + /// + /// A changed file that can't break a test: .md anywhere, or another documentation extension outside every project + /// folder. A .txt/.png inside a project can be test data (TestData/expected.txt, Verify's *.verified.txt), so it + /// counts. A project at the solution root would "own" every file (docs/, README.md), so its ownership doesn't count. + /// + private static bool IsIgnoredDocumentation(Solution solution, string solutionDir, string path) + { + var ext = Path.GetExtension(path); + if (!DocumentationExtensions.Contains(ext)) return false; + if (ext.Equals(".md", StringComparison.OrdinalIgnoreCase)) return true; + var root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(solutionDir)); + return !AffectedTestFinder.OwningProjects(solution, path).Select(solution.GetProject).OfType() + .Any(p => p.FilePath is { } fp && !string.Equals(Path.GetDirectoryName(Path.GetFullPath(fp)), root, StringComparison.OrdinalIgnoreCase)); + } + /// Files outside any project folder that still feed every build. private static bool IsBuildWideFile(string path) { diff --git a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs index 15110ea..95a1e23 100644 --- a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs +++ b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs @@ -226,6 +226,23 @@ public async Task Round2_N5_test_data_inside_a_project_counts_as_a_change() Assert.Equal("solution", Prop(both, "RanScope")); } + [Theory] + [InlineData("README.md")] // outside every project + [InlineData("Other.Tests/README.md")] // .md is never test data, even inside a project + [InlineData("docs/diagram.png")] // owned only by the root-level project below + public async Task Round3_documentation_changes_run_nothing(string relativePath) + { + var repo = BuildRepo(); + // A project at the solution root "owns" every file by folder; its ownership must not turn docs into code changes. + var rootId = ProjectId.CreateNewId(); + var solution = repo.Solution.AddProject(ProjectInfo.Create(rootId, VersionStamp.Default, "Root", "Root", LanguageNames.CSharp, + filePath: Path.Combine(_root, "Root.csproj"))); + + var result = await CallRunAffected(solution, [Path.Combine(_root, relativePath)]); + + Assert.Equal("No changed code or project files.", Prop(result, "Message")); + } + private static T? Prop(object obj, string name) { var value = obj.GetType().GetProperty(name)?.GetValue(obj) ?? throw new InvalidOperationException($"No property '{name}' on {obj.GetType()}");