Flaky tests / lighter test queries#4344
Open
mdaigle wants to merge 6 commits into
Open
Conversation
mdaigle
added
Hotfix 7.0.3
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the SqlClient test suite by quarantining a small set of intermittently failing unit tests, replacing a manual transaction-pool cleanup scenario with a deterministic unit test, and reducing CPU/scan overhead in high-volume MARS manual tests (notably for Azure SQL environments).
Changes:
- Quarantines a few simulated-server transient fault/failover tests as
Category=flakywith inline failure notes where helpful. - Adds a deterministic unit test validating that pool pruning does not impact transacted connections, and exposes the pruning callback internally to support that test.
- Optimizes MARS manual test queries to avoid expensive system scanning (
sys.databases,sp_who) while still producing sufficient observable request activity.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs | Marks select transient-fault test cases as flaky to prevent intermittent CI failures. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs | Quarantines an intermittently failing failover-related async test with a documented failure symptom. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs | Adds a pruning-focused unit test asserting transacted connections are not pruned/destroyed. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/MARSSessionPoolingTest/MarsSessionPoolingTest.cs | Replaces high-cost SQL text/SP usage with lighter alternatives to reduce CPU/scan impact while preserving test intent. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs | Removes the older manual transaction-cleanup test (scenario now covered deterministically via unit test). |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs | Makes the pruning callback internal (with documentation) so unit tests can invoke pruning deterministically. |
paulmedynski
added
the
Area\Tests
paulmedynski
previously approved these changes
Jun 6, 2026
apoorvdeshmukh
previously approved these changes
Jun 8, 2026
cheenamalhotra
added
the
Author attention needed
|
@mdaigle This pull request has been marked as Author attention needed. When you have addressed the reviewer feedback and are ready for another review, please post a comment with |
Member
|
@mdaigle I see test failures that needs attention here. |
The VALUES-based query and sp_server_info produced result sets too small for MARS flow control — the server completed requests immediately, so dm_exec_requests showed no active requests. This broke ExecuteReader_DisposeCommand which asserts open request counts. Revert to sys.databases and sp_who which produce enough data to keep server-side requests active while readers remain open. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| /// Exposed as <c>internal</c> (rather than <c>private</c>) solely so unit tests can | ||
| /// invoke a deterministic prune cycle without waiting on the cleanup timer. | ||
| /// </remarks> | ||
| internal void CleanupCallback(object state) |
paulmedynski
previously approved these changes
Jul 17, 2026
# Conflicts: # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs
Comment on lines
78
to
83
| [Theory] | ||
| [InlineData(40613)] | ||
| [InlineData(42108)] | ||
| [InlineData(42109)] | ||
| [Trait("Category", "flaky")] | ||
| public async Task TransientFault_RetryEnabled_ShouldSucceed_Async(uint errorCode) |
paulmedynski
approved these changes
Jul 17, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4344 +/- ##
==========================================
- Coverage 65.83% 65.17% -0.66%
==========================================
Files 287 284 -3
Lines 43763 66782 +23019
==========================================
+ Hits 28812 43528 +14716
- Misses 14951 23254 +8303
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Marks a few flaky tests. Rewrites a manual test as a unit test. Reduces CPU/scan impact on a few high volume tests queries.