Add shared test dependencies guidance to test review guidelines#15861
Merged
Add shared test dependencies guidance to test review guidelines#15861
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15861Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15861" |
JamesNK
force-pushed
the
test-guidelines-shared-dependencies
branch
from
bf1e9a7 to
2383ca5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new aspire dashboard run CLI workflow (including localized strings and tests), refactors CLI process execution behind a generic IProcessExecutionFactory abstraction, and adds new dashboard startup exit codes plus tests around startup failure modes. It also updates the test review guidelines with advice to reuse shared test fakes.
Changes:
- Add
aspire dashboard/aspire dashboard runcommands, resources, and tests (unit + E2E). - Refactor CLI process launching from
DotNetCliExecutionFactorytoIProcessExecutionFactory+ProcessExecution/LayoutProcessRunner. - Standardize dashboard startup exit codes and add coverage for validation failure + address-in-use.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Dashboard.Tests/Integration/StartupTests.cs | Adds integration tests for dashboard startup exit codes (validation failure, address in use). |
| tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs | Updates CLI test DI wiring for new process execution abstractions and dashboard commands. |
| tests/Aspire.Cli.Tests/TestServices/TestProcessExecutionFactory.cs | Renames/rewires test process execution factory for the new IProcessExecutionFactory API. |
| tests/Aspire.Cli.Tests/TestServices/TestDotNetCliRunner.cs | Updates test runner callbacks to use ProcessInvocationOptions. |
| tests/Aspire.Cli.Tests/Templating/DotNetTemplateFactoryTests.cs | Updates test stubs to ProcessInvocationOptions. |
| tests/Aspire.Cli.Tests/Projects/ProjectUpdaterTests.cs | Updates comments/signatures to ProcessInvocationOptions. |
| tests/Aspire.Cli.Tests/DotNet/DotNetCliRunnerTests.cs | Updates tests to new invocation options type. |
| tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs | Updates tests to new invocation options type. |
| tests/Aspire.Cli.Tests/Commands/DashboardRunCommandTests.cs | Adds unit tests for dashboard run argument/env forwarding and failure modes. |
| tests/Aspire.Cli.EndToEnd.Tests/DashboardOtelTracesTests.cs | Adds an E2E test for running the dashboard and querying aspire otel traces against it. |
| src/Shared/DashboardExitCodes.cs | Adds shared dashboard startup exit code constants. |
| src/Aspire.Dashboard/DashboardWebApplication.cs | Returns stable exit codes for validation failures, address-in-use, and unexpected startup errors. |
| src/Aspire.Dashboard/Aspire.Dashboard.csproj | Links in shared DashboardExitCodes.cs. |
| src/Aspire.Cli/Utils/AppHostHelper.cs | Updates helper APIs to use ProcessInvocationOptions. |
| src/Aspire.Cli/Templating/DotNetTemplateFactory.cs | Updates dotnet invocation options type. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.zh-Hant.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.zh-Hans.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.tr.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ru.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.pt-BR.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.pl.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ko.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ja.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.it.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.fr.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.es.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.de.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.cs.xlf | Adds dashboard command localization file (new resource set). |
| src/Aspire.Cli/Resources/DashboardCommandStrings.resx | Adds dashboard command strings. |
| src/Aspire.Cli/Resources/DashboardCommandStrings.Designer.cs | Adds generated strongly-typed resource accessor. |
| src/Aspire.Cli/Projects/PrebuiltAppHostServer.cs | Updates build invocation options type. |
| src/Aspire.Cli/Projects/DotNetBasedAppHostServerProject.cs | Updates build invocation options type. |
| src/Aspire.Cli/Projects/DotNetAppHostProject.cs | Updates dotnet invocations and options type throughout apphost operations. |
| src/Aspire.Cli/Program.cs | Registers new process execution services and dashboard commands in DI. |
| src/Aspire.Cli/NuGet/NuGetPackageCache.cs | Updates invocation options type. |
| src/Aspire.Cli/NuGet/BundleNuGetService.cs | Switches to injected LayoutProcessRunner instance API. |
| src/Aspire.Cli/NuGet/BundleNuGetPackageCache.cs | Switches to injected LayoutProcessRunner instance API. |
| src/Aspire.Cli/Layout/LayoutProcessRunner.cs | Refactors from static Process usage to IProcessExecutionFactory-backed execution. |
| src/Aspire.Cli/DotNet/ProcessExecutionFactory.cs | Adds factory creating real OS-backed IProcessExecution instances. |
| src/Aspire.Cli/DotNet/ProcessExecution.cs | Adds IProcessExecution implementation for real processes (stream forwarding, kill, dispose). |
| src/Aspire.Cli/DotNet/IProcessExecutionFactory.cs | Renames/expands execution factory interface from dotnet-specific to generic process execution. |
| src/Aspire.Cli/DotNet/IProcessExecution.cs | Renames/expands execution interface, adding kill/dispose. |
| src/Aspire.Cli/DotNet/DotNetCliRunner.cs | Updates runner to use generic process execution factory and resolve dotnet path/environment. |
| src/Aspire.Cli/DotNet/DotNetCliExecutionFactory.cs | Removes the old dotnet-specific execution factory implementation. |
| src/Aspire.Cli/Commands/RootCommand.cs | Adds dashboard command to the CLI root. |
| src/Aspire.Cli/Commands/RestoreCommand.cs | Updates invocation options type. |
| src/Aspire.Cli/Commands/InitCommand.cs | Updates invocation options type. |
| src/Aspire.Cli/Commands/ExecCommand.cs | Updates invocation options type. |
| src/Aspire.Cli/Commands/DashboardRunCommand.cs | Adds the dashboard run implementation (args/env handling, readiness wait, exit-code mapping). |
| src/Aspire.Cli/Commands/DashboardCommand.cs | Adds the parent dashboard command. |
| src/Aspire.Cli/Aspire.Cli.csproj | Links shared DashboardExitCodes and TokenGenerator into CLI build. |
| .github/instructions/test-review-guidelines.instructions.md | Adds guidance to reuse shared test service implementations. |
joperezr
approved these changes
Apr 6, 2026
Member
joperezr
left a comment
joperezr
left a comment
There was a problem hiding this comment.
Looks good overall — one suggestion to broaden the example so it applies better across all test projects, not just CLI tests.
|
|
||
| ## Shared Test Dependencies | ||
|
|
||
| When writing tests, prefer using shared test service implementations from a common location (e.g., `Aspire.Cli.Tests/TestServices`) rather than creating private implementation classes within individual test files. Reusing existing test fakes and helpers keeps tests consistent, reduces duplication, and makes maintenance easier. Do not create private test classes when a shared one already exists or can be extended. |
Member
There was a problem hiding this comment.
The only concrete example is Aspire.Cli.Tests/TestServices, but the TestServices/ convention only exists in that one project. The file applies to tests/**/*.cs (all test projects). The repo has multiple shared test locations with different conventions:
tests/Shared/— cross-project helpers (TestDashboardClient, Hex1b automation, etc.)tests/Aspire.TestUtilities/— cross-cutting infrastructure (QuarantinedTestAttribute, TestLoggerFactory)tests/Aspire.Components.Common.TestUtilities/— conformance testing for client integrations- Per-project directories like
Helpers/in Aspire.Hosting.Tests
An agent working on non-CLI tests could be misled into referencing CLI-specific fakes or not knowing where to look. Consider broadening the example, e.g.: "shared test service implementations (e.g., project-level TestServices/ or Helpers/ directories, or the cross-project tests/Shared/ folder)".
Contributor
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24061506518 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add guidance to the test review guidelines instructing agents to prefer shared test service implementations (e.g.,
Aspire.Cli.Tests/TestServices) instead of creating private implementation classes within individual test files. This reduces duplication and keeps test fakes consistent.Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: