Skip to content

Improve TestMode deserialization error message #1626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
+83 −7
Open

Improve TestMode deserialization error message #1626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
752d5d3
Initial plan
Copilot Feb 2, 2026
44b4fa3
empty-commit
scbedd Feb 2, 2026
c9e29a0
Add improved error message for invalid TestMode values
Copilot Feb 3, 2026
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
66 changes: 66 additions & 0 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/LiveTestSettingsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Text.Json;
using System.Text.Json.Serialization;
using Azure.Mcp.Tests.Helpers;
using Xunit;

namespace Azure.Mcp.Core.UnitTests.Helpers;

public class LiveTestSettingsTests
{
[Fact]
public void JsonDeserialization_WithInvalidTestMode_ThrowsJsonException()
{
// Arrange - Create JSON with an invalid TestMode value
var json = """{"TestMode": "InvalidMode", "TenantId": "test-tenant"}""";

// Act & Assert - Verify JsonException is thrown
var exception = Assert.Throws<JsonException>(() =>
JsonSerializer.Deserialize<LiveTestSettingsProxy>(json, new JsonSerializerOptions()
{
PropertyNameCaseInsensitive = true,
Converters = { new JsonStringEnumConverter() }
}));

// Verify the error message mentions TestMode
Assert.Contains("TestMode", exception.Message);
}

[Theory]
[InlineData("Live", TestMode.Live)]
[InlineData("Record", TestMode.Record)]
[InlineData("Playback", TestMode.Playback)]
[InlineData("live", TestMode.Live)] // Test case-insensitive
[InlineData("record", TestMode.Record)]
[InlineData("playback", TestMode.Playback)]
public void JsonDeserialization_WithValidTestMode_LoadsSuccessfully(string testModeValue, TestMode expectedMode)
{
// Arrange - Create JSON with valid TestMode value
var json = $$$"""{"TestMode": "{{{testModeValue}}}", "TenantId": "test-tenant", "SubscriptionId": "test-subscription"}""";

// Act - Deserialize the JSON
var settings = JsonSerializer.Deserialize<LiveTestSettingsProxy>(json, new JsonSerializerOptions()
{
PropertyNameCaseInsensitive = true,
Converters = { new JsonStringEnumConverter() }
});

// Assert - Verify the settings were loaded correctly
Assert.NotNull(settings);
Assert.Equal(expectedMode, settings.TestMode);
Assert.Equal("test-tenant", settings.TenantId);
Assert.Equal("test-subscription", settings.SubscriptionId);
}

/// <summary>
/// Proxy class for testing LiveTestSettings JSON deserialization without file I/O
/// </summary>
private class LiveTestSettingsProxy
{
public TestMode TestMode { get; set; } = TestMode.Live;
public string TenantId { get; set; } = string.Empty;
public string SubscriptionId { get; set; } = string.Empty;
}
}
Comment on lines +13 to +66
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The tests verify JSON deserialization behavior but don't actually test the TryLoadTestSettings method where the improved error message is thrown. The current tests use a LiveTestSettingsProxy class and test JsonSerializer.Deserialize directly, so they won't validate that the InvalidOperationException with the enhanced error message is actually thrown when loading an invalid .testsettings.json file. Consider adding an integration test that creates a temporary .testsettings.json file with an invalid TestMode value and verifies that TryLoadTestSettings throws the expected InvalidOperationException with the helpful error message listing valid values.

Copilot uses AI. Check for mistakes.
24 changes: 17 additions & 7 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Tests/Client/Helpers/LiveTestSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,26 @@ public static bool TryLoadTestSettings([NotNullWhen(true)] out LiveTestSettings?
{
var json = File.ReadAllText(path);

settings = JsonSerializer.Deserialize<LiveTestSettings>(json, new JsonSerializerOptions()
try
{
PropertyNameCaseInsensitive = true,
Converters = { new JsonStringEnumConverter() }
});
settings = JsonSerializer.Deserialize<LiveTestSettings>(json, new JsonSerializerOptions()
{
PropertyNameCaseInsensitive = true,
Converters = { new JsonStringEnumConverter() }
});

if (settings != null)
if (settings != null)
{
settings.SettingsDirectory = Path.GetDirectoryName(path) ?? string.Empty;
return true;
}
}
catch (JsonException ex) when (ex.Message.Contains("TestMode"))
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The catch filter condition ex.Message.Contains("TestMode") is fragile and may not reliably identify TestMode deserialization errors. The JsonException message format could vary across .NET versions or be localized. Consider catching JsonException without a when clause and examining the exception's Path property to determine if it relates to TestMode. For example: catch (JsonException ex) when (ex.Path?.Contains("TestMode") == true)

Suggested change
catch (JsonException ex) when (ex.Message.Contains("TestMode"))
catch (JsonException ex) when (ex.Path?.Contains(nameof(TestMode)) == true)

Copilot uses AI. Check for mistakes.
{
settings.SettingsDirectory = Path.GetDirectoryName(path) ?? string.Empty;
return true;
var validValues = string.Join(", ", Enum.GetNames<TestMode>());
throw new InvalidOperationException(
$"Invalid TestMode value in {TestSettingsFileName}. Valid values are: {validValues}. " +
$"Error details: {ex.Message}", ex);
Comment on lines +73 to +75
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

Throwing an exception from a Try* method violates the standard Try-Parse pattern in .NET, where Try* methods should return false for errors rather than throwing exceptions. Existing callers of TryLoadTestSettings (e.g., ServerCommandTests.cs line 37) use it in conditional statements and don't expect exceptions. This breaking change could cause unexpected crashes. Consider either: (1) renaming the method to LoadTestSettings since it now throws exceptions, or (2) returning false and logging the detailed error message instead of throwing, to maintain the Try* pattern contract.

Suggested change
throw new InvalidOperationException(
$"Invalid TestMode value in {TestSettingsFileName}. Valid values are: {validValues}. " +
$"Error details: {ex.Message}", ex);
System.Console.Error.WriteLine(
$"Invalid TestMode value in {TestSettingsFileName}. Valid values are: {validValues}. " +
$"Error details: {ex.Message}");

Copilot uses AI. Check for mistakes.
}
Comment on lines +56 to 76
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The exception handling only catches and rethrows JsonException when the message contains "TestMode", but other JSON deserialization errors (e.g., invalid JSON syntax, type mismatches on other properties) will propagate as JsonException without the enhanced error handling. This could lead to inconsistent error messages. If you only want to enhance the error message for TestMode specifically, the current implementation is acceptable but document this limitation. Alternatively, consider whether other deserialization errors also deserve better error messages.

Copilot uses AI. Check for mistakes.
}

Expand Down
Loading