Skip to content

[7.0] Address flaky DEBUG assertions#4143

Open
paulmedynski wants to merge 1 commit intorelease/7.0from
dev/paul/release/7.0/flaky-debug
Open

[7.0] Address flaky DEBUG assertions#4143
paulmedynski wants to merge 1 commit intorelease/7.0from
dev/paul/release/7.0/flaky-debug

Conversation

@paulmedynski
Copy link
Copy Markdown
Contributor

Description

Port of #4085 to release/7.0.

Refactors the DataTestUtility.AssertThrowsWrapper assertion helpers and fixes race condition test failures across the manual test suite.

Problem

  1. Flaky assertion regression: A DEBUG assertion started failing on Ubuntu .NET 8 in PR pipelines, causing test failures in DataStreamTest (see SqlDataReader async read throws inconsistent exception types on disposal (IOException vs InvalidOperationException) #4088, Fix commented-out Debug.Assert()s that are failing #3604).
  2. Race conditions in async disposal: When PendAsyncReadsScope is disposed while an async ReadAsync is in-flight, the inner exception type varies depending on timing — it may be IOException, InvalidOperationException, ObjectDisposedException, or TaskCanceledException. The existing assertion helpers only accepted a single expected type and would fail intermittently.
  3. Misleading assertion helpers: AssertThrowsWrapper accepted generic type parameters for inner/inner-inner exceptions but never validated them with actual type checks. It also had unused parameters (innerExceptionMustBeNull, customExceptionVerifier, innerInnerInnerExceptionMustBeNull) that no callers were passing.
  4. Duplicated exception-handling code: Six identical try { t.Wait(); } catch (AggregateException) { ... } blocks were scattered throughout DataStreamTest.cs.

Changes

DataTestUtility.cs — Assertion helper redesign

Replaced the three AssertThrowsWrapper overloads with four properly-typed, well-documented helpers:

Old Method New Method What Changed
AssertThrowsWrapper<T>(...) AssertThrows<T>(...) Removed unused params (innerExceptionMustBeNull, customExceptionVerifier)
AssertThrowsWrapper<T, TInner>(...) AssertThrowsInner<T, TInner>(...) Now actually validates the inner exception type via Assert.IsAssignableFrom<TInnerException>
AssertThrowsWrapper<T, TInner, TInnerInner>(...) AssertThrowsInnerWithAlternate<T, TInner, TAlt>(...) Repurposed: accepts two possible inner exception types for race conditions instead of checking inner-inner exceptions

All helpers are wrapped in a #nullable enable block with XML documentation.

DataStreamTest.cs — Race condition fixes

  • Extracted WaitIgnoringFlakyException(Task): Replaces 6 duplicated try/catch blocks. Calls task.Wait() and ignores AggregateException only for faulted tasks (with a comment explaining that a faulted Task permanently stores its exception, so re-waiting always rethrows).
  • Fixed race-prone assertion sites using AssertThrowsInnerWithAlternate:
    • Lines ~1407, ~1797, ~1813: IOExceptionInvalidOperationException depending on reader disposal timing
    • Line ~1828: TaskCanceledExceptionInvalidOperationException depending on PendAsyncReadsScope disposal timing
    • Lines ~1841, ~1969: Inline assertions handling IOExceptionSqlException or ObjectDisposedException alternate path
  • Removed all TODO(GH-3604) comments — replaced with proper alternate-type assertions that handle the race correctly.
  • Added explanatory comments at each race-prone site referencing SqlDataReader async read throws inconsistent exception types on disposal (IOException vs InvalidOperationException) #4088.

22 other test files — Assertion renames

All call sites across the manual test suite updated from AssertThrowsWrapper to the new names:

  • AssertThrowsWrapper<T>(...)AssertThrows<T>(...)
  • AssertThrowsWrapper<T, TInner>(...)AssertThrowsInner<T, TInner>(...)

Files: AdapterTest, LocalDBTest, MARSTest, ParallelTransactionsTest, ParametersTest, 10 SqlBulkCopy tests, SqlCommandCancelTest, SqlNotificationTest, TransactionTest, UdtTest2, CopyAllFromReaderConnectionCloseOnEventAsync.

Minor cleanups

  • Fixed #pragma warning indentation in DataTestUtility.cs
  • Changed #nullable disable#nullable restore for proper nullable context scoping

Testing

  • Verified all assertion call sites compile and pass locally
  • CI pipeline validates across all configurations (Windows/Linux/macOS, net462/net8.0/net9.0, ManagedSNI/NativeSNI, SQL Server 2019/2022/Azure SQL/Named Instance/ARM64)
  • Confirmed the only remaining CI failure (ReadStream_ReadsStreamDataCorrectly line 1828) is fixed by the AssertThrowsInnerWithAlternate change

@paulmedynski paulmedynski requested a review from a team as a code owner April 6, 2026 15:47
Copilot AI review requested due to automatic review settings April 6, 2026 15:47
@github-project-automation github-project-automation bot moved this to To triage in SqlClient Board Apr 6, 2026
@paulmedynski paulmedynski added this to the 7.0.1 milestone Apr 6, 2026
@paulmedynski paulmedynski moved this from To triage to In review in SqlClient Board Apr 6, 2026
@paulmedynski paulmedynski added the Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems. label Apr 6, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Ports PR #4085 to the release/7.0 branch by refactoring the ManualTests assertion helpers to correctly validate inner exception types and updating race-prone async-disposal tests to accept known alternate exception types, reducing DEBUG-mode flakiness on Linux/.NET.

Changes:

  • Replaced DataTestUtility.AssertThrowsWrapper* overloads with new AssertThrows* helpers that properly validate inner exception types and support “alternate inner exception” races.
  • Refactored DataStreamTest to remove duplicated Task.Wait()/AggregateException handling and updated race-sensitive assertions to tolerate expected alternate exception types.
  • Renamed assertion helper call sites across the manual test suite to the new APIs (plus minor whitespace/format cleanups).

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs Introduces new AssertThrows* helpers (with proper inner-type validation) and updates nullable scoping.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs Consolidates flaky Task.Wait() handling and updates assertions for known race-condition exception variability.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionTest.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs Renames assertion helper usages to AssertThrows; minor comment whitespace cleanup.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TransactionTestAsync.cs Switches to AssertThrowsInner for validating AggregateException inner type.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Transaction4.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Transaction3.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Transaction1.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Transaction.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/OrderHintMissingTargetColumn.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/OrderHintDuplicateColumn.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/MissingTargetColumns.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/MissingTargetColumn.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyWithEvent1.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyAllFromReaderConnectionCloseOnEventAsync.cs Adds System.IO and uses AssertThrowsInnerWithAlternate to handle race-dependent inner exception types.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyAllFromReaderConnectionCloseAsync.cs Switches to AssertThrowsInner for validating AggregateException inner type.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopyAllFromReaderCancelAsync.cs Switches to AssertThrowsInner for validating AggregateException inner type.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/ParametersTest.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParallelTransactionsTest/ParallelTransactionsTest.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/MARSTest/MARSTest.cs Renames assertion helper usages to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs Renames assertion helper usage to AssertThrows.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AdapterTest/AdapterTest.cs Renames assertion helper usages to AssertThrows.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.58%. Comparing base (864f666) to head (b793595).
⚠️ Report is 2 commits behind head on release/7.0.

❗ There is a different number of reports uploaded between BASE (864f666) and HEAD (b793595). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (864f666) HEAD (b793595)
CI-SqlClient 1 0
Additional details and impacted files
@@               Coverage Diff               @@
##           release/7.0    #4143      +/-   ##
===============================================
- Coverage        73.07%   65.58%   -7.50%     
===============================================
  Files              280      275       -5     
  Lines            42997    65822   +22825     
===============================================
+ Hits             31422    43171   +11749     
- Misses           11575    22651   +11076     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 65.58% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems.

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants