Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/agents/onboarding.agent.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ eng/common/TestResources/New-TestResources.ps1 `

Azure resource commands **require recorded live tests**. See `docs/recorded-tests.md` for the record/playback workflow.

For tools marked `LocalRequired = true`, every applicable test in a class extending `RecordedCommandTestsBase` must call `AssertLocalToolIsUnavailableInHttpMode(toolName)` and return early when it returns `true`. This verifies that remote HTTP mode excludes the local-only tool.

### Testing Your Local Build

Point your `mcp.json` at the freshly built binary:
Expand Down
1 change: 1 addition & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
- Always convert `tool` services to inject `IHttpClientFactory` into its clients and use `IHttpClientFactory.CreateClient` method to instantiate the `HttpClient` for usage in the tool classes' methods.
- If `IHttpClientFactory` is already injected into the client, ensure that `IHttpClientFactory.CreateClient` is used to instantiate the `HttpClient`. If this is done, then no further action is needed.
- Always re-parent test classes parented by `CommandTestsBase` to `RecordedCommandTestsBase`. This will require minor fixture adjustments.
- In test classes extending `RecordedCommandTestsBase`, every test for a `LocalRequired = true` tool must call `AssertLocalToolIsUnavailableInHttpMode(toolName)` and return early when it returns `true`. This verifies that remote HTTP mode does not expose the local-only tool.
- Always generate a new `assets.json` file alongside the livetest csproj file if one does not exist. This file should contain the following content:
```jsonc
{
Expand Down
13 changes: 13 additions & 0 deletions .github/skills/add-azure-mcp-tools/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,19 @@ public class {Toolset}CommandTests(ITestOutputHelper output, TestProxyFixture fi
}
```

#### `LocalRequired` tools

Remote HTTP mode intentionally excludes tools marked `LocalRequired = true`. Every recorded test for such a tool must verify that exclusion and return before exercising local-only behavior:

```csharp
if (await AssertLocalToolIsUnavailableInHttpMode("{toolset}_{resource}_{operation}"))
{
return;
}
```

Use the inherited helper in every applicable test in a class extending `RecordedCommandTestsBase`; do not duplicate the transport check or unavailable-tool assertions.

### 3c. Record and Verify

#### Create assets.json
Expand Down
2 changes: 2 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,8 @@ Command unit tests should extend `SubscriptionCommandUnitTestsBase<TCommand, TSe
### Live Testing Requirements
Azure service commands require live tests to validate functionality against actual Azure resources. Live tests must be recorded for playback using `RecordedCommandTestsBase`. See `/docs/recorded-tests.md` for the full recording workflow, sanitizer configuration, and migration guide.

Tests for tools marked `LocalRequired = true` need special handling in classes extending `RecordedCommandTestsBase`. Start each such test with `AssertLocalToolIsUnavailableInHttpMode(toolName)` and return early when it returns `true`; the helper verifies that the tool is unavailable in remote HTTP mode before local-only behavior is tested in other transports.

### Live Test Infrastructure
Azure service commands require Bicep templates for test resources:
```powershell
Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ Do not assume the Pull Request pipeline will always ingest a missing package aut

4. **Follow implementation guidelines** in [.github/skills/add-azure-mcp-tools/SKILL.md](https://github.com/microsoft/mcp/blob/main/.github/skills/add-azure-mcp-tools/SKILL.md)

Tools marked `LocalRequired = true` need special recorded-test handling. In every applicable test in a class extending `RecordedCommandTestsBase`, call `AssertLocalToolIsUnavailableInHttpMode(toolName)` and return early when it returns `true` so the test verifies that remote HTTP mode excludes the local-only tool.

5. **Update documentation**:
- Add the new command to [/servers/Azure.Mcp.Server/docs/azmcp-commands.md](https://github.com/microsoft/mcp/blob/main/servers/Azure.Mcp.Server/docs/azmcp-commands.md)
- Run `.\eng\scripts\Update-AzCommandsMetadata.ps1` to update tool metadata in azmcp-commands.md (required for CI)
Expand Down
72 changes: 44 additions & 28 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/ClientToolTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Net;
using System.Text.Json;
using Microsoft.Mcp.Tests;
using Microsoft.Mcp.Tests.Client;
Expand Down Expand Up @@ -62,49 +63,49 @@ public async Task Client_Should_Ping_Server_Successfully()
// The `ping` method was removed in the MCP 2026-07-28 protocol revision. The client
// negotiates the modern protocol, so the server rejects ping as unavailable.
// (Method name is retained so the recorded playback session continues to match.)
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () =>
await Client.PingAsync(cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("ping", ex.Message, StringComparison.OrdinalIgnoreCase);
await AssertMethodNotFoundAsync(
async () => await Client.PingAsync(cancellationToken: TestContext.Current.CancellationToken),
"ping");
}

[Fact]
public async Task Should_Error_When_Resources_List_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.ListResourcesAsync(cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
async () => await Client.ListResourcesAsync(cancellationToken: TestContext.Current.CancellationToken),
"resources/list");
}

[Fact]
public async Task Should_Error_When_Resources_Read_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.ReadResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
async () => await Client.ReadResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken),
"resources/read");
}

[Fact]
public async Task Should_Error_When_Resources_Templates_List_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.ListResourceTemplatesAsync(cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
async () => await Client.ListResourceTemplatesAsync(cancellationToken: TestContext.Current.CancellationToken),
"resources/templates/list");
}

[Fact]
public async Task Should_Error_When_Resources_Subscribe_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.SubscribeToResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
() => Client.SubscribeToResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken),
"resources/subscribe");
}

[Fact]
public async Task Should_Error_When_Resources_Unsubscribe_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.UnsubscribeFromResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
() => Client.UnsubscribeFromResourceAsync("test://resource", cancellationToken: TestContext.Current.CancellationToken),
"resources/unsubscribe");
}

[Fact]
Expand All @@ -114,27 +115,42 @@ public async Task Should_Not_Hang_On_Logging_SetLevel_Not_Supported()
// The method is no longer supported; per-request log level is now set via
// _meta/io.modelcontextprotocol/logLevel. The call should throw rather than hang.
#pragma warning disable MCP9005 // Type or member is obsolete
var ex = await Assert.ThrowsAsync<McpProtocolException>(
async () => await Client.SetLoggingLevelAsync(LoggingLevel.Info,
cancellationToken: TestContext.Current.CancellationToken));
await AssertMethodNotFoundAsync(
() => Client.SetLoggingLevelAsync(LoggingLevel.Info,
cancellationToken: TestContext.Current.CancellationToken),
"logging/setLevel");
#pragma warning restore MCP9005 // Type or member is obsolete
Assert.Contains("logging/setLevel", ex.Message, StringComparison.OrdinalIgnoreCase);
}

[Fact]
public async Task Should_Error_When_Prompts_List_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.ListPromptsAsync(cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
async () => await Client.ListPromptsAsync(cancellationToken: TestContext.Current.CancellationToken),
"prompts/list");
}

[Fact]
public async Task Should_Error_When_Prompts_Get_Not_Supported()
{
var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => await Client.GetPromptAsync("unsupported_prompt", cancellationToken: TestContext.Current.CancellationToken));
Assert.Contains("Request failed", ex.Message);
Assert.Equal(McpErrorCode.MethodNotFound, ex.ErrorCode);
await AssertMethodNotFoundAsync(
async () => await Client.GetPromptAsync("unsupported_prompt", cancellationToken: TestContext.Current.CancellationToken),
"prompts/get");
}

private static async Task AssertMethodNotFoundAsync(Func<Task> action, string method)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment in this code that explains why we're treating the modes separately. Your comment may be anecdotal and point to observations of the specific C# MCP SDK version we're seeing this transport-dependent exception behavior with.

Adding this comment will help other maintainers know why this is happening and crucially learn its outside of our control and outside the scope of us understanding. If the C# MCP SDK behavior changes to be consistent between the transport modes, then a contextual comment here will empower someone to make a quick reactive change without needing to investigate the history of this code.

As a general rule, if we ever learn something unintuitive that causes us to conditionalize code, then it's worth documenting in code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this is worth bringing up with the C# MCP SDK as well. I'd like to learn if this is intentional or not, and if it is intentional, to learn how we might have other error handling scenarios wrong.

{
if (string.Equals(Environment.GetEnvironmentVariable("MCP_TEST_TRANSPORT"), "http", StringComparison.OrdinalIgnoreCase))
{
var exception = await Assert.ThrowsAsync<HttpRequestException>(action);
Assert.Equal(HttpStatusCode.NotFound, exception.StatusCode);
Assert.Contains(method, exception.Message, StringComparison.OrdinalIgnoreCase);
return;
}

var protocolException = await Assert.ThrowsAsync<McpProtocolException>(action);
Assert.Equal(McpErrorCode.MethodNotFound, protocolException.ErrorCode);
Assert.Contains(method, protocolException.Message, StringComparison.OrdinalIgnoreCase);
}

public override List<BodyRegexSanitizer> BodyRegexSanitizers =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,33 @@ public static async Task<Process> StartHttpServerProcessAndWaitForReadinessAsync
output,
disableAuthentication);

// Invert: disableAuthentication=false means authenticationEnabled=true
await WaitForServerReadinessAsync(serverUrl, timeoutSeconds, pollIntervalMs, authenticationEnabled: !disableAuthentication);
using var readinessCancellation = new CancellationTokenSource();
var readinessTask = WaitForServerReadinessAsync(
serverUrl,
timeoutSeconds,
pollIntervalMs,
authenticationEnabled: !disableAuthentication,
readinessCancellation.Token);
var processExitTask = process.WaitForExitAsync();

Comment on lines +245 to +253

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This process start code is not using the Xunit.TestContext.Current.CancellationToken.

Use that token as a CancellationToken to process.WaitForExitAsync();

Leverage CancellationTokenSource.CreateLinkedTokenSource() when making new CancellationTokenSources. using var readinessCancellation = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken);

You will want to think about what happens if the harnesses cancellation is set and if you need to adjust this code accordingly, e.g., do you need to add code for catching TaskCanceledException here or let it bubble up?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (await Task.WhenAny(readinessTask, processExitTask) == processExitTask)
{
await readinessCancellation.CancelAsync();
try
{
await readinessTask;
}
catch (OperationCanceledException)
{
}

throw new ClientTransportClosedException(new ClientCompletionDetails
{
Exception = new InvalidOperationException($"HTTP server process exited with code {process.ExitCode} before becoming ready.")
});
}

await readinessTask;

return process;
}
Expand All @@ -259,7 +284,8 @@ public static async Task WaitForServerReadinessAsync(
string serverUrl,
int timeoutSeconds = 30,
int pollIntervalMs = 500,
bool authenticationEnabled = false)
bool authenticationEnabled = false,
CancellationToken cancellationToken = default)
{
using var httpClient = new HttpClient();
var timeout = TimeSpan.FromSeconds(timeoutSeconds);
Expand All @@ -284,7 +310,7 @@ public static async Task WaitForServerReadinessAsync(
};
requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/event-stream"));
using var resp = await httpClient.SendAsync(requestMessage);
using var resp = await httpClient.SendAsync(requestMessage, cancellationToken);

// If authentication is enabled, 401 Unauthorized means server is ready
// If authentication is disabled, we need a success status code
Expand All @@ -298,7 +324,7 @@ public static async Task WaitForServerReadinessAsync(
{
// Server not yet available, continue polling
}
await Task.Delay(pollIntervalMs);
await Task.Delay(pollIntervalMs, cancellationToken);
}

throw new TimeoutException($"Server at {serverUrl} did not become ready within {timeoutSeconds} seconds");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,27 @@ public virtual string RegisterOrRetrieveDeploymentOutputVariable(string name, st
// todo: use this when we have versioned tests to run this against.
protected virtual string? VersionQualifier => null;

/// <summary>
/// In HTTP mode, verifies that a local-only tool is unavailable and indicates that the test should return early.
/// </summary>
/// <param name="toolName">The fully qualified MCP tool name.</param>
/// <returns><see langword="true"/> when running in HTTP mode; otherwise, <see langword="false"/>.</returns>
protected async Task<bool> AssertLocalToolIsUnavailableInHttpMode(string toolName)
{
if (!string.Equals(Environment.GetEnvironmentVariable("MCP_TEST_TRANSPORT"), "http", StringComparison.OrdinalIgnoreCase))
{
return false;
}

var result = await Client.CallToolAsync(
toolName,
new Dictionary<string, object?>(),
cancellationToken: TestContext.Current.CancellationToken);
Assert.True(result.IsError);
Assert.Contains("not found", McpTestUtilities.GetFirstText(result.Content), StringComparison.OrdinalIgnoreCase);
return true;
}

protected override async ValueTask LoadSettingsAsync()
{
await base.LoadSettingsAsync();
Expand Down
7 changes: 4 additions & 3 deletions docs/recorded-tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ The `.proxy` directory is recreated whenever a recorded test run needs the Test
1. **Rebase on latest** – Ensure your branch includes the current recorded-test infrastructure.
2. **Re-parent the test class** – Update live tests to inherit from `RecordedCommandTestsBase` instead of `CommandTestsBase`.
3. **Ensure proxy-aware HTTP usage** – Commands must obtain `HttpClient` instances via `IHttpClientFactory.CreateClient()` to benefit from playback redirection.
4. **Add `assets.json`** – If the toolset doesn’t have one, create `tools/<Tool>/tests/<Tests.CsProj.Folder>/assets.json`:
4. **Handle local-only tools** – For every test of a tool marked `LocalRequired = true`, call `AssertLocalToolIsUnavailableInHttpMode(toolName)` at the start of the test and return early when it returns `true`. The inherited helper verifies that remote HTTP mode does not expose the tool; do not duplicate this transport-specific assertion in individual test classes.
5. **Add `assets.json`** – If the toolset doesn’t have one, create `tools/<Tool>/tests/<Tests.CsProj.Folder>/assets.json`:
```json
{
"AssetsRepo": "Azure/azure-sdk-assets",
Expand All @@ -54,8 +55,8 @@ The `.proxy` directory is recreated whenever a recorded test run needs the Test
}
```
If using `copilot` for initial migration, ensure that it indeed created this file.
5. **Record and push** – Follow the workflow above to generate recordings and push them to the assets repo.
6. **Document sanitizers** – Leave brief comments explaining why custom sanitizers exist to help future maintainers.
6. **Record and push** – Follow the workflow above to generate recordings and push them to the assets repo.
7. **Document sanitizers** – Leave brief comments explaining why custom sanitizers exist to help future maintainers.

Example Migrations:
- [Azure.Mcp.Tools.KeyVault](https://github.com/microsoft/mcp/pull/1080)
Expand Down
2 changes: 1 addition & 1 deletion eng/pipelines/templates/jobs/live-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,14 @@ jobs:
ServiceConnection: azure-sdk-tests-public
PersistOidcToken: true
TestResourcesDirectory: $(Build.SourcesDirectory)/$(TestResourcesPath)
AdditionalParameters: "@{ UseHttpTransport = true }"
SkipEnvironmentSetup: true # environment setup was performed in the first deployment

- task: AzurePowershell@5
displayName: "Run tests (http) - az pwsh"
env:
SYSTEM_ACCESSTOKEN: $(System.AccessToken)
AZURE_MCP_COLLECT_TELEMETRY: 'false'
MCP_TEST_TRANSPORT: 'http'
Comment thread
chidozieononiwu marked this conversation as resolved.
inputs:
azureSubscription: azure-sdk-tests-public
azurePowerShellVersion: 'LatestVersion'
Expand Down
11 changes: 11 additions & 0 deletions servers/Azure.Mcp.Server/docs/new-command.md
Original file line number Diff line number Diff line change
Expand Up @@ -1566,6 +1566,17 @@ Azure service commands requiring test resource deployment must add a bicep templ

All live tests **must** be recorded for playback using `RecordedCommandTestsBase`. See [`/docs/recorded-tests.md`](https://github.com/microsoft/mcp/blob/main/docs/recorded-tests.md) for the full recording workflow, sanitizer configuration, and migration guide.

Tools marked `LocalRequired = true` are not exposed by the remote HTTP server. In every test for such a tool in a class extending `RecordedCommandTestsBase`, call the inherited helper before exercising the tool and return early when it reports HTTP mode:

```csharp
if (await AssertLocalToolIsUnavailableInHttpMode("{toolset}_{resource}_{operation}"))
{
return;
}
```

The helper asserts that the tool is unavailable in HTTP mode. Use it instead of repeating environment detection and unavailable-tool assertions in each toolset.

#### Live Test Resource Infrastructure

**1. Create Toolset Bicep Template (`/tools/Azure.Mcp.Tools.{Toolset}/tests/test-resources.bicep`)**
Expand Down
Loading
Loading