Skip to content

Test WPF inspection across targets and modes - #21

Closed
miloszkukla wants to merge 16 commits into
faze79:mainfrom
miloszkukla:agent/test-injection-matrix
Closed

miloszkukla wants to merge 16 commits into
faze79:mainfrom
miloszkukla:agent/test-injection-matrix

Conversation

@miloszkukla

@miloszkukla miloszkukla commented Aug 14, 2026

Copy link
Copy Markdown

Important

This is the upper PR in a fork-compatible stack. Merge #20, then #18, then #19, then this PR, using merge commits. GitHub cannot set a branch from the contributor fork as the base of an upstream PR, so the diff is temporarily cumulative; merge commits preserve the shared commit IDs and make the later PRs shrink as their dependencies merge.

Summary

  • move the Inspector build/package dependency from Injector to Server while keeping Injector and InjectorHelper on net8.0
  • make the included sample app target net472, net48, and net8.0-windows, with both self-hosted and auto-injection startup paths
  • add an integration matrix covering all three target frameworks, x86/x64, and both inspection modes
  • detect an already-running Inspector through its named pipe when managed-module enumeration is unavailable, preventing repeated Auto-injection
  • add a PowerShell runner that builds both native bootstrapper architectures before executing the matrix and restores neutral sample build state afterward

Why

On .NET Framework, Process.Modules does not reliably expose the managed Inspector assembly. That caused self-hosted applications to be reported as not loaded and made a second attach --auto-inject attempt injection again. The Inspector's existing named pipe is an authoritative readiness signal and now acts as the fallback detection mechanism.

Validation

  • fork workflow run 31774992112: all jobs passed, including the 12-case WPF matrix, publish, NuGet pack, and both payload verifications
  • tests/run-integration-tests.ps1 -SkipNativeBuild: 12/12 final live inspection cases passed
  • all six self-hosted cases assert Loaded (self-hosted)
  • all six Auto-injection cases attach a second time with --auto-inject, assert the already-loaded path, and remain inspectable
  • native bootstrapper: x64 and Win32 builds with the command-line-only v145 override passed with 0 warnings and 0 errors
  • full dotnet test WpfVisualTreeMcp.sln --configuration Release --no-restore: 55 server tests plus 19 Shared tests on each of net472, net48, and net8.0 passed
  • CI runs the live matrix with an isolated x86 .NET 8 runtime and uses current action majors
  • WpfVisualTreeMcp.Shared.Tests: 19 passed on each of net472, net48, and net8.0
  • remaining existing tests: 55 passed
  • the integration theory intentionally skips during ordinary test runs unless the matrix runner supplies its environment
  • no .vcxproj toolset/version change is included

Depends on #19, which is stacked after #18 and #20.

@miloszkukla
miloszkukla marked this pull request as ready for review August 14, 2026 05:13
Copilot AI lite review requested due to automatic review settings August 14, 2026 05:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens end-to-end validation of WPF inspection by expanding supported target frameworks/modes and adding an integration matrix that exercises self-hosted vs auto-injection across x86/x64 and net472/net48/net8.0-windows. It also adjusts packaging/build wiring so the Server owns the Inspector payload needed for injection and adds a named-pipe-based fallback to detect an already-running Inspector when module enumeration is unreliable.

Changes:

  • Add new Shared contract tests (multi-TFM) and new integration tests + PowerShell runner to execute the 12-case inspection matrix.
  • Move/reshape injection payload packaging so Server includes the full .NET Framework Inspector dependency closure and validates required payload files in CI and pack/publish.
  • Improve “Inspector already loaded” detection by probing the Inspector’s named pipe as a fallback signal.

Reviewed changes

Copilot reviewed 28 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
WpfVisualTreeMcp.sln Adds new test projects to the solution.
tests/WpfVisualTreeMcp.Shared.Tests/WpfVisualTreeMcp.Shared.Tests.csproj New multi-TFM Shared tests project definition.
tests/WpfVisualTreeMcp.Shared.Tests/SharedModelsTests.cs Adds basic Shared model smoke tests.
tests/WpfVisualTreeMcp.Shared.Tests/IpcSerializerTests.cs Adds IPC envelope regression tests across TFMs.
tests/WpfVisualTreeMcp.IntegrationTests/WpfVisualTreeMcp.IntegrationTests.csproj New integration test project (net8.0-windows).
tests/WpfVisualTreeMcp.IntegrationTests/IntegrationTheoryAttribute.cs Skips integration theory unless runner-provided env vars exist.
tests/WpfVisualTreeMcp.IntegrationTests/InspectionModeMatrixTests.cs Implements 12-case CLI-driven inspection matrix.
tests/WpfVisualTreeMcp.IntegrationTests/AssemblyInfo.cs Disables test parallelization for integration runs.
tests/run-integration-tests.ps1 Builds native payload + publishes sample matrix and runs integration filter.
src/WpfVisualTreeMcp.Shared/WpfVisualTreeMcp.Shared.csproj Adds net472 target + STJ package for .NET Framework targets.
src/WpfVisualTreeMcp.Server/WpfVisualTreeMcp.Server.csproj Server now owns/validates injection payload content and runtimeconfig naming.
src/WpfVisualTreeMcp.Server/Services/ProcessManager.cs Adds pipe-probing fallback for “Inspector already running” detection.
src/WpfVisualTreeMcp.Inspector/WpfVisualTreeMcp.Inspector.csproj Adds net472 target and aligns .NET Framework WPF references.
src/WpfVisualTreeMcp.Inspector/InspectorService.cs Adds .NET Framework private dependency resolution hook (NET48).
src/WpfVisualTreeMcp.InjectorHelper/WpfVisualTreeMcp.InjectorHelper.csproj Removes an outdated suppression comment.
src/WpfVisualTreeMcp.Injector/WpfVisualTreeMcp.Injector.csproj Simplifies Injector to net8.0 only and removes Inspector reference.
src/WpfVisualTreeMcp.Injector/README.md Updates injector docs to reflect real injection implementation/constraints.
src/WpfVisualTreeMcp.Injector/ProcessInjector.cs Updates payload discovery logic for Inspector DLL location.
samples/SampleWpfApp/SelfHostedInspector.cs Adds helper to start/stop Inspector in self-hosted mode.
samples/SampleWpfApp/SampleWpfApp.csproj Multi-targets sample app and adds platform configurations.
samples/SampleWpfApp/App.xaml.cs Adds environment-variable-controlled self-hosted startup path.
README.md Documents auto-injection mode, requirements, and matrix runner usage.
docs/TOOLS_REFERENCE.md Clarifies scope and points to README/CLI help for full tool list.
docs/GETTING_STARTED.md Updates installation wording and framework support details.
docs/ARCHITECTURE.md Updates Inspector/Injector descriptions to match current capabilities/TFMs.
CLAUDE.md Updates build/test commands and notes integration runner usage.
CHANGELOG.md Adds Unreleased entries for packaging/runtimeconfig fixes.
.github/workflows/release.yml Builds native bootstrapper and verifies injection payload in artifacts/packages.
.github/workflows/publish-mcp-registry.yml Updates checkout action major.
.github/workflows/build.yml Adds native bootstrapper build, installs x86 runtime, runs integration matrix, and verifies payload.
Suppressed comments (1)

src/WpfVisualTreeMcp.Server/Services/ProcessManager.cs:138

  • When the Inspector is detected via loaded modules or by successfully connecting to its named pipe, the code always sets InspectorStatus to "Loaded (self-hosted)". This can be inaccurate for auto-injection scenarios (e.g., a second attach --auto-inject after a prior injection will also hit this branch) and makes the status string misleading for callers/tests.
        // Check if Inspector is already loaded (self-hosted mode)
        var inspectorLoaded = IsInspectorLoaded(targetProcess) ||
            await WaitForInspectorPipeAsync(targetProcess.Id, TimeSpan.FromMilliseconds(700));
        if (inspectorLoaded)
        {
            _logger.LogInformation("Inspector DLL already loaded in target process (self-hosted mode)");
            session.InspectorStatus = "Loaded (self-hosted)";
        }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +375 to +381
foreach (var candidate in candidates)
{
if (File.Exists(candidate))
return candidate;
}

return candidates[1];

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessed in consolidated PR #22 and intentionally unchanged. InjectIntoProcess checks File.Exists before any process work and throws FileNotFoundException with the expected path; ProcessManager handles that exception explicitly, so a missing payload already produces the informative failure.

Comment on lines +102 to +108
using var secondAttachJson = JsonDocument.Parse(secondAttach.StandardOutput);
secondAttachJson.RootElement.GetProperty("success").GetBoolean().Should().BeTrue();
secondAttachJson.RootElement.GetProperty("processId").GetInt32().Should().Be(sample.Id);
secondAttachJson.RootElement.GetProperty("inspectorStatus").GetString()
.Should().Be("Loaded (self-hosted)",
"the second attach should reuse the loaded Inspector without reinjecting it");
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in consolidated PR #22 (commit 35c1177): already-running Inspectors now report the mode-neutral Loaded (existing) status, and the 12-case matrix verifies that a second auto-inject attach follows that path and remains inspectable.

Comment on lines 131 to 134
// Check if Inspector is already loaded (self-hosted mode)
var inspectorLoaded = IsInspectorLoaded(targetProcess);
var inspectorLoaded = IsInspectorLoaded(targetProcess) ||
await WaitForInspectorPipeAsync(targetProcess.Id, TimeSpan.FromMilliseconds(700));
if (inspectorLoaded)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in consolidated PR #22 (commit 35c1177): the pre-injection check is now a 100 ms single-shot pipe probe, while the longer retry loop remains only after injection.

@miloszkukla

Copy link
Copy Markdown
Author

Superseded by #22, which consolidates the full change set and incorporates the applicable review feedback. Closing this stacked PR to avoid duplicate review threads and workflow ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants