Fix subscription ID tests when logged in with Azure CLI#2391
Fix subscription ID tests when logged in with Azure CLI#2391alzimmermsft merged 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates subscription-related unit tests to be resilient when a developer is signed into Azure CLI by allowing tests to use the Azure CLI default subscription when present, otherwise falling back to AZURE_SUBSCRIPTION_ID.
Changes:
- Adjusts
EnvironmentHelperssubscription helpers (addsClearAzureSubscriptionId, changesSetAzureSubscriptionIdbehavior/signature). - Exposes
CommandHelper.GetProfileSubscription()and uses it in default-subscription resolution. - Updates subscription-focused unit tests to assert against “CLI subscription if available, else env var”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.ContainerApps/tests/Azure.Mcp.Tools.ContainerApps.UnitTests/ContainerApp/ContainerAppListCommandTests.cs | Clears AZURE_SUBSCRIPTION_ID for validation tests and partially restores it afterward. |
| core/Microsoft.Mcp.Core/src/Helpers/EnvironmentVariableHelpers.cs | Adds ClearAzureSubscriptionId; changes SetAzureSubscriptionId signature/behavior to prefer Azure CLI subscription. |
| core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs | Uses GetProfileSubscription() and exposes it internally for reuse. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperTests.cs | Updates expectations to allow Azure CLI subscription to win; adds tests that reference profile subscription. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Subscription/SubscriptionCommandTests.cs | Updates expectations to allow Azure CLI subscription to win when env var is “set” for tests. |
jongio
left a comment
There was a problem hiding this comment.
CI is red - the signature change on SetAzureSubscriptionId breaks RegistryListCommandTests.cs:57 which passes null. Beyond that compile fix, I think the approach here has a deeper issue worth discussing.
The core problem: SetAzureSubscriptionId now conditionally doesn't set anything and returns a value unrelated to its input. On a CLI-logged-in machine, calling Set("env-subs") silently no-ops and returns the CLI profile subscription. Set/Get isn't round-trippable, and callers can't tell whether their value was actually written. That's a correctness hazard for any future caller.
The test assertions also become tautological when CLI is logged in - both sides of Assert.Equal(subscription, actual) resolve through the same s_profileDefault.Value cache, so the tests prove cache consistency rather than subscription resolution behavior.
A simpler alternative: keep SetAzureSubscriptionId as a pure setter (always writes the env var), and have tests derive the expected value via CommandHelper.GetDefaultSubscription(). That way the env var IS written (no hidden no-op), the resolution precedence is tested for real, and the assertions are meaningful regardless of whether CLI is logged in.
jongio
left a comment
There was a problem hiding this comment.
The iteration looks good - all my previous concerns are addressed. Moving SetAzureSubscriptionId to TestEnvironment as a pure setter and using GetDefaultSubscription() / GetProfileSubscription() to derive expected values is the right approach. Clean separation between production and test code.
One small nit for a future pass: the ContainerAppListCommandTests.cs finally block could add an else { TestEnvironment.ClearAzureSubscriptionId(); } for defensive cleanup when the original value was null. Not blocking - the env var is already cleared from the try block.
What does this PR do?
Fixes testing pain point where some tests fail if you're signed into Azure CLI due to
subscriptionfalling back to the logged in Azure CLI subscription if it's available.Tests targeting
subscriptiontesting now use the Azure CLI subscription if it's set, otherwise fallback to previous handling with using the injected / omittedAZURE_SUBSCRIPTION_IDenvironment variable.GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline