Merged
Conversation
…fail fast on any unexpected exception
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves test code maintainability and diagnostics by extracting the XEventScope class from DataTestUtility into its own file, rewriting test name extraction logic to be more robust, introducing helper extensions for SqlDataReader, and refactoring the AsyncCancelledConnectionsTest to use modern async/await patterns with better exception handling.
Key Changes:
- Extracted
XEventScopefromDataTestUtilityinto a standalone class with improved documentation and error handling (throws instead of asserts) - Rewrote
DataTestUtility.CurrentTestName()to traverse the xUnit object tree directly rather than parsingDisplayName, making it resilient to IDE test runner configuration differences - Converted
AsyncCancelledConnectionsTestfrom aFactwith internal loops to aTheorywithInlineData, implementing fail-fast exception handling instead of collecting exceptions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
SqlDataReaderExtensions.cs |
New helper class with FlushAllResults and FlushResultSet extension methods for discarding data reader results |
XEventScope.cs |
Newly extracted class for managing XEvent sessions with improved XML docs, exception-based validation, and formatted SQL strings |
DataTestUtility.cs |
Removed XEventScope nested class and rewrote CurrentTestName to use object tree traversal instead of regex parsing |
AsyncCancelledConnectionsTest.cs |
Converted from Fact to Theory, refactored to use async/await properly, and implemented fail-fast exception handling with IsExpectedCancellation helper |
XEventsTracingTest.cs |
Updated to use standalone XEventScope class |
DataStreamTest.cs |
Updated to use standalone XEventScope class with target initializer syntax |
Microsoft.Data.SqlClient.ManualTesting.Tests.csproj |
Added reference to new XEventScope.cs file |
Contributor
paulmedynski
left a comment
There was a problem hiding this comment.
One question in the AsyncCancelledConnectionsTest.
…nc cancellation tests
mdaigle
approved these changes
Dec 15, 2025
mdaigle
requested changes
Dec 15, 2025
Contributor
mdaigle
left a comment
There was a problem hiding this comment.
Accidentally clicked approve before
mdaigle
approved these changes
Dec 16, 2025
paulmedynski
approved these changes
Dec 22, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
As part of debugging some failing tests I've been working on, I tweaked some test code so I could more easily diagnose the issue. These changes aren't really worth mucking up the PRs I used them on, but I feel they are valuable enough to submit anyhow. Consider it a byproduct of other, more important work.
Here's a rundown of the changes:
SqlDataReaderExtensionclass that has some test-only extensions for commonly used behaviors that are just kinda ugly :)FlushAllResults- Reads and discards all results/sets from the provided data readerFlushResultSet- Reads and discards all results in the current set of the provided data readerDataTestUtilityfrom @paulmedynskiDisplayNameproperty of the currently runningITest- unfortunately this property can differently structured depending on the value ofmethodDisplay, which may be overridden by IDE test runners (ie, my IDE's test runner).AsyncCancelledConnectionTestfrom Fact that has two separate test runs inside it to aTheorywith inline data for enabling/disabling MARS.AsyncCancelledConnectionTestto no longer capture all exceptions and check for an empty collection of exceptions at the end, but instead let the exceptions bubble to the top to cause the test to fail faster and better expose the exception in test results. (this was implemented by codex 🤖)AsyncCancelledConnectionTestsuch that all the tracking/status displaying code is removed. The logging was pretty meaningless and added a lot of complexity to the code.Issues
N/A
Testing
Tests were ran locally and passed with their new changes. CI will validate.