Improve TestMode deserialization error message#1626
Conversation
Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com>
scbedd
requested review from
KarishmaGhiya,
alzimmermsft,
Copilot,
g2vinay,
joshfree,
msalaman,
sandeep-sen and
weshaggard
There was a problem hiding this comment.
Pull request overview
This PR improves the error message when .testsettings.json contains an invalid TestMode value by catching JsonException during deserialization and throwing an InvalidOperationException with a more helpful message that lists the valid enum values (Live, Record, Playback).
Changes:
- Modified
LiveTestSettings.TryLoadTestSettingsto catchJsonExceptionwhen the message contains "TestMode" and throw anInvalidOperationExceptionwith valid enum values enumerated - Added unit tests for JSON deserialization behavior with valid and invalid TestMode values (though tests use a proxy class rather than testing the actual method)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| core/Azure.Mcp.Core/tests/Azure.Mcp.Tests/Client/Helpers/LiveTestSettings.cs | Added try-catch block to intercept TestMode deserialization errors and provide enhanced error message |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/LiveTestSettingsTests.cs | Added unit tests to verify JSON deserialization behavior with valid and invalid TestMode values |
| return true; | ||
| } | ||
| } | ||
| catch (JsonException ex) when (ex.Message.Contains("TestMode")) |
There was a problem hiding this comment.
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)
| catch (JsonException ex) when (ex.Message.Contains("TestMode")) | |
| catch (JsonException ex) when (ex.Path?.Contains(nameof(TestMode)) == true) |
| [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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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")) | ||
| { | ||
| 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); | ||
| } |
There was a problem hiding this comment.
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.
| throw new InvalidOperationException( | ||
| $"Invalid TestMode value in {TestSettingsFileName}. Valid values are: {validValues}. " + | ||
| $"Error details: {ex.Message}", ex); |
There was a problem hiding this comment.
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.
| 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}"); |
When
.testsettings.jsoncontains an invalidTestModevalue, deserialization fails with a generic JSON error that doesn't indicate which values are valid.Changes
LiveTestSettings.TryLoadTestSettings: CatchesJsonExceptionfor TestMode deserialization failures and throwsInvalidOperationExceptionwith enumerated valid valuesBefore/After
Before:
After:
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
learn.microsoft.com/home/REDACTED/work/mcp/mcp/core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/bin/Debug/net10.0/Azure.Mcp.Core.UnitTests /home/REDACTED/work/mcp/mcp/core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/bin/Debug/net10.0/Azure.Mcp.Core.UnitTests @@ /tmp/tmpsQl0f4.tmp(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
Azure.Mcp.Testsframework produces bad error for bad TestMode setting #1379✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.