Improve aspire doctor TypeScript AppHost diagnostics across package managers#17214
Conversation
Expands TypeScriptAppHostToolingCheckTests to cover npm, pnpm, Yarn, and Bun (previously only Bun was tested). For each toolchain, verifies that: - A Pass result is returned when all required CLIs are on PATH, with the result Message naming every required command. - A Fail result is returned for each missing CLI, carrying the toolchain's installation link from CommandPathResolver, the Fix string that names the installation display name, and Details produced by GetMissingCommandMessage. Also adds a dedicated theory for the npm toolchain (the only toolchain that requires two CLIs) covering npm-missing-only and npx-missing-only scenarios. Closes #17032 Co-authored-by: Copilot <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17214Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17214" |
There was a problem hiding this comment.
Pull request overview
This PR expands TypeScriptAppHostToolingCheckTests so aspire doctor TypeScript AppHost tooling diagnostics are exercised across npm, Bun, Yarn, and pnpm.
Changes:
- Converts the existing pass/fail tooling checks into package-manager theories.
- Adds validation for failure status, category, links, fixes, details, and per-command result names.
- Adds npm-specific partial-missing coverage for
npmvsnpx.
ab5e3e8 to
e5a3f85
Compare
Previously, when a TypeScript AppHost workspace used Yarn Classic ([email protected] or a yarn.lock with the v1 marker), TypeScriptAppHostToolchainResolver.Resolve would throw InvalidOperationException, the exception would bubble out of TypeScriptAppHostToolingCheck.CheckAsync uncaught, and EnvironmentChecker.CheckAllAsync would silently swallow it at Debug level. Net result: 'aspire doctor' produced no TypeScript AppHost output at all for these workspaces. This change catches the InvalidOperationException in TypeScriptAppHostToolingCheck and emits a Fail result instead, including: - Name: typescript-apphost-yarn-classic - Status: Fail - Message: 'TypeScript AppHost does not support Yarn Classic.' - Details: the resolver's user-facing exception text (mentions the specific offending packageManager value or lockfile path, depending on which signal triggered detection) - Fix: 'Upgrade to Yarn 4 or later, or switch to npm, pnpm, or Bun, then rerun aspire doctor.' - Link: https://yarnpkg.com/getting-started/install Adds two tests in TypeScriptAppHostToolingCheckTests covering both detection signals (packageManager field and yarn.lock with v1 marker). Co-authored-by: Copilot <[email protected]>
The previous commit caught plain InvalidOperationException from TypeScriptAppHostToolchainResolver.Resolve and treated every IOE as a Yarn Classic diagnostic. That binding is implicit: if a future contributor adds any other 'throw new InvalidOperationException(...)' inside Resolve (or anything it calls), the doctor check would silently surface it as 'TypeScript AppHost does not support Yarn Classic' with a Yarn install link - misleading users about the actual problem. Introduces YarnClassicNotSupportedException : InvalidOperationException, has the resolver throw that specific type from CreateYarnClassicNotSupportedException, and narrows the catch in TypeScriptAppHostToolingCheck.CheckAsync to the new type. Subclassing InvalidOperationException preserves the resolver's existing public throw contract (callers catching IOE still work). Updates the three existing 'Resolve_When*Yarn*Classic*_Throws' tests in TypeScriptAppHostToolchainResolverTests to assert on the specific exception type. Co-authored-by: Copilot <[email protected]>
PR Testing Report — PR #17214PR Information
CLI Version Verification
Installed from PR comment dogfood instructions via Changes AnalyzedFiles changed (4 prod, 2 test):
Change categories:
Test Scenarios ExecutedAll scenarios use a fresh synthetic TypeScript AppHost ( Scenario 1 — Smoke:
|
| # | Scenario | Status | Notes |
|---|---|---|---|
| 1 | doctor --help smoke |
✅ Passed | --format Json supported |
| 2 | Yarn Classic via packageManager: [email protected] |
✅ Passed | Fail diagnostic correctly emitted; details reference package.json |
| 3 | Yarn Classic via yarn.lock v1 marker |
✅ Passed | Fail diagnostic correctly emitted; details reference yarn.lock |
| 4 | Modern yarn.lock (Yarn 4 metadata) |
✅ Passed | No false positive; tooling check passes |
| 5 | Bun toolchain, bun absent | ✅ Passed | Existing missing-tool path intact; catch is properly scoped |
| 6 | npm tooling baseline | ✅ Passed | Existing pass path intact |
Overall Result
✅ PR VERIFIED
All 6 scenarios passed. The new aspire doctor Yarn Classic diagnostic:
- Correctly identifies both detection paths (packageManager field and yarn.lock v1 marker) and emits a fully-structured Fail result with
Name,Status,Category,Message,Fix,Link,Details,Metadata. - Does not produce false positives for modern Yarn 4 projects without the v1 lockfile marker.
- Does not break adjacent diagnostics — missing-toolchain Fails (e.g., bun absent) still surface as
typescript-apphost-<command>rather than being misclassified. - Is properly scoped via the dedicated exception type (
YarnClassicNotSupportedException : InvalidOperationException), confirmed by Scenario 5 not surfacing Yarn Classic for an unrelated Fail.
Recommendations
- Behavior matches the PR description and test coverage. No regressions observed.
- The PR is currently in draft; once davidfowl's open file-level "nah" review on
TypeScriptAppHostToolingCheck.csis addressed (the newYarnClassicNotSupportedExceptionalready resolves the brittle-catch concern raised in code review), this looks ready to ship.
Artifacts
Captured under each scenario-N-* subdirectory of the test workspace:
doctor.json— raw doctor JSON output per scenario- Synthetic
apphost.ts+package.json(+ optionalyarn.lock) fixtures
Test workspace: C:\Users\dapine\AppData\Local\Temp\aspire-pr-test-fa5c94cbf40f4cdf8827b2e739b88b8b (will be cleaned up).
|
❓ CLI E2E Tests unknown — 86 passed, 0 failed, 1 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26052143080 |
Description
Closes #17032 (part of #16961).
Two changes to
aspire doctor's TypeScript AppHost diagnostics, each targeting a different package-manager scenario.1. Surface Yarn Classic as a real diagnostic (product change)
TypeScriptAppHostToolchainResolver.ResolvethrowsInvalidOperationExceptionwhen it sees Yarn Classic (either"packageManager": "[email protected]"inpackage.jsonor ayarn.lockcontaining the# yarn lockfile v1marker). That exception was bubbling out ofTypeScriptAppHostToolingCheck.CheckAsyncuncaught;EnvironmentChecker.CheckAllAsyncwould swallow it atDebuglog level and continue. The net effect for users:aspire doctorproduced no output at all about the TypeScript AppHost in a Yarn Classic workspace — neither Pass nor Fail.TypeScriptAppHostToolingCheck.CheckAsyncnow catches thatInvalidOperationExceptionand emits a structuredFailresult:Nametypescript-apphost-yarn-classicStatusFailCategoryenvironmentMessageTypeScript AppHost does not support Yarn Classic.DetailspackageManagervalue oryarn.lockpathFixUpgrade to Yarn 4 or later, or switch to npm, pnpm, or Bun, then rerun 'aspire doctor'.Linkhttps://yarnpkg.com/getting-started/installTwo new tests in
TypeScriptAppHostToolingCheckTestscover both detection signals (thepackageManagerfield and theyarn.lockv1 marker).2. Expand existing pass/fail coverage to all four package managers (test change)
tests/Aspire.Cli.Tests/Commands/TypeScriptAppHostToolingCheckTests.cs:CheckAsync_ReturnsPass_WhenConfiguredToolchainIsAvailableis now a[Theory]parameterized acrossNpm,Pnpm,Yarn, andBun.CheckAsync_ReturnsFail_WhenConfiguredToolchainIsMissingis now a[Theory]parameterized acrossNpm,Pnpm,Yarn, andBun. AssertsStatus=Fail,Name=typescript-apphost-{command},LinkfromCommandPathResolver,Fix = "Install {displayName} tooling and rerun 'aspire doctor'.", andDetailsfromGetMissingCommandMessage. TheNpmcase correctly produces twoFailentries — one fornpm, one fornpx— becauseGetRequiredCommands(Npm) = ["npm", "npx"].CheckAsync_ReturnsFailOnlyForMissingNpmCommand_WhenTheOtherIsPresenttheory covers the partial-npm cases (npm present / npx missing, and vice versa).CheckAsync_Skips_WhenNoTypeScriptAppHostExistsis unchanged.Toolchain enum values are passed via
nameof(TypeScriptAppHostToolchain.X)and parsed inside the test method to avoid putting aninternalenum in a public test method signature.Verification
Build: clean (0 warnings, 0 errors). Tests: 58/58 passing locally.
Checklist