.NET: Fix copilot studio integration tests failure#4209
Conversation
d04228d
into
microsoft:feature-xunit3-mtp-upgrade
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix CopilotStudio integration test failures by wrapping the configuration loading in a try-catch block and calling Assert.Skip() when the configuration cannot be loaded. The intent is to gracefully skip tests when CopilotStudio configuration is not available rather than failing with an exception.
Changes:
- Added try-catch error handling around
TestConfiguration.LoadSection<CopilotStudioAgentConfiguration>()call - Calls
Assert.Skip()with an error message if configuration loading fails - Uses null-forgiving operator on config variable after the try-catch block
Comments suppressed due to low confidence (1)
dotnet/tests/CopilotStudio.IntegrationTests/CopilotStudioFixture.cs:41
- The error message concatenation lacks a space between "Error:" and the exception message. Consider using string interpolation for better readability:
Assert.Skip($"CopilotStudio configuration could not be loaded. Error: {ex.Message}");
Assert.Skip("CopilotStudio configuration could not be loaded. Error:" + ex.Message);
| catch (InvalidOperationException ex) | ||
| { | ||
| Assert.Skip("CopilotStudio configuration could not be loaded. Error:" + ex.Message); | ||
| } |
There was a problem hiding this comment.
Calling Assert.Skip() inside InitializeAsync() won't properly skip tests. When InitializeAsync() is called during fixture initialization (via AgentTests<TAgentFixture>.InitializeAsync()), the skip is triggered before any actual test method runs, which may not be handled correctly by the test framework.
A better approach would be to follow the pattern used in CosmosChatHistoryProviderTests:
- Store the configuration loading result in an instance field (e.g.,
_configLoadFailedor_skipReason) - Don't throw or call Assert.Skip during initialization
- Have each test method check this field at the start and skip if needed, OR
- Store the skip reason in a public static field that tests can check (similar to
AnthropicChatCompletionFixture.SkipReason)
The Cosmos pattern is particularly relevant here since it also handles external dependencies that may not be available. See CosmosChatHistoryProviderTests.SkipIfEmulatorNotAvailable() at line 142-149.
| Assert.Skip("CopilotStudio configuration could not be loaded. Error:" + ex.Message); | ||
| } | ||
|
|
||
| var settings = new CopilotStudioConnectionSettings(config!.TenantId, config.AppClientId) |
There was a problem hiding this comment.
The null-forgiving operator (!) is used after checking for null and potentially calling Assert.Skip. However, since the catch block calls Assert.Skip (which should terminate execution), the config variable will still be null if the exception occurs. If Assert.Skip doesn't properly terminate execution in this context (which is the case when called from InitializeAsync), this will result in a NullReferenceException.
The config variable should not need the null-forgiving operator if the exception handling is properly restructured. After fixing the Assert.Skip issue, consider either: (1) not initializing the agent when config loading fails, or (2) storing the skip state and allowing tests to check it later.
Motivation and Context
Description
Contribution Checklist