Add dotnet test device selection support for MAUI - #54295
Conversation
9e813f8 to
0dfbb74
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformTestCommand.cs:200
- When
buildOptions.IsInteractiveis false,RunCommandSelector.TrySelectTargetFramework(...)already writes a detailed error message (including the available TFMs) to stderr before returningfalse. Throwing a newGracefulExceptionwith the same message causes duplicated output. Consider avoiding the extra exception message in non-interactive mode (for example by returning an exit code to the caller, or by throwing an exception marked as already displayed).
if (!RunCommandSelector.TrySelectTargetFramework(frameworks, buildOptions.IsInteractive, out string? selectedFramework))
{
// Error already written to stderr by TrySelectTargetFramework
throw new GracefulException(
string.Format(CliCommandStrings.RunCommandExceptionUnableToRunSpecifyFramework, "--framework"));
}
src/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs:358
RunCommandSelector.TrySelectDevice(...)prints a full non-interactive error (including the available devices list) before returningfalse. Throwing anotherGracefulExceptionwith the same top-line message will duplicate output. Consider throwing an exception that doesn’t print a second message (or refactor to propagate a failure/exit code up to the command) so users see a single error block.
{
// Device selection failed - error already written to stderr by TrySelectDevice
throw new GracefulException(
string.Format(CliCommandStrings.RunCommandExceptionUnableToRunSpecifyDevice, "--device"));
}
| * **`--list-devices`** works the same as with `dotnet run`. | ||
|
|
||
| * **`-e` / `--environment`** — `dotnet test` already supports `-e` to | ||
| pass environment variables to the test process. This existing | ||
| behavior is unchanged. | ||
|
|
||
| ## New Command-line Switches | ||
|
|
||
| So far, it feels like no new subcommand is needed. In interactive | ||
| mode, `dotnet run` will now prompt to select a `$(TargetFramework)` | ||
| for all multi-targeted projects. Platform-specific projects like | ||
| Android, iOS, etc. will prompt for device selection. | ||
| Android, iOS, etc. will prompt for device selection. `dotnet test` | ||
| shares the `--list-devices` and `--device` switches described below. | ||
|
|
||
| `dotnet run --list-devices` will: | ||
| `dotnet run --list-devices` (or `dotnet test --list-devices`) will: | ||
|
|
There was a problem hiding this comment.
Filed #54378 to track this. --list-devices for dotnet test was intentionally deferred to a later phase — this PR only covers --device and --interactive (Phase 1).
0dfbb74 to
b0e973e
Compare
Update dotnet-run-for-maui.md spec with dotnet test behavior section. Add --device and --interactive options to dotnet test (MTP path). When --device is specified without -f, prompt for target framework selection since devices are platform-specific. For each TFM, check for ComputeAvailableDevices target and use RunCommandSelector to handle device selection (auto-select single device, error in non-interactive mode with multiple devices). Includes DotnetTestDevices test asset and 7 integration tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
b0e973e to
9739302
Compare
Evangelink
left a comment
There was a problem hiding this comment.
Reviewed the PR end-to-end. Posting comments only (no change requested). The two threading findings are the most important to address before merge; the rest are listed for awareness and follow-up.
Summary of findings
| # | Area | File / Line | Severity |
|---|---|---|---|
| 1 | Threading | SolutionAndProjectUtility.cs ~L348 — selector.TrySelectDevice(...) inside Parallel.ForEach |
Likely blocking |
| 2 | Threading | Same site, interactive prompt path | Blocking for interactive multi-project case |
| 3 | Correctness / ordering | SolutionAndProjectUtility.cs ~L362 — SetProperty("Device"/"RuntimeIdentifier") after build |
Major |
| 4 | Defensive coding | MicrosoftTestingPlatformTestCommand.cs ~L217 — -p:Device=... injected unconditionally |
Major |
| 5 | Documentation accuracy | documentation/specs/dotnet-run-for-maui.md L143/L155/L157 — --list-devices for dotnet test not implemented |
Major |
| 6 | Test coverage | GivenDotnetTestSelectsDevice.cs — several missing scenarios |
Major |
| 7 | Performance | SolutionAndProjectUtility.cs ~L342 — per-TFM RunCommandSelector re-evaluates project |
Moderate |
| 8 | Nits | HandleDeviceWithTargetFrameworkSelection |
Minor |
Thanks for the well-structured PR (good doc updates and a dedicated test asset). Inline comments below have suggested fixes for each finding.
| // Add Device to MSBuild args so the build and evaluation see it | ||
| return buildOptions with | ||
| { | ||
| MSBuildArgs = buildOptions.MSBuildArgs.Append($"-p:Device={buildOptions.Device}") |
There was a problem hiding this comment.
🟡 Defensive coding — --device injected unconditionally, even for projects that don't support devices
HandleDeviceWithTargetFrameworkSelection never checks whether the resolved project actually defines a ComputeAvailableDevices target. So if a user mistakenly types dotnet test --device foo against a normal MSTest / xUnit / NUnit project:
- Multi-TFM: they get a forced target-framework prompt (or an error in non-interactive mode) for a flag that has no meaning for their project, and then
-p:Device=foois appended anyway. - Single-TFM:
-p:Device=foois silently appended. Harmless today, but it makes the contract unclear and complicates future work that consumes$(Device)for non-MAUI scenarios.
Compare with TrySelectDeviceForProject in SolutionAndProjectUtility.cs (~L320) which correctly probes projectInstance.Targets.ContainsKey(Constants.ComputeAvailableDevices) first — that guard should also exist on the --device path.
Suggested fix
After resolving the project file in HandleDeviceWithTargetFrameworkSelection, do a cheap targets probe before forcing the TF prompt and before appending the property. Roughly:
if (!projectInstance.Targets.ContainsKey(Constants.ComputeAvailableDevices))
{
throw new GracefulException(
"--device is only supported for projects that define a ComputeAvailableDevices MSBuild target (e.g. .NET MAUI).");
}For multi-TFM, ideally the check is done per resolved TFM (since the target may be imported conditionally per platform). A pragmatic shortcut: check on the cross-targeting evaluation first; if absent across the board, error out. Localize the message via a new CliCommandStrings resource (and an English .resx entry for translators to follow up on).
There was a problem hiding this comment.
The ComputeAvailableDevices target isn't required for the --device switch, this behavior is the same as dotnet run.
Passing the -p:Device global property is just passed along no matter what. I can't think of a platform or scenario that would use $(Device) and not have ComputeAvailableDevices, though.
| CommonOptions.CreateVerbosityOption(), | ||
| CommonOptions.CreateNoLogoOption()); | ||
|
|
||
| using var selector = new RunCommandSelector( |
There was a problem hiding this comment.
🟢 Performance — per-TFM RunCommandSelector re-evaluates the project
Minor: TrySelectDeviceForProject constructs new RunCommandSelector(projectFilePath, ...) and lets it open + evaluate the project from scratch on every call, while the caller already has a fully evaluated ProjectInstance in hand. For a solution with N projects × M TFMs that's N×M extra evaluations on top of the ones GetProjectProperties already does — non-trivial on large MAUI solutions.
Not a correctness issue, and not a blocker for this PR; consider as follow-up. Options:
- Pass the already-evaluated
ProjectInstance(and the sharedEvaluationContext) into a newRunCommandSelectorconstructor overload so it skips its ownOpenProjectIfNeededcall. - Or extract the
ComputeAvailableDevicesbuild call into a small free helper that takes aProjectInstancedirectly, sodotnet testcan avoidRunCommandSelectorentirely on this path.
There was a problem hiding this comment.
Partially addressed in f6298f2 — SelectDevicesBeforeBuild now has an overload that accepts a shared ProjectCollection and EvaluationContext, avoiding redundant evaluations in the solution path. The RunCommandSelector itself still does its own evaluation since it needs to call Build() for ComputeAvailableDevices, but the outer evaluation layer no longer doubles up.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Protects against concurrent BuildManager.DefaultBuildManager access when a solution has multiple MAUI projects evaluated in parallel. Also serializes interactive Spectre.Console device prompts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a device provides a RuntimeIdentifier, the build output must match. Previously, device selection happened after the build, so the build did not include the device-provided RID. Now for projects with ComputeAvailableDevices, device selection runs before the build and each TFM is built separately with its device/RID injected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove TrySelectDeviceForProject (post-build fallback) and DeviceSelectionDone flag. Both project and solution paths now use SelectDevicesBeforeBuild as the single device selection mechanism, eliminating duplicate prompting paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Process device projects sequentially in GetProjectsProperties to avoid BuildManager.DefaultBuildManager race (in-process MSBuild singleton) - Skip device selection when Device is already set via -p:Device=... - Add tests: ItRunsDeviceProjectsInSolution, ItAcceptsDeviceViaMSBuildProperty, ItShowsBuildErrorWhenBuildFailsAfterDeviceSelection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Propagate non-zero exit codes from BuildPerTfmWithDevices in the solution path instead of silently swallowing them - Add SelectDevicesBeforeBuild overload that accepts ProjectCollection and EvaluationContext to reuse shared evaluation state, avoiding redundant project evaluation for non-device projects Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Thread solution per-project Configuration/Platform through BuildPerTfmWithDevices so device projects use correct mappings - Pass HasNoRestore || HasNoBuild to TrySelectDevice, matching dotnet run behavior where --no-build implies --no-restore - Respect TestTfmsInParallel=false by grouping device project modules into a single sequential group when parallelism is disabled - Make EvaluateProject internal for cross-class access Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously Configuration/Platform from solution project mappings were only passed to the evaluation step but not to BuildOrRestoreProjectOrSolution, causing per-TFM device builds to use default config instead of the solution-mapped config. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove redundant collection.UnloadAllProjects() (using disposes it) - Remove unnecessary EvaluationContext for single evaluation - Materialize Append results with collection expressions - Remove unused using directive Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merge the two overloads and private Core method into one method with optional ProjectCollection/EvaluationContext parameters. When not provided, creates an owned collection that is disposed; when provided (solution path), reuses the caller's collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract AnalyzeStandardTestMSBuildArgs helper to deduplicate 4 identical 5-option AnalyzeMSBuildArguments call sites - Return DeviceSelectionResult record from SelectDevicesBeforeBuild that includes TestTfmsInParallel, eliminating redundant EvaluateProject call in BuildPerTfmWithDevices - Keep module groups from GetProjectProperties directly instead of flattening to List<TestModule> and rebuilding; only merge into sequential group when TestTfmsInParallel is false - Revert EvaluateProject back to private visibility Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On macOS ARM64 Helix machines, the net9.0 shared framework is x86_64 only. Setting RuntimeIdentifier=osx-arm64 on net9.0 test devices causes libhostpolicy.dylib architecture mismatch (exit code 130). Remove RuntimeIdentifier from SingleDevice and net9.0 downlevel devices since those paths run on both TFMs. Keep it on the current TFM devices which only run with --framework and always have the matching shared framework. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Adds
--deviceoption todotnet test(MTP path) for .NET MAUI projects, enabling per-target-framework device selection.Behavior
dotnet testiterates all target frameworks without prompting (existing behavior unchanged)ComputeAvailableDevicestarget exists, usesRunCommandSelectorto handle device selection (auto-select single device, error in non-interactive mode with multiple devices)--device: When specified without-f, forces a target framework prompt since devices are platform-specificdotnet run).Changes
TestCommandDefinition.MicrosoftTestingPlatform.cs- AddedDeviceOptionOptions.cs/MSBuildUtility.cs-DeviceonBuildOptionsMicrosoftTestingPlatformTestCommand.cs-HandleDeviceWithTargetFrameworkSelection()for--device+ TF selectionSolutionAndProjectUtility.cs-TrySelectDeviceForProject()called per-TFM before running testsdotnet-run-for-maui.md- Updated spec withdotnet testbehavior sectionMTPHelpSnapshotTests.VerifyMTPHelpOutput.verified.txt- Updated snapshot for new--deviceoptionTests
7 integration tests in
GivenDotnetTestSelectsDevicecovering:--devicein non-interactive mode-f+--deviceComputeAvailableDevicestarget