diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c17088..05ba70d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,28 @@ 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 `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. + +### Fixed +- `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 Prompted by an external evaluation; each claim was checked against the code first. diff --git a/README.md b/README.md index 624828c..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 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: @@ -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..4d993a5 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; @@ -214,12 +216,30 @@ 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 = BuildDependentsGraph(solution, assetsCache ?? new AssetsCache()); + + var reachable = new HashSet(changedProjectIds); + var frontier = new Queue(changedProjectIds); + 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); + } + + return reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) + .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) { @@ -229,17 +249,231 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, list.Add(project.Id); } } + AddPackageEdges(solution, dependents, assetsCache); + return dependents; + } - var reachable = new HashSet(changedProjectIds); - var frontier = new Queue(changedProjectIds); + /// + /// 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, AssetsCache assetsCache) + { + 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 packageIds = assetsCache.Get(testProjectPath); + if (packageIds is not { 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); + } + } + } + } + + /// Result 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). 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); + + /// + /// 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 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) + { + var projectsByPath = solution.Projects.Where(p => p.FilePath is not null) + .GroupBy(p => p.FilePath!, StringComparer.OrdinalIgnoreCase).ToList(); + + foreach (var group in projectsByPath) + { + if (assetsCache.Get(group.Key) is null) return new PackageChangeImpact([], group.Key); + } + + 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); } - return reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) + var testProjects = reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + var usingProjects = matched.Select(solution.GetProject).OfType().Select(p => p.FilePath).OfType() + .Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + return new PackageChangeImpact(testProjects, null, usingProjects); + } + + /// + /// 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". + /// + 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 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. + /// + 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 + /// 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). 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. + /// + private static HashSet? ReadAssetsPackageIds(string assetsJsonPath) + { + var ids = new HashSet(StringComparer.OrdinalIgnoreCase); + 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 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) return null; + foreach (var entry in tfm.Value.EnumerateObject()) + { + if (entry.Value.ValueKind != JsonValueKind.Object) 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 or UnauthorizedAccessException) + { + // 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 2e90d2d..e0234e0 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; @@ -85,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(); + var changed = files.Select(f => Path.GetFullPath(Path.Combine(solutionDir, f))) + .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) // 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(); @@ -93,7 +98,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(); @@ -114,6 +121,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; @@ -127,13 +135,47 @@ 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."; - var reachableProjects = AffectedTestFinder.FindAffectedTestProjects(solution, files.Concat(untraced)); + // Central Package Management precision: a Directory.Packages.props change is build-wide in general (it can + // 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 (buildWideChange || reachableProjects.Count == 0 || reachableProjects.Count >= allTestProjects.Count) + 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; - 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."; } @@ -143,11 +185,24 @@ 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, assetsCache); + if (unrestoredCount > 0) + { + 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) + { + 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)) @@ -203,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) }; } - /// 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) { @@ -266,4 +336,183 @@ 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. 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 - 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, AffectedTestFinder.AssetsCache assetsCache, CancellationToken ct) + { + const string fallbackSuffix = "running the whole solution instead."; + string newXml; + try + { + newXml = await File.ReadAllTextAsync(propsFilePath, ct); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + 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, 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}"); + } + + 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) + { + // 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([], idsSummary, $"Directory.Packages.props changed but no package version actually differs between {gitRef} and the working tree; {fallbackSuffix}"); + } + + 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 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) + { + 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."); + } + + /// + /// Package ids whose <PackageVersion Include="X" Version="V" /> entry was added, removed, or changed + /// 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 - 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) + { + 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; + } + + /// + /// 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 + { + 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/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/NuGetPackageEdgeTests.cs b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs new file mode 100644 index 0000000..9fe03f8 --- /dev/null +++ b/tests/DotNetDevMCP.Testing.Tests/NuGetPackageEdgeTests.cs @@ -0,0 +1,278 @@ +// 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])); + } + + [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() + { + 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"; +} diff --git a/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs new file mode 100644 index 0000000..95a1e23 --- /dev/null +++ b/tests/DotNetDevMCP.Testing.Tests/RunAffectedCpmReviewFixTests.cs @@ -0,0 +1,436 @@ +// 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")); + } + + [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")); + } + + [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()}"); + 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() { } + } +}