Skip to content

Replace dead #if _WINDOWS test guards with runtime OS checks#4423

Open
paulmedynski wants to merge 3 commits into
mainfrom
dev/paul/test-dead-guards
Open

Replace dead #if _WINDOWS test guards with runtime OS checks#4423
paulmedynski wants to merge 3 commits into
mainfrom
dev/paul/test-dead-guards

Conversation

@paulmedynski

@paulmedynski paulmedynski commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The test projects (UnitTests, TestCommon) never define _WINDOWS/_UNIX — those constants are only set by the main src csproj based on TargetOs. This made all #if NET && _WINDOWS blocks around UseManagedNetworking in the test code permanently dead, so the switch's default was never actually exercised.

This PR replaces the compile-time _WINDOWS guards with runtime RuntimeInformation.IsOSPlatform(OSPlatform.Windows) checks, kept under #if NET. The s_useManagedNetworking field / UseManagedNetworking property only exist in the SqlClient assembly when it is built for .NET on Windows (#if NET && _WINDOWS); on Unix .NET it is a constant => true (no field) and on .NET Framework => false (no field). Since the repo builds per-OS (build OS == run OS), the runtime Windows check is an accurate proxy for the old _WINDOWS constant, and #if NET still excludes .NET Framework where the field never exists.

Changes

  • tests/Common/LocalAppContextSwitchesHelper.cs
    • Added using System.Runtime.InteropServices;.
    • Changed the _useManagedNetworkingOriginal field and UseManagedNetworking property guards from #if NET && _WINDOWS to #if NET.
    • Wrapped capture (constructor) and restore (Dispose) of s_useManagedNetworking in if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)).
  • tests/UnitTests/.../LocalAppContextSwitchesTest.cs
    • Added using System.Runtime.InteropServices;.
    • Replaced both #if NET && _WINDOWS blocks (the reset and the Assert.False(...UseManagedNetworking)) with runtime Windows checks inside the existing #if NET.

Effect

  • On Windows / .NET: the UseManagedNetworking default (false) is now actually tested — previously the assertion was dead code.
  • On Unix / .NET and .NET Framework: behavior unchanged (branch skipped / excluded), and the nonexistent field is never touched via reflection.

Testing

  • Tests added or updated (re-enabled the previously-dead UseManagedNetworking coverage)
  • Public API changes documented (n/a)
  • No breaking changes introduced

Verified on Linux / net9.0:

  • UnitTests build: succeeded, 0 warnings, 0 errors.
  • LocalAppContextSwitchesTest: passed (Windows branch correctly skipped at runtime).

Note: the net462 / Windows path could not be executed from the Linux dev host, but that path is unchanged in behavior (still excluded via #if NET).

Work item: AB#45771

The test projects (UnitTests, TestCommon) never define _WINDOWS/_UNIX,
so the #if NET && _WINDOWS blocks around UseManagedNetworking were
permanently dead and the switch was never exercised.

Replace the compile-time _WINDOWS guards with runtime
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) checks (kept under
#if NET, since the s_useManagedNetworking field only exists in the
.NET-on-Windows SqlClient build). On Windows the UseManagedNetworking
default is now actually tested.

Work item: AB#45771
Copilot AI review requested due to automatic review settings July 3, 2026 17:39
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Jul 3, 2026
@paulmedynski paulmedynski moved this from To triage to In progress in SqlClient Board Jul 3, 2026
@paulmedynski paulmedynski added the Area\Tests Issues that are targeted to tests or test projects label Jul 3, 2026
@paulmedynski paulmedynski added this to the 7.1.0-preview3 milestone Jul 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes dead test code paths by replacing compile-time #if NET && _WINDOWS guards (which aren’t defined in the test projects) with runtime OS checks, so the UseManagedNetworking AppContext switch behavior is actually exercised where it exists.

Changes:

  • Updated test and helper code to use RuntimeInformation.IsOSPlatform(OSPlatform.Windows) under #if NET instead of _WINDOWS.
  • Ensured reflection-based capture/restore of s_useManagedNetworking only happens at runtime on Windows.
  • Re-enabled effective coverage of the Windows/.NET default for UseManagedNetworking (previously skipped at compile time).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs Switch capture/restore now uses runtime Windows checks under #if NET to avoid reflecting missing fields on non-Windows.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs Replaced dead _WINDOWS guarded assertions/reset with runtime Windows checks inside the existing #if NET.
Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs:386

  • UseManagedNetworking is now compiled for all .NET test runs, but its setter always reflects the private field s_useManagedNetworking, which only exists in the product assembly when built for .NET on Windows. If a test accidentally sets this property on non-Windows, it will fail with a generic "Field not found" exception. Consider guarding the setter with a Windows runtime check and throwing a clearer PlatformNotSupportedException (and update the remarks accordingly).
    /// <remarks>
    /// The underlying s_useManagedNetworking field only exists in the SqlClient
    /// assembly when it is built for .NET on Windows. Callers must only use this
    /// property when running on Windows (see
    /// RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); otherwise the
    /// reflection lookup of the field will fail.
    /// </remarks>
    public bool? UseManagedNetworking
    {
        get => GetSwitchPropertyValue(nameof(UseManagedNetworking));
        set => SetSwitchValue("s_useManagedNetworking", value);
    }

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.94%. Comparing base (c30bbbc) to head (cb1f654).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4423      +/-   ##
==========================================
- Coverage   65.78%   63.94%   -1.85%     
==========================================
  Files         286      282       -4     
  Lines       43696    66658   +22962     
==========================================
+ Hits        28745    42622   +13877     
- Misses      14951    24036    +9085     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 63.94% <ø> (?)

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

☔ View full report in Codecov by Harness.
📢 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.

…itchesTest (PR #4423)

Address review feedback on PR #4423. The runtime Windows guard previously
skipped validating UseManagedNetworking on non-Windows platforms. Since
LocalAppContextSwitches.UseManagedNetworking is a constant true on .NET Unix,
add an else branch asserting it is true on Linux/macOS while keeping the
false-default assertion on Windows. The reset block stays Windows-only because
the underlying s_useManagedNetworking field only exists in the Windows build.
@paulmedynski paulmedynski marked this pull request as ready for review July 4, 2026 13:41
@paulmedynski paulmedynski requested a review from a team as a code owner July 4, 2026 13:41
Copilot AI review requested due to automatic review settings July 4, 2026 13:41
@paulmedynski paulmedynski moved this from In progress to In review in SqlClient Board Jul 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Address a second review round on PR #4423:
- Wrap 'using System.Runtime.InteropServices;' in #if NET in both
  LocalAppContextSwitchesTest.cs and LocalAppContextSwitchesHelper.cs, since it
  is only used by #if NET blocks and would raise CS8019 (warnings-as-errors) on
  the net462 Windows build.
- Correct the UseManagedNetworking <remarks> in the helper to note that only the
  setter requires Windows (the getter reads the cross-platform property).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Tests Issues that are targeted to tests or test projects

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants