Migrate Cosmos to recordings #1402
Conversation
|
/azp run mcp - pullrequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
scbedd
requested review from
JasonYeMSFT,
JonathanCrd,
conniey,
hallipr,
joshfree and
vukelich
There was a problem hiding this comment.
Pull request overview
This PR migrates Cosmos tool tests from live-only tests to recorded tests, enabling faster test execution during playback and reducing dependency on live Azure resources.
- Migrated CosmosCommandTests to inherit from RecordedCommandTestsBase instead of CommandTestsBase
- Injected IHttpClientFactory into CosmosService to enable HTTP request recording/playback
- Added RecordingOptions support to the base test infrastructure for configuring test proxy behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Cosmos/tests/Azure.Mcp.Tools.Cosmos.LiveTests/assets.json | New assets.json file created for test proxy configuration with the appropriate TagPrefix for Cosmos LiveTests |
| tools/Azure.Mcp.Tools.Cosmos/tests/Azure.Mcp.Tools.Cosmos.LiveTests/CosmosDbFixture.cs | Removed the fixture class as it's no longer needed with recorded tests - test data will come from recordings |
| tools/Azure.Mcp.Tools.Cosmos/tests/Azure.Mcp.Tools.Cosmos.LiveTests/CosmosCommandTests.cs | Migrated to RecordedCommandTestsBase, added sanitizers and matcher configuration, implemented variable registration for deterministic playback |
| tools/Azure.Mcp.Tools.Cosmos/src/Services/CosmosService.cs | Injected IHttpClientFactory and configured CosmosClient to use it for HTTP requests, enabling recording/playback |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Tests/Client/RecordedCommandTestsBase.cs | Added RecordingOptions property and SetRecordingOptions method to support per-test recording configuration; simplified sanitizer initialization |
| /// 3493 = $..name | ||
| /// </summary> | ||
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "3493"]; |
There was a problem hiding this comment.
The sanitizer ID should be "AZSDK3493" instead of "3493". All other tools in the codebase consistently use the "AZSDK" prefix for this sanitizer (e.g., AcrCommandTests, AppConfigCommandTests, QuotaCommandTests, SearchCommandTests, SqlCommandTests, SignalRCommandTests).
| /// 3493 = $..name | |
| /// </summary> | |
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "3493"]; | |
| /// AZSDK3493 = $..name | |
| /// </summary> | |
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "AZSDK3493"]; |
| { "database", dbName! }, | ||
| { "account", resourceBaseName }, | ||
| { "database", dbName }, | ||
| { "container", containerName! } |
There was a problem hiding this comment.
The null-forgiving operator (!) is unnecessary here. The variable 'containerName' is declared as a non-nullable string, and there's an assertion on line 152 that verifies it's not null or empty. The null-forgiving operator should be removed for cleaner code.
| { "container", containerName! } | |
| { "container", containerName } |
| /// 3493 = $..name | ||
| /// </summary> | ||
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "3493"]; |
There was a problem hiding this comment.
The comment should reference "AZSDK3493" instead of "3493" to match the correct sanitizer ID. This is consistent with how other tools document this sanitizer (e.g., "AZSDK3493 = $..name" in AcrCommandTests, AppConfigCommandTests, QuotaCommandTests, etc.).
| /// 3493 = $..name | |
| /// </summary> | |
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "3493"]; | |
| /// AZSDK3493 = $..name | |
| /// </summary> | |
| public override List<string> DisabledDefaultSanitizers => [.. base.DisabledDefaultSanitizers, "AZSDK3493"]; |
|
Followup filed #1421 |
2 participants
What does this PR do?
Migrates Cosmos to recorded live tests.
GitHub issue number?
Resolves #1282
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline