Run tests as exe (RunAsExe attribute)#16129
Conversation
…l file copies need to be made to make it work
Bring the run-tests-as-exe work up to current main (262 commits). Conflict resolutions: - TestPlatform.sln: accept main's migration to TestPlatform.slnx. - test/.../Build.cs: take main's version; the branch's acceptance-test caching is already present in main's IntegrationTestBuild helper. - eng/verify-nupkgs.ps1, Microsoft.TestPlatform.csproj: take main's modernized versions (branch edits were superseded). - RunConfiguration: keep the branch's experimental DisableSharedTestHost default while adopting main's new CreateNoNewWindow. - PublicAPI.Unshipped.txt: union of both sides' additions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
It introduces breaking ObjectModel API/behavior changes and includes multiple unconditional throw/forced-path probes that would regress normal execution and packaging correctness.
Pull request overview
This PR revives the “run tests as the test project’s own executable” experiment by introducing an assembly-level opt-in attribute ([RunAsExe]), flowing an ExecutionPreference through source details/runsettings, and adjusting host/engine behavior to route those sources through a different hosting path. It also updates packaging to ship additional TestHost assets (notably for net462) and adds supporting infrastructure in inference/utilities.
Changes:
- Add
RunAsExeAttribute+ExecutionPreference, detect the attribute via PE metadata, and propagate execution preference per source into runsettings/source details. - Modify vstest.console request/infer paths to normalize
.exeinputs to adjacent.dllwhere applicable and emit per-source execution preference maps. - Update engine/host/provider + packaging projects to support new execution preference and ship required TestHost build assets.
File summaries
| File | Description |
|---|---|
| test/vstest.ProgrammerTests/Fakes/FakeAssemblyMetadataProvider.cs | Adds HasRunAsExe stub for the fake metadata provider used by programmer tests. |
| test/Microsoft.TestPlatform.TestUtilities/CustomCompatibilityDataSource.cs | Adds JustRow plumbing to restrict compatibility matrix generation. |
| src/vstest.console/TestPlatformHelpers/TestRequestManager.cs | Normalizes sources to .dll next to .exe and propagates execution preference maps into SourceDetail. |
| src/vstest.console/CommandLine/Interfaces/IAssemblyMetadataProvider.cs | Extends metadata provider contract with HasRunAsExe. |
| src/vstest.console/CommandLine/InferHelper.cs | Updates framework inference for exe/dll pairing and introduces execution-preference detection. |
| src/vstest.console/CommandLine/CommandLineOptions.cs | Applies exe→dll normalization when adding sources from CLI patterns. |
| src/vstest.console/CommandLine/AssemblyMetadataProvider.cs | Implements HasRunAsExe by scanning PE metadata for RunAsExeAttribute. |
| src/package/Microsoft.TestPlatform.TestHost/Microsoft.TestPlatform.TestHost.nuspec | Expands net462 dependencies/assets and adjusts packed content layout. |
| src/package/Microsoft.TestPlatform.TestHost/Microsoft.TestPlatform.TestHost.csproj | Switches to “all TFMs” target set and copies build/** assets into outputs. |
| src/package/Microsoft.TestPlatform.TestHost/build/net8.0/Microsoft.TestPlatform.TestHost.targets | New net8 targets file for runtime host configuration option. |
| src/package/Microsoft.TestPlatform.TestHost/build/net8.0/Microsoft.TestPlatform.TestHost.props | New net8 props for copying testhost assets to output. |
| src/package/Microsoft.TestPlatform.TestHost/build/net462/Microsoft.TestPlatform.TestHost.targets | New net462 targets file for runtime host configuration option. |
| src/package/Microsoft.TestPlatform.TestHost/build/net462/Microsoft.TestPlatform.TestHost.props | New net462 props for copying multiple framework-specific testhost exes/configs. |
| src/package/Microsoft.NET.Test.Sdk/Microsoft.NET.Test.Sdk.nuspec | Makes net462 depend on Microsoft.TestPlatform.TestHost. |
| src/Microsoft.TestPlatform.Utilities/PublicAPI/PublicAPI.Unshipped.txt | Records new public helper methods for runsettings mutation. |
| src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs | Adds helpers to set ExecutionPreference and disable appdomains in runsettings. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs | WIP changes to resolve/run a test exe “as host” and new exe-resolution helper. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs | Adds run-as-exe guard/selection and (currently) hard-blocks shared/built-in host paths. |
| src/Microsoft.TestPlatform.ObjectModel/SourceDetail.cs | Adds ExecutionPreference to SourceDetail and changes Architecture nullability. |
| src/Microsoft.TestPlatform.ObjectModel/RunSettings/RunConfiguration.cs | Adds ExecutionPreference to runsettings and changes default shared-host behavior. |
| src/Microsoft.TestPlatform.ObjectModel/RunAsExeAttribute.cs | Introduces the new assembly-level attribute and ExecutionPreference enum. |
| src/Microsoft.TestPlatform.ObjectModel/PublicAPI/PublicAPI.Unshipped.txt | Tracks new public types/members (RunAsExeAttribute, ExecutionPreference, etc.). |
| src/Microsoft.TestPlatform.ObjectModel/PublicAPI/PublicAPI.Shipped.txt | Updates shipped contract for SourceDetail.Architecture nullability. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Utilities/SourceDetailHelper.cs | Updates runsettings based on per-source execution preference. |
| src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs | Threads execution preference into in-proc decisioning and host grouping; currently forces isolation. |
| src/Microsoft.TestPlatform.Common/Utilities/AssemblyResolver.cs | Adds a “skip resolver in testhost” heuristic. |
| playground/TestPlatform.Playground/Program.cs | Adjusts playground wiring to run against external exe sources. |
| playground/MSTest1/UnitTest1.cs | Modifies a playground test to sleep longer (debug/diagnostics use). |
Copilot's findings
- Files reviewed: 26/28 changed files
- Comments generated: 17
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| public bool DebugVSTestConsole { get; set; } | ||
| public bool DebugTestHost { get; set; } | ||
| public bool DebugDataCollector { get; set; } | ||
| public bool DebugStopAtEntrypoint { get; set; } | ||
| public int JustRow { get; set; } | ||
|
|
| _builder.DebugTestHost = DebugTestHost; | ||
| _builder.DebugStopAtEntrypoint = DebugStopAtEntrypoint; | ||
|
|
||
| _builder.JustRow = JustRow; | ||
|
|
| foreach (var customAttributeHandle in metadataReader.CustomAttributes) | ||
| { | ||
| var attr = metadataReader.GetCustomAttribute(customAttributeHandle); |
| { | ||
| public string? Source { get; internal set; } | ||
| public Architecture Architecture { get; internal set; } | ||
| public Architecture? Architecture { get; internal set; } | ||
| public Framework? Framework { get; internal set; } | ||
| public ExecutionPreference? ExecutionPreference { get; internal set; } | ||
| } |
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SettingsNameAttribute.SettingsNameAttribute(string! settingsName) -> void | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Architecture.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Architecture | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Architecture.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Architecture? | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Framework.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Framework? |
| _isTestHost = Process.GetCurrentProcess().ProcessName.StartsWith("testhost", StringComparison.OrdinalIgnoreCase) | ||
| || Process.GetCurrentProcess().ProcessName.StartsWith("Test", StringComparison.OrdinalIgnoreCase); | ||
| } |
|
|
||
| if (_isTestHost) | ||
| { | ||
| Debug.WriteLine($"Skipping custom resolve for dll {args?.Name}"); | ||
| return null; | ||
| } |
| <!-- Add a third party notice file --> | ||
| <file src="$SrcPackageFolder$ThirdPartyNotices.txt" target="" /> | ||
|
|
||
| <file src="net462\build\**" target="build" /> | ||
|
|
| internal static string UpdateToDllNextToExe(string executablePath) | ||
| { | ||
| var extension = Path.GetExtension(executablePath); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dllPath = Path.ChangeExtension(executablePath, ".dll"); | ||
| if (File.Exists(dllPath)) | ||
| { | ||
| return dllPath; | ||
| } | ||
| } | ||
|
|
||
| return executablePath; | ||
| } |
| { | ||
| fx = _assemblyMetadataProvider.GetFrameworkName(source); | ||
| var extension = Path.GetExtension(source); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dll = Path.ChangeExtension(source, ".dll"); | ||
| if (File.Exists(dll)) |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Run Tests as Exe (WIP)
Draft PR acknowledged — The description accurately documents the intentional hard-coded probes and WIP state. The findings below are scoped to real bugs and design issues that should be addressed before this graduates to a reviewable state, not commentary on the probes.
Findings
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| 3 | AssemblyResolver.cs |
Process Architecture & Host Resolution | 🐛 Design bug |
| 4 | DefaultTestHostManager.cs |
Process Architecture & Host Resolution | 🐛 Latent null |
| 5 | DotnetTestHostManager.cs |
Process Architecture & Host Resolution | 🚧 WIP tracked |
Summary
Finding 1 — SourceDetail.Architecture in PublicAPI.Shipped.txt is changed from Architecture (non-nullable struct) to Architecture? (nullable). This is a source-breaking change to an already-shipped API. PublicAPI.Shipped.txt should not be modified; the nullable variant belongs in PublicAPI.Unshipped.txt.
Finding 2 — GetRunAsExeFromAssemblyMetadata matches attributes by short name only, meaning any attribute named RunAsExeAttribute in any namespace will trigger RunAsExe. The match must also verify the declaring namespace is Microsoft.VisualStudio.TestPlatform.ObjectModel.
Finding 3 — AssemblyResolver._isTestHost uses ProcessName.StartsWith("Test", ...). This is far too broad — any process starting with "Test" (test utilities, runners, explorers) will have all custom assembly resolution silently bypassed. A constructor-injected host-mode flag would be more precise.
Finding 4 — In DefaultTestHostManager, the !isUsingTestHostFromNextToSource fallback path has a latent null: when _fileHelper.Exists(testHostPath) is true, testHostProcessPath is never assigned before the TPDebug.Assert(testHostProcessPath is not null) fires. This is masked by the WIP guard today but will surface immediately when it is removed.
Finding 5 — DotnetTestHostManager.GetTestHostProcessStartInfo calls TryGetExePathFromDll unconditionally, which throws for all standard .dll-only test runs. Tracked as acknowledged WIP; the final structure needs a gate on ExecutionPreference.RunAsExe.
PR Description Alignment
Title and description accurately describe the scope and WIP nature. No misalignment found. ✅
🧠 Reviewed by expert-reviewer workflow
🧠 Reviewed by Expert Code Reviewer 🧠
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Architecture.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Architecture | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Architecture.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Architecture? | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.SourceDetail.Framework.get -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Framework? |
There was a problem hiding this comment.
[Public API Surface Protection] SourceDetail.Architecture is modified in PublicAPI.Shipped.txt from non-nullable Architecture to nullable Architecture?. Modifying PublicAPI.Shipped.txt is a source-breaking change for existing callers — code that uses this property without a null-check will get a compiler warning/error (Roslyn treats it as a nullability contract change).
PublicAPI.Shipped.txt represents the already-released surface. The correct pattern is:
- Remove or leave the existing entry in
PublicAPI.Shipped.txtalone - Add the new nullability signature only to
PublicAPI.Unshipped.txt
Consider keeping the shipped property non-nullable and adding a distinct property (e.g., in PublicAPI.Unshipped.txt) if the nullable form is required for the new flow.
🧠 Reviewed by expert-reviewer workflow
| var sourceFile = Path.GetFileNameWithoutExtension(sourcePath); | ||
|
|
||
| // use the exe as host | ||
| _dotnetHostPath = TryGetExePathFromDll(sourcePath); |
There was a problem hiding this comment.
[Process Architecture & Host Resolution] TryGetExePathFromDll is called unconditionally for every source regardless of ExecutionPreference. It throws ArgumentException when no .exe exists next to the .dll — which is the standard case for all .NET Core test runs. This means every ordinary dotnet test invocation will fail at this point.
The PR description acknowledges this as an intentional WIP probe, but flagging for tracking: the structural pattern should gate on ExecutionPreference before attempting exe resolution, rather than always throwing. Something like:
if (runConfiguration.ExecutionPreference == ExecutionPreference.RunAsExe)
{
_dotnetHostPath = TryGetExePathFromDll(sourcePath);
}🧠 Reviewed by expert-reviewer workflow
|
|
||
| var typeName = metadataReader.GetString(typeNameHandle.Value); | ||
|
|
||
| if (string.Equals(typeName, "RunAsExeAttribute", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
[Null Safety & Boundary Validation] The attribute is matched by short name only ("RunAsExeAttribute"), without checking the namespace. Any assembly that happens to declare an attribute named RunAsExeAttribute in any namespace will be treated as RunAsExe-capable — including third-party or test-utility attributes that coincidentally share the name.
Before this ships, the match should also verify the declaring namespace is Microsoft.VisualStudio.TestPlatform.ObjectModel:
// For TypeReference (external ref):
StringHandle? namespaceHandle = null;
if (mref.Parent.Kind == HandleKind.TypeReference)
{
TypeReference tref = metadataReader.GetTypeReference((TypeReferenceHandle)mref.Parent);
typeNameHandle = tref.Name;
namespaceHandle = tref.Namespace;
}
// Then check both:
if (string.Equals(typeName, "RunAsExeAttribute", StringComparison.OrdinalIgnoreCase)
&& string.Equals(ns, "Microsoft.VisualStudio.TestPlatform.ObjectModel", StringComparison.Ordinal))🧠 Reviewed by expert-reviewer workflow
| var testHostPath = Path.Combine(currentWorkingDirectory, testHostProcessName); | ||
|
|
||
| if (!_fileHelper.Exists(testHostPath)) | ||
| { |
There was a problem hiding this comment.
[Process Architecture & Host Resolution] Latent null-dereference when the WIP guard is removed: if _fileHelper.Exists(testHostPath) returns true (testhost found in the current directory), the if (!_fileHelper.Exists(...)) branch is skipped and testHostProcessPath is never assigned inside this block — it stays null. The TPDebug.Assert(testHostProcessPath is not null, ...) immediately below will then fail.
The fix when removing the guard: assign testHostProcessPath = testHostPath; in the else branch (or unconditionally before the inner if).
var testHostPath = Path.Combine(currentWorkingDirectory, testHostProcessName);
testHostProcessPath = testHostPath; // always assign the candidate path
if (!_fileHelper.Exists(testHostPath))
{
// ... fallback to TestHostNetFramework subfolder ...
testHostProcessPath = Path.Combine(...);
}🧠 Reviewed by expert-reviewer workflow
| _platformAssemblyResolver.AssemblyResolve += OnResolve; | ||
|
|
||
| _isTestHost = Process.GetCurrentProcess().ProcessName.StartsWith("testhost", StringComparison.OrdinalIgnoreCase) | ||
| || Process.GetCurrentProcess().ProcessName.StartsWith("Test", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
[Process Architecture & Host Resolution] StartsWith("Test", ...) is far too broad — it matches any process whose name begins with "Test" (e.g., TestConsole.exe, TestRunner.exe, TestAgent.exe, TestExplorer.exe). When _isTestHost is true, all custom assembly resolution is silently skipped for the entire lifetime of that AssemblyResolver instance.
This could silently disable extension/adapter loading in test utilities or runners that share the same AssemblyResolver code path but aren't actually a testhost. The condition should be more precise — ideally a constructor parameter or injected host-mode flag rather than process-name inference, since process names are fragile and ambiguous.
Also: Process.GetCurrentProcess() is called twice here; the result should be cached in a local variable.
🧠 Reviewed by expert-reviewer workflow
…al test projects A test framework opts a test project into running as its own exe with the [RunAsExe] assembly attribute. Without it everything behaves exactly as before. - InferHelper routes a source to RunAsExe only when the attribute is present, instead of forcing it on for every managed source. - DefaultTestHostManager (desktop) and DotnetTestHostManager (.NET) look for the exe next to the source and fall back to the built-in testhost when it is not there, instead of throwing. - TestEngine no longer forces isolation, so in-process runs work again, and the non-default execution-preference check treats null as Default. - RunConfiguration keeps the shared-testhost feature-flag default instead of forcing sharing off. Packaging is now strictly opt-in so normal (Library) test projects are byte-for- byte like before: - The Test.Sdk generates the Main, references testhost.dll, and copies the testhost engine closure only when the project declares OutputType=Exe. - That payload moved from lib\net462 (auto-referenced by everyone) to build\net462\runAsExe\, so lib\net462 is back to _._ and Library projects no longer drag the testhost engine into their output. - testhost.exe is copied next to the output only for non-exe projects. RunAsExeTestProject is a focused opt-in asset that builds as its own exe and runs its tests through the generated Main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 2 (updated since 2026-06-17T10:16Z)
Draft PR — WIP findings only. The previous review (run 27681333801) identified 5 issues. This round covers the delta introduced by the 17 new commits.
Progress since last review ✅
| # | Finding | Status |
|---|---|---|
| 4 | DefaultTestHostManager latent null (testHostProcessPath unassigned) |
Resolved — both branches now assign testHostProcessPath before use |
| 5 | DotnetTestHostManager.TryGetExePathFromDll called unconditionally |
Resolved — now correctly gated on if (_runAsExe) |
Still open from last review
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| 3 | AssemblyResolver.cs |
Process Architecture & Host Resolution | 🐛 Design bug |
New findings (this round)
| # | File | Dimension | Severity |
|---|---|---|---|
| A | DotnetTestHostManager.cs:1063 |
Process Architecture & Host Resolution | 🐛 Silent failure |
| B | net462/Microsoft.TestPlatform.TestHost.targets:120 |
Build Script & Infrastructure Hygiene | 🐛 Package bloat |
| C | AssemblyResolver.cs:62 |
Process Architecture & Host Resolution | ⚡ Perf / design |
Finding A — TryGetExePathFromDll hardcodes .exe. On Linux/macOS the SDK-produced apphost has no extension, so the method always returns null. The fallback is trace-only — end users get no diagnostic. See inline comment.
Finding B — Both the x86 and x64 ItemGroup blocks in the new net462/Microsoft.TestPlatform.TestHost.targets use the identical condition ('$(OutputType)' != 'Exe' AND '$(OS)' == 'Windows_NT'). The original architecture-differentiating conditions are commented out. Every Library test project on Windows will receive both x86 and x64 testhost binaries — double the expected file count. See inline comment.
Finding C — Process.GetCurrentProcess() is called twice in the AssemblyResolver constructor. The StartsWith("Test") predicate from Finding 3 is still there. See inline comment.
PR Description Alignment
Title and description accurately describe the WIP scope and the intentionally hard-coded probes. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
| return null; | ||
| } | ||
|
|
||
| var exe = Path.ChangeExtension(sourcePath, ".exe"); // todo: linux macos |
There was a problem hiding this comment.
[Process Architecture & Host Resolution] TryGetExePathFromDll hardcodes .exe — silent failure on Linux/macOS.
var exe = Path.ChangeExtension(sourcePath, ".exe"); // todo: linux macosOn Linux and macOS the SDK produces apphosts with no extension (e.g. MyTests, not MyTests.exe). This method will return null on every non-Windows run, and _dotnetHostPath will not be updated. The fallback trace message is Verbose-level, so end users who opt in with [RunAsExe] on Linux/macOS will get no indication that the feature is silently disabled.
Before graduation, the cross-platform path needs to probe for the no-extension form:
// Linux/macOS: apphost has no extension
var candidate = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
? Path.ChangeExtension(sourcePath, ".exe")
: Path.Combine(Path.GetDirectoryName(sourcePath)!, Path.GetFileNameWithoutExtension(sourcePath));And the fallback should surface a Warning-level message so framework authors can diagnose the issue.
| _platformAssemblyResolver.AssemblyResolve += OnResolve; | ||
|
|
||
| _isTestHost = Process.GetCurrentProcess().ProcessName.StartsWith("testhost", StringComparison.OrdinalIgnoreCase) | ||
| || Process.GetCurrentProcess().ProcessName.StartsWith("Test", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
[Process Architecture & Host Resolution] Process.GetCurrentProcess() is called twice in a single assignment. The first call's result is discarded before the || short-circuits to the second call. Capture the process name once:
var processName = Process.GetCurrentProcess().ProcessName;
_isTestHost = processName.StartsWith("testhost", StringComparison.OrdinalIgnoreCase)
|| processName.StartsWith("Test", StringComparison.OrdinalIgnoreCase);Note: StartsWith("Test", ...) remains extremely broad — it matches TestApp, TestRunner, TestExplorer, etc., silently disabling custom assembly resolution for any process whose name starts with "Test". This is the same concern raised in the previous review (Finding 3). A constructor-injected bool isTestHost flag would be more precise and testable.
|
|
||
| <!-- x64 / non-x86, non-ARM64: default bucket --> | ||
| <!--<ItemGroup Condition=" ( '$(Platform)' != 'x86' AND '$(PlatformTarget)' != 'x86' AND '$(Platform)' != 'arm64' AND '$(PlatformTarget)' != 'arm64' ) AND '$(OS)' == 'Windows_NT' ">--> | ||
| <ItemGroup Condition=" '$(OutputType)' != 'Exe' AND '$(OS)' == 'Windows_NT' "> |
There was a problem hiding this comment.
[Build Script & Infrastructure Hygiene] Both x86 and x64 ItemGroup elements use the same condition, so Library test projects on Windows receive all x86 and all x64 testhost binaries in their output directory.
The original x86/x64 selector conditions are commented out (lines 41 and 119). Both groups now evaluate to the same '$(OutputType)' != 'Exe' AND '$(OS)' == 'Windows_NT', making the split meaningless. A Library project's output will get 24 testhost exe/config files instead of the expected 12.
The x64 group's own comment says "x64 / non-x86, non-ARM64: default bucket", implying these groups were meant to be mutually exclusive. Consider either restoring the platform conditions or merging the two groups into one if architecture-based selection is no longer desired for the net462 path.
…Preference SourceDetail.Architecture and ExecutionPreference are nullable, so calling Nullable<T>.ToString() on them is treated as possibly-null under source-build's stricter nullable analysis (CS8604) when passed to the non-null string parameters of UpdateTargetPlatform/UpdateExecutionPreference. Use GetValueOrDefault().ToString() so the non-null Enum.ToString() is used, matching the non-nullable behaviour on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Human review recommended
It introduces a binary-breaking ObjectModel API change and a high-risk assembly-loading behavior change that can affect adapter resolution and host stability.
Copilot's findings
- Files reviewed: 37/39 changed files
- Comments generated: 9
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| public string? Source { get; internal set; } | ||
| public Architecture Architecture { get; internal set; } | ||
| public Architecture? Architecture { get; internal set; } | ||
| public Framework? Framework { get; internal set; } | ||
| public ExecutionPreference? ExecutionPreference { get; internal set; } |
| var extension = Path.GetExtension(source); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dll = Path.ChangeExtension(source, ".dll"); | ||
| if (File.Exists(dll)) | ||
| { | ||
| fx = _assemblyMetadataProvider.GetFrameworkName(dll); | ||
| } | ||
| else | ||
| { | ||
| fx = _assemblyMetadataProvider.GetFrameworkName(source); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| fx = _assemblyMetadataProvider.GetFrameworkName(source); | ||
| } |
| var extension = Path.GetExtension(executablePath); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dllPath = Path.ChangeExtension(executablePath, ".dll"); | ||
| if (File.Exists(dllPath)) | ||
| { | ||
| return dllPath; | ||
| } | ||
| } |
| { | ||
| var metadataReader = peReader.GetMetadataReader(); | ||
|
|
||
| foreach (var customAttributeHandle in metadataReader.CustomAttributes) |
| internal static string? TryGetExePathFromDll(string sourcePath) | ||
| { | ||
| if (Path.GetExtension(sourcePath) != ".dll") | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var exe = Path.ChangeExtension(sourcePath, ".exe"); // todo: linux macos | ||
| return File.Exists(exe) ? exe : null; | ||
| } |
| _platformAssemblyResolver.AssemblyResolve += OnResolve; | ||
|
|
||
| _isTestHost = Process.GetCurrentProcess().ProcessName.StartsWith("testhost", StringComparison.OrdinalIgnoreCase) | ||
| || Process.GetCurrentProcess().ProcessName.StartsWith("Test", StringComparison.OrdinalIgnoreCase); | ||
| } |
| <!-- | ||
| 4. run-as-exe (.NET Framework): the test project is built as its own exe and references testhost.dll | ||
| (shipped in lib\net462), so the generic testhost*.exe launchers are intentionally NOT copied next to | ||
| the test output. This file previously copied testhost.exe and the per-tfm/arch variants here. | ||
| --> |
| public ExecutionPreference ExecutionPreference { get; private set; } = ExecutionPreference.Default; | ||
|
|
||
| /// <inheritdoc/> | ||
| public override XmlElement ToXml() | ||
| { |
| isDiscovery: false, | ||
| out string updatedRunsettings, | ||
| out IDictionary<string, Architecture> sourceToArchitectureMap, | ||
| out IDictionary<string, Framework> sourceToFrameworkMap)) | ||
| out IDictionary<string, Framework> sourceToFrameworkMap, | ||
| out IDictionary<string, ExecutionPreference> sourceToExecutionPreferenceMap)) |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 3 (updated since 2026-06-17T17:56Z)
Draft PR — WIP findings only. The previous review (27708486557) covered commit
2de42d65. This round covers the single new commit017fd64a.
New commit since Round 2
One commit: 017fd64a — fixes CS8604 source-build warning in SourceDetailHelper.cs by using GetValueOrDefault().ToString() on nullable Architecture? and ExecutionPreference? properties.
New finding
| # | File | Dimension | Severity |
|---|---|---|---|
| D | SourceDetailHelper.cs:24 |
Null Safety & Boundary Validation | 🐛 Correctness |
Finding D — Architecture.GetValueOrDefault() when null returns Architecture.Default (enum 0), which serializes to "Default". With overwrite: true, this writes <TargetPlatform>Default</TargetPlatform> into RunSettings and silently overwrites any user-configured explicit architecture (e.g. X64).
InferRunSettingsHelper itself flags Architecture.Default as invalid in its own validator (value != Architecture.Default), so the round-trip semantics of "Default" are not the same as omitting the node. The correct fix is a null guard: if (sourceDetail.Architecture is { } architecture). See inline comment.
Still open from prior rounds
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| 3 | AssemblyResolver.cs |
Process Architecture & Host Resolution | 🐛 Design bug |
| A | DotnetTestHostManager.cs:1063 |
Process Architecture & Host Resolution | 🐛 Silent failure |
| B | net462/Microsoft.TestPlatform.TestHost.targets:120 |
Build Script & Infrastructure Hygiene | 🐛 Package bloat |
PR Description Alignment
Title and description continue to accurately describe the WIP scope. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
|
|
||
| InferRunSettingsHelper.UpdateTargetFramework(document, sourceDetail.Framework!.ToString(), overwrite: true); | ||
| InferRunSettingsHelper.UpdateTargetPlatform(document, sourceDetail.Architecture.ToString(), overwrite: true); | ||
| InferRunSettingsHelper.UpdateTargetPlatform(document, sourceDetail.Architecture.GetValueOrDefault().ToString(), overwrite: true); |
There was a problem hiding this comment.
[Null Safety & Boundary Validation] GetValueOrDefault() when Architecture is null silently promotes null to Architecture.Default (enum value 0), which then writes <TargetPlatform>Default</TargetPlatform> into the RunSettings XML with overwrite: true. This overwrites any user-configured explicit architecture (e.g. X64) with "Default".
Noteworthy: InferRunSettingsHelper itself has a validator that explicitly marks Architecture.Default as invalid for TargetPlatform (value != Architecture.Default), so the round-trip semantics of "Default" are not the same as omitting the node.
The correct pattern is a null guard — skip the call when no architecture was determined:
if (sourceDetail.Architecture is { } architecture)
{
InferRunSettingsHelper.UpdateTargetPlatform(document, architecture.ToString(), overwrite: true);
}Line 30 has the same pattern for ExecutionPreference but is safe because the enclosing if (... == ExecutionPreference.RunAsExe) already guarantees the value is non-null; .Value.ToString() would be more idiomatic there.
The Microsoft.TestPlatform.ObjectModel package is unchanged by the run-as-exe work, but its expected file count had been set to 67 from a local pack that produces fewer localized satellite resource assemblies than the official build. The official (CI Release) package contains 93 files, so restore that value to match main. TestHost (140) and Test.Sdk (27) already match the official build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 4 (updated since 2026-06-17T18:34Z)
Draft PR — WIP findings only. The previous review (27710600718) covered commit
017fd64a. This round covers the single new commit36e8bf96.
New commit since Round 3
One commit: 36e8bf96 — restores Microsoft.TestPlatform.ObjectModel expected nupkg file count from 67 → 93 to match the official CI Release build.
Analysis — eng/expected-nupkg-file-counts.json
Dimension: Build Script & Infrastructure Hygiene / Dependency & Package Integrity
The change is correct. The value 67 came from a local Debug pack that omits the localized satellite assemblies; the CI Release build produces 93 files. The TestHost (140) and Microsoft.NET.Test.Sdk (27) counts were already aligned with CI — only ObjectModel needed correction.
One forward-looking note: when Finding B (duplicate ItemGroup condition in Microsoft.TestPlatform.TestHost.targets) is resolved, the TestHost expected count (140) will need to be re-verified against a clean Release pack to ensure it still accounts for the corrected opt-in file layout. No action required now.
No new findings from this commit. ✅
Still open from prior rounds
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| 3 | AssemblyResolver.cs |
Process Architecture & Host Resolution | 🐛 Design bug |
| A | DotnetTestHostManager.cs:1063 |
Process Architecture & Host Resolution | 🐛 Silent failure |
| B | net462/Microsoft.TestPlatform.TestHost.targets:120 |
Build Script & Infrastructure Hygiene | 🐛 Package bloat |
| D | SourceDetailHelper.cs:24 |
Null Safety & Boundary Validation | 🐛 Correctness |
PR Description Alignment
Title and description accurately describe the WIP scope and intentional hard-coded probes. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
…ter loading) The run-as-exe work disabled AssemblyResolver.OnResolve whenever the current process name started with 'testhost' or 'Test', intending to skip custom assembly resolution in run-as-exe mode. But that also disabled it for the normal testhost.exe, so the testhost could no longer resolve test adapter dependencies (e.g. MSTest.TestAdapter) and every run reported 'No test is available in <assembly>'. This is why integration tests failed in CI while in-process runs (vstest.console) and the released testhost both worked. Run-as-exe assets run under their own process name and never matched the check anyway, so removing it restores the resolver to main's behavior with no impact on run-as-exe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
It introduces a breaking public API change (SourceDetail.Architecture nullability) and has packaging/host-resolution issues that would affect consumers and cross-platform behavior.
Copilot's findings
- Files reviewed: 36/38 changed files
- Comments generated: 10
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| public string? Source { get; internal set; } | ||
| public Architecture Architecture { get; internal set; } | ||
| public Architecture? Architecture { get; internal set; } | ||
| public Framework? Framework { get; internal set; } | ||
| public ExecutionPreference? ExecutionPreference { get; internal set; } |
| public bool DebugVSTestConsole { get; set; } | ||
| public bool DebugTestHost { get; set; } | ||
| public bool DebugDataCollector { get; set; } | ||
| public bool DebugStopAtEntrypoint { get; set; } | ||
| public int JustRow { get; set; } | ||
|
|
| _builder.DebugTestHost = DebugTestHost; | ||
| _builder.DebugStopAtEntrypoint = DebugStopAtEntrypoint; | ||
|
|
||
| _builder.JustRow = JustRow; | ||
|
|
| foreach (var customAttributeHandle in metadataReader.CustomAttributes) | ||
| { | ||
| var attr = metadataReader.GetCustomAttribute(customAttributeHandle); | ||
|
|
| internal static string? TryGetExePathFromDll(string sourcePath) | ||
| { | ||
| if (Path.GetExtension(sourcePath) != ".dll") | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var exe = Path.ChangeExtension(sourcePath, ".exe"); // todo: linux macos | ||
| return File.Exists(exe) ? exe : null; | ||
| } |
| <file src="net8.0\testhost.x86.dll" target="build\net8.0\x86\" /> | ||
| <file src="net8.0\testhost.x86.exe" target="build\net8.0\x86\" /> | ||
|
|
| <!-- | ||
| 4. run-as-exe (.NET Framework): the test project is built as its own exe and references testhost.dll | ||
| (shipped in lib\net462), so the generic testhost*.exe launchers are intentionally NOT copied next to | ||
| the test output. This file previously copied testhost.exe and the per-tfm/arch variants here. | ||
| --> |
| public static void UpdateDisableAppDomain(XmlDocument runSettingsDocument, string value, bool overwrite = false) | ||
| => AddNodeIfNotPresent(runSettingsDocument, "/RunSettings/RunConfiguration/DisableAppDomain", "DisableAppDomain", value, overwrite); |
| if (sourceToSourceDetailMap.Any(s => (s.Value.ExecutionPreference ?? ExecutionPreference.Default) != ExecutionPreference.Default)) | ||
| { | ||
| EqtTrace.Info("TestEngine.ShouldRunInNoIsolation: At least one source has non-default execution preference, running in isolation (in a separate testhost process)."); | ||
| return false; | ||
| } |
| [TestMethod] | ||
| public void TestMethod5() | ||
| { | ||
| // Thread.Sleep(1000); | ||
| Thread.Sleep(10000); | ||
| } |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 5 (updated since 2026-06-17T19:41Z)
Draft PR — WIP findings only. The previous review (27714519616) covered commit
36e8bf96. This round covers the single new commit8fa6e4f1.
New commit since Round 4
One commit: 8fa6e4f1 — Don't disable the custom AssemblyResolver in the testhost (fixes adapter loading)
Removes the _isTestHost field and its associated guard in OnResolve that was skipping custom assembly resolution whenever the current process name started with "testhost" or "Test". The AssemblyResolve event is now always registered.
Analysis
Dimension: Process Architecture & Host Resolution
This commit resolves Finding 3 and Finding C from prior rounds:
- Finding 3 —
_isTestHost = ProcessName.StartsWith("Test", ...): overly-broad predicate that bypassed adapter resolution for any process beginning with "Test". The field and its guard are now fully removed. ✅ - Finding C —
Process.GetCurrentProcess()called twice in the constructor (one to test host-name, one to skip registration). Both calls are gone. ✅
The root cause is accurately diagnosed in the commit message: the check was only ever triggered by the standard testhost.exe process name, which never needed to skip resolution. Run-as-exe test processes use the test project's own executable name and never matched, so removing the check is both safe and correct.
No new findings are introduced by this commit.
Progress summary across all rounds
| # | Finding | Status |
|---|---|---|
| 4 | DefaultTestHostManager latent null (testHostProcessPath) |
✅ Resolved (Round 2) |
| 5 | DotnetTestHostManager.TryGetExePathFromDll called unconditionally |
✅ Resolved (Round 2) |
| C | AssemblyResolver: Process.GetCurrentProcess() called twice |
✅ Resolved (Round 5) |
| 3 | AssemblyResolver._isTestHost: overly-broad StartsWith("Test") guard |
✅ Resolved (Round 5) |
Still open from prior rounds
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| A | DotnetTestHostManager.cs:1063 |
Process Architecture & Host Resolution | 🐛 Silent failure |
| B | net462/Microsoft.TestPlatform.TestHost.targets:120 |
Build Script & Infrastructure Hygiene | 🐛 Package bloat |
| D | SourceDetailHelper.cs:24 |
Null Safety & Boundary Validation | 🐛 Correctness |
PR Description Alignment
Title and description accurately describe the WIP scope and intentionally hard-coded probes. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
A test project that runs as its own executable has its dependencies and test adapters next to the executable. The custom assembly resolver and the runner's default and additional extensions (loggers, data collectors, code coverage) point at the runner's own folders, and pulling them into the host only mixes in unrelated dependencies. The test host manager now sets VSTEST_RUNASEXE on the host process when it launches the executable. When it is set the host: - Disables the custom AssemblyResolver, so dependencies resolve from the executable's own folder instead of the runner's locations. - Keeps only the extension paths under the app base, dropping the ones the runner passes from its own folder. The test project's own adapters sit next to the executable, so they are still used. Normal testhost runs do not set the variable and are unchanged. Loggers and data collectors run in vstest.console and the datacollector, which are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 6 (updated since 2026-06-18T10:55Z)
Draft PR — WIP findings only. The previous review (27754036082) covered commit
8fa6e4f1. This round covers the single new commit8e9f739b.
New commit since Round 5
One commit: 8e9f739b — Make run-as-exe host ignore the runner's resolver and extensions
Five files changed (101 additions, 1 deletion):
RunAsExeHelper.cs(new) — static helper that readsVSTEST_RUNASEXE=1from the environment via a static initializer; consumed by bothAssemblyResolverandTestPluginCacheAssemblyResolver.cs— new_isRunningAsExefield;OnResolvereturnsnullearly with a verbose trace when set, so the runtime resolves from the app base instead of the runner's extension directoriesTestPluginCache.cs—DefaultExtensionPathssetter filters the incoming paths to only those underAppContext.BaseDirectorywhenIsRunningAsExeis true, dropping the runner's loggers/data-collectors/adaptersDefaultTestHostManager.cs— setsVSTEST_RUNASEXE=1in the child process environment when launching a run-as-exe hostDotnetTestHostManager.cs— same env-var write for the .NET Core run-as-exe path
Analysis
Correctness — The overall approach is sound. The env-var gate is process-scoped and set before process launch, so the static-initializer read in RunAsExeHelper.IsRunningAsExe is correct. The AssemblyResolver early-return includes a Verbose trace that identifies which assembly couldn't be found, providing the diagnostic hook needed when validating that everything ships next to a run-as-exe test executable. The TestPluginCache filter uses Path.GetFullPath(path).StartsWith(directory, OrdinalIgnoreCase) where directory = AppContext.BaseDirectory; AppContext.BaseDirectory always includes a trailing Path.DirectorySeparatorChar, so the prefix comparison is safe.
Falls-back cleanly — When DotnetTestHostManager cannot find the exe next to the dll, it does not set VSTEST_RUNASEXE, so the standard testhost path is unaffected.
New finding
| # | File | Dimension | Severity |
|---|---|---|---|
| E | DefaultTestHostManager.cs:73 |
Environment Variable & Feature Flag Contracts | 🔧 Maintenance concern |
Finding E — RunAsExeEnvironmentVariableName is defined independently in both DefaultTestHostManager (writer, CrossPlatEngine) and RunAsExeHelper (reader, Common) with no compile-time cross-reference. Additionally, the doc comment on line 70–71 says "reader in AssemblyResolver" but the actual reader is RunAsExeHelper — AssemblyResolver and TestPluginCache both read via RunAsExeHelper.IsRunningAsExe. See inline comment.
Still open from prior rounds
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs |
Null Safety & Boundary Validation | 🐛 Bug |
| A | DotnetTestHostManager.cs:1063 |
Process Architecture & Host Resolution | 🐛 Silent failure (Linux/macOS) |
| B | net462/Microsoft.TestPlatform.TestHost.targets:120 |
Build Script & Infrastructure Hygiene | 🐛 Package bloat |
| D | SourceDetailHelper.cs:24 |
Null Safety & Boundary Validation | 🐛 Correctness |
PR Description Alignment
Title and description accurately describe the WIP scope and intentionally hard-coded probes. The new commit's intent (self-contained isolation for run-as-exe hosts) is consistent with the overall feature direction. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
| /// so it disables the custom assembly resolver (all dependencies are next to the exe). Kept in sync with the | ||
| /// reader in <c>Microsoft.VisualStudio.TestPlatform.Common.Utilities.AssemblyResolver</c>. | ||
| /// </summary> | ||
| internal const string RunAsExeEnvironmentVariableName = "VSTEST_RUNASEXE"; |
There was a problem hiding this comment.
[Finding E — Environment Variable & Feature Flag Contracts] 🔧 Duplicate constant / stale cross-reference comment
// Kept in sync with the reader in Microsoft.VisualStudio.TestPlatform.Common.Utilities.AssemblyResolver
internal const string RunAsExeEnvironmentVariableName = "VSTEST_RUNASEXE";Two issues:
-
Comment inaccuracy — the reader is
RunAsExeHelper(inMicrosoft.TestPlatform.Common), notAssemblyResolver.AssemblyResolverreads viaRunAsExeHelper.IsRunningAsExe, as doesTestPluginCache. Updating the comment to say "reader inRunAsExeHelper" prevents future confusion about which type owns the canonical definition. -
Duplicate constant —
RunAsExeHelper.RunAsExeEnvironmentVariableName(the reader's definition inCommon) and this constant (the writer's definition inCrossPlatEngine) are independent string literals with no compile-time cross-reference. IfCrossPlatEnginehas a project reference toCommon(which it may, forAssemblyResolverand related types), thenDefaultTestHostManagercould useRunAsExeHelper.RunAsExeEnvironmentVariableNamedirectly, eliminating the duplication and making the contract impossible to drift. If there's no such reference, moving the constant toObjectModel(shared by both assemblies) would achieve the same protection.
This is low-risk in the short term (both values are hard-coded strings), but for a contract that crosses a process boundary it is good to have a single compile-time source of truth.
main migrated the package projects from hand-written .nuspec to MSBuild pack (#16125, #16132), which deleted the nuspecs the run-as-exe .NET Framework packaging lived in. Re-port that packaging onto the new model: - Ship the run-as-exe netfx testhost closure (testhost.dll, the platform assemblies, msdia and localized resources) under build\net462\runAsExe\ via a TestHost IncludeRunAsExeContent target, instead of the old nuspec file entries. - Pack the build\net462 props/targets (the Exe-conditional testhost reference and closure copy) and the generated Main next to them. - net462 Test.Sdk references TestHost again, and TestHost declares ObjectModel for net462, so a run-as-exe project resolves the [RunAsExe] attribute and the testhost engine at compile time and NuGet unifies ObjectModel to this version. - Drop the per-tfm testhost*.exe launcher copying; normal .NET Framework hosting keeps coming from vstest.console, run-as-exe projects host themselves. Validated: the TestHost package ships the closure, and RunAsExeTestProject builds and runs as its own exe with the custom resolver and the runner extensions disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| discoveryPayload.Sources = KnownPlatformSourceFilter.FilterKnownPlatformSources(discoveryPayload.Sources?.Distinct().ToList()); | ||
| discoveryPayload.Sources = NextToExeDllSourceProvider.UpdateToDllNextToExes(discoveryPayload.Sources); | ||
| discoveryPayload.RunSettings ??= "<RunSettings></RunSettings>"; |
| internal static string UpdateToDllNextToExe(string executablePath) | ||
| { | ||
| var extension = Path.GetExtension(executablePath); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dllPath = Path.ChangeExtension(executablePath, ".dll"); | ||
| if (File.Exists(dllPath)) | ||
| { | ||
| return dllPath; | ||
| } | ||
| } |
| var extension = Path.GetExtension(source); | ||
| if (extension is ".exe" or "") | ||
| { | ||
| var dll = Path.ChangeExtension(source, ".dll"); | ||
| if (File.Exists(dll)) | ||
| { | ||
| fx = _assemblyMetadataProvider.GetFrameworkName(dll); |
| preference = ExecutionPreference.Default; | ||
| } | ||
|
|
||
| sourceToExecutionPreferenceMap.Add(source, preference); | ||
| } |
| foreach (var customAttributeHandle in metadataReader.CustomAttributes) | ||
| { | ||
| var attr = metadataReader.GetCustomAttribute(customAttributeHandle); | ||
|
|
||
| // Display the attribute type full name | ||
| StringHandle? typeNameHandle = null; | ||
| if (attr.Constructor.Kind == HandleKind.MethodDefinition) | ||
| { | ||
| MethodDefinition methodDefinition = metadataReader.GetMethodDefinition((MethodDefinitionHandle)attr.Constructor); | ||
| TypeDefinition typeDefinition = metadataReader.GetTypeDefinition(methodDefinition.GetDeclaringType()); | ||
| typeNameHandle = typeDefinition.Name; | ||
| } |
| var typeName = metadataReader.GetString(typeNameHandle.Value); | ||
|
|
||
| if (string.Equals(typeName, "RunAsExeAttribute", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } |
| private static bool IsUnderDirectory(string path, string directory) | ||
| { | ||
| try | ||
| { | ||
| return Path.GetFullPath(path).StartsWith(directory, StringComparison.OrdinalIgnoreCase); | ||
| } |
| public Architecture? Architecture { get; internal set; } | ||
| public Framework? Framework { get; internal set; } | ||
| public ExecutionPreference? ExecutionPreference { get; internal set; } |
| executionPreference = (ExecutionPreference)Enum.Parse(typeof(ExecutionPreference), value, true); | ||
| // Ensure that the parsed value is actually in the enum, and that Default or AnyCpu are not provided. | ||
| if (!Enum.IsDefined(typeof(ExecutionPreference), executionPreference)) | ||
| { | ||
| throw new SettingsException( |
| class AutoGeneratedProgram | ||
| { | ||
| static void Main(string[] args) | ||
| { |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Expert Review — Round 7 (updated since 2026-06-18T14:09Z)
Draft PR — WIP findings only. The previous review (27764500271) covered commit
8e9f739b. This round covers the single new commit274b3db6— a merge of main plus a complete re-port of the run-as-exe .NET Framework packaging onto the MSBuild-pack model.
New commit since Round 6
One commit: 274b3db6 — Merge main and re-port the run-as-exe packaging to MSBuild pack
Key changes in this commit (excluding the merged .github/ workflow files from main):
testhost.library/testhost.library.csproj(new) — anet462OutputType=Libraryproject that compiles the testhost entry-point code fromtesthost.x86sources, producing a referenceabletesthost.dllthat run-as-exe test projects can link againstbuild/net462/Microsoft.TestPlatform.TestHost.targets(new) — conditionally (only when$(OutputType)=='Exe') adds aReferencetotesthost.dlland copies the platform assembly closure next to the test output; non-exe projects are unaffectedbuild/net462/Microsoft.TestPlatform.TestHost.props(new) — stub file with an explanatory comment; no MSBuild side effectsMicrosoft.TestPlatform.TestHost.csproj— adds theIncludeRunAsExeContenttarget that packs the net462 testhost closure underbuild/net462/runAsExe/; extendsObjectModelproject reference to be unconditional (was net8.0-only) so net462 consumers get the[RunAsExe]attribute type and NuGet unifiesObjectModelto this versionMicrosoft.NET.Test.Sdk.csproj— makes theTestHostproject reference unconditional (wasnet8.0-only); addsProgram.cstemplate to the net462 pack contentMicrosoft.NET.Test.Sdk.targets(modified) — addsInitialTargets="GenerateProgramFile"and the corresponding target that injects the generated Main intoC#/VBtest exe projectsMicrosoft.NET.Test.Sdk.Program.cs(new) — the generatedMain; probes for--testsourcepath, appends it if missing, then delegates toMicrosoft.VisualStudio.TestPlatform.TestHost.Program.Runeng/expected-nupkg-file-counts.json—TestHost65 → 116 (+51: 8 platform DLLs, 2 msdia natives, 2 build integration files, ~39 localized satellite assemblies for 3 resource DLLs × 13 locales);Test.Sdk26 → 27 (+1:Program.cstemplate)eng/expected-dll-frameworks.json— 8 newnetframeworkentries underbuild/net462/runAsExe/
Analysis
Finding B: ✅ RESOLVED — The old Microsoft.TestPlatform.TestHost.targets file (which had two ItemGroup blocks with identical conditions, causing every Library test project on Windows to receive both x86 and x64 testhost binaries) has been completely replaced. The new file has a single clean Condition="'$(OutputType)' == 'Exe'" guard; non-exe projects receive nothing from this new content. The file count update (65 → 116) is consistent with the correctly-gated closure content.
Packaging design — no new findings:
- The
testhost.libraryapproach (library form of the testhost entry point, compiled fromtesthost.x86sources,AssemblyName=testhost,net462only) is sound. SinceDisableAppDomain=trueis always set for run-as-exe, theAppDomainEngineInvokerthat is compiled in but not invoked does not introduce risk. - The
InitialTargets="GenerateProgramFile"in.targetsis a valid MSBuild pattern and accumulates correctly with otherInitialTargetsin the project. - The
Microsoft.NET.Test.Sdk.Program.csgenerated Main is correct: it probesargsfor--testsourcepathbefore appending it, avoiding duplicates when the caller already passes the argument. - The
IncludeRunAsExeContentsatellite glob (**/X.resources.dll→PackagePath="build/net462/runAsExe/") preserves locale subdirectory structure in the pack output (confirmed by the +39 satellite file delta), so .NET Framework satellite resolution from<culture>/subdirs works correctly. - Making
TestHostandObjectModeldependencies unconditional inMicrosoft.NET.Test.Sdk/Microsoft.TestPlatform.TestHostfornet462is intentional and necessary for the compile-time[RunAsExe]type resolution in run-as-exe projects; Library projects are unaffected at runtime because the integration targets gate all content on$(OutputType)=='Exe'.
Progress summary (all rounds)
| # | Finding | Status |
|---|---|---|
| 4 | DefaultTestHostManager latent null — testHostProcessPath unassigned |
✅ Resolved (Round 2) |
| 5 | DotnetTestHostManager.TryGetExePathFromDll called unconditionally |
✅ Resolved (Round 2) |
| C | AssemblyResolver: Process.GetCurrentProcess() called twice |
✅ Resolved (Round 5) |
| 3 | AssemblyResolver._isTestHost: overly-broad StartsWith("Test") guard |
✅ Resolved (Round 5) |
| B | TestHost.targets duplicate ItemGroup conditions → Library projects got double testhost files |
✅ Resolved (Round 7) |
Still open from prior rounds
| # | File | Dimension | Severity |
|---|---|---|---|
| 1 | PublicAPI.Shipped.txt:676 |
Public API Surface Protection | |
| 2 | AssemblyMetadataProvider.cs:217 |
Null Safety & Boundary Validation | 🐛 Bug |
| A | DotnetTestHostManager.cs:1068 |
Process Architecture & Host Resolution | 🐛 Silent failure (Linux/macOS) |
| D | SourceDetailHelper.cs:24 |
Null Safety & Boundary Validation | 🐛 Correctness |
| E | DefaultTestHostManager.cs:73 |
Environment Variable & Feature Flag Contracts | 🔧 Maintenance concern |
Inline comments for all five remain on their respective threads from prior rounds.
PR Description Alignment
The description continues to accurately describe the WIP scope and intentional probes. The new commit's scope (MSBuild pack re-port) is not mentioned in the description, but this is a mechanics/housekeeping change that doesn't warrant updating the feature description. No misalignment. ✅
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
|
Warning The Addressed the three non-outdated expert-reviewer findings: Finding 1 — PublicAPI.Shipped.txt breaking change Finding 2 — Namespace check in Finding 4 — Finding 5 (non-outdated variant at line 1068) — Cross-platform
|
|
There is a high chance of regressing behavior for existing users, instead we look into running mtp enabled exes under vstest, so test frameworks can drop vstest entirely and still stay compatible. in #16201 |
Reviving the experiment that lets a test project run as its own
.exeinstead of going throughtesthost.exe. A test framework opts in with an assembly-level[RunAsExe]attribute; vstest reads it, marks the source asExecutionPreference.RunAsExe, turns off app domains, and launches the test's own exe as the host.The branch was ~260 commits behind, so most of this is just merging current main back in and getting it to build. The acceptance-test caching I had here is already in main now (
IntegrationTestBuild), so I dropped my copy and kept main's.This is WIP and intentionally rough. A few probes are still hard-coded to force the new path so I can see what falls over:
DefaultTestHostManagerthrows instead of using the built-in/shared testhost.DotnetTestHostManageralways resolves the.exenext to the.dll.TestEngineforces isolation (no in-process run).RunConfiguration.DisableSharedTestHostdefaults totrue.So a lot of unit/acceptance tests fail by design for now. Opening as draft to get it building in CI and to pick the work back up.
Builds locally with
build.cmd(Debug, all TFMs).