Replace dead #if _WINDOWS test guards with runtime OS checks#4423
Replace dead #if _WINDOWS test guards with runtime OS checks#4423paulmedynski wants to merge 3 commits into
Conversation
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
There was a problem hiding this comment.
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 NETinstead of_WINDOWS. - Ensured reflection-based capture/restore of
s_useManagedNetworkingonly 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
UseManagedNetworkingis now compiled for all .NET test runs, but its setter always reflects the private fields_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 clearerPlatformNotSupportedException(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 Report✅ All modified and coverable lines are covered by tests. 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
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:
|
…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.
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).
Summary
The test projects (
UnitTests,TestCommon) never define_WINDOWS/_UNIX— those constants are only set by the mainsrccsproj based onTargetOs. This made all#if NET && _WINDOWSblocks aroundUseManagedNetworkingin the test code permanently dead, so the switch's default was never actually exercised.This PR replaces the compile-time
_WINDOWSguards with runtimeRuntimeInformation.IsOSPlatform(OSPlatform.Windows)checks, kept under#if NET. Thes_useManagedNetworkingfield /UseManagedNetworkingproperty 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_WINDOWSconstant, and#if NETstill excludes .NET Framework where the field never exists.Changes
tests/Common/LocalAppContextSwitchesHelper.csusing System.Runtime.InteropServices;._useManagedNetworkingOriginalfield andUseManagedNetworkingproperty guards from#if NET && _WINDOWSto#if NET.Dispose) ofs_useManagedNetworkinginif (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)).tests/UnitTests/.../LocalAppContextSwitchesTest.csusing System.Runtime.InteropServices;.#if NET && _WINDOWSblocks (the reset and theAssert.False(...UseManagedNetworking)) with runtime Windows checks inside the existing#if NET.Effect
UseManagedNetworkingdefault (false) is now actually tested — previously the assertion was dead code.Testing
UseManagedNetworkingcoverage)Verified on Linux / net9.0:
UnitTestsbuild: succeeded, 0 warnings, 0 errors.LocalAppContextSwitchesTest: passed (Windows branch correctly skipped at runtime).Work item: AB#45771