You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Replaces reflection with InternalsVisibleTo in LocalAppContextSwitchesHelper, and splits the internals-bound test helpers into their own assembly so that change does not lock every test project to the same-commit driver.
Targets main directly. This supersedes #4759, which was stacked on the now-abandoned #4763.
Commit 1 — Use InternalsVisibleTo instead of reflection
The helper reached LocalAppContextSwitches' cached switch fields by reflection, so switch names were stringly-typed and a rename in the driver would only fail at run time.
SwitchValue and the s_* cache fields are now internal, with an IVT grant to the helper's assembly. The helper reads and writes them directly, so a rename is now a build break. All get/set logic lives in the helper; the driver keeps only the field declarations (net -112 lines there).
Commit 2 — Split internals-bound helpers out of TestCommon
Binding to internals locks an assembly to the driver at the same repo commit. TestCommon is shared by every test project, so that lock propagated to consumers that only touch public API.
PerformanceTests is the one that breaks. Its baseline pass pins a released driver via MdsPackageVersion, which TestCommon must honour or restore fails:
So the two internals-bound helpers — LocalAppContextSwitchesHelper and ConnectionPoolVersionScope — move to a new Microsoft.Data.SqlClient.TestCommon.Internals project that carries the IVT grant and the implementation-assembly binding, and is never pinned. TestCommon keeps the other 20 files, touches public API only, and honours MdsPackageVersion again.
The split is by coupling, not by dependency. Most of what stays behind (the DatabaseObjects fixtures, SqlDataReaderExtensions) does use SqlConnection/SqlCommand — but only their public surface, so it compiles against any supported driver version. A "no dependencies vs. has dependencies" split would have left those fixtures on the locked side and fixed nothing.
Both helpers keep the Microsoft.Data.SqlClient.Tests.Common namespace, so consumers need no source changes — only a project reference. UnitTests, FunctionalTests and ManualTests get one; PerformanceTests deliberately does not, since it uses neither helper.
Validation
All on net9.0:
Scenario
Result
UnitTests — Project mode
1313 passed, 1 pre-existing failure
UnitTests — Package mode
1313 passed, 1 pre-existing failure
PerformanceTests — Package mode pinned to 7.1.0
builds; resolves Microsoft.Data.SqlClient/7.1.0
FunctionalTests, ManualTests — Project mode
0 errors
The single failure is SimulatedServerTests.ConnectionTests.IntegratedAuthConnectionTest, which fails identically on main (no Kerberos/SSPI on the Linux host) — verified, not a regression.
Package mode is the meaningful run: exercised against a locally packed driver, it confirms the internal field access resolves at run time through the packaged assembly (no FieldAccessException). The packed DLL was confirmed to carry InternalsVisibleTo + Microsoft.Data.SqlClient.TestCommon.Internals.
The perf baseline build was confirmed to resolve Microsoft.Data.SqlClient/7.1.0 with Microsoft.Data.SqlClient.TestCommon.Internalsabsent from its output — i.e. the lock no longer reaches PerformanceTests.
Reviewer notes
runtimes/win/... HintPath. Verified safe today — the win and unix payloads are byte-identical and the driver has zero OS-conditional compilation. Carries the same @TODO as UnitTests.csproj.
Signed IVT is dormant.TestSigningKeyPath is set by no pipeline, and the only pipeline setting SigningKeyPath runs no tests, so tests always match the unsigned grant. The PublicKey=-qualified grant uses the same blob as UnitTests (same key). I could not verify the blob against the real key — it is an ADO secure file.
Signed + Project mode cannot compile TestCommon.Internals, since that config intentionally grants no IVT. UnitTests already had this constraint; the new project joins it. TestCommon itself is now free of it.
Stale local feed gotcha. The package version is unchanged (8.0.0-preview1-dev), so a previously cached copy carrying the old IVT name yields CS0122 in Package mode. Repack and clear ~/.nuget/packages/microsoft.data.sqlclient/8.0.0-preview1-dev when testing locally.
Checklist
Tests added or updated
Public API changes documented — none; internal only
Verified against customer repro — n/a
Ensure no breaking changes introduced — no public surface change
…esHelper
LocalAppContextSwitchesHelper reached the driver's cached switch fields by
reflection, which meant the switch names were stringly-typed and a rename in
LocalAppContextSwitches would only fail at run time.
Make the SwitchValue enum and the s_* cache fields internal and grant
InternalsVisibleTo to Microsoft.Data.SqlClient.TestCommon, so the helper reads
and writes them directly at compile time. All get/set logic now lives in the
helper; the driver keeps only the field declarations.
TestCommon must bind to the implementation assembly for this to work, because
NuGet hands the compiler the ref assembly (public API only) in Package mode.
ExcludeAssets="compile" plus an explicit Reference does that, mirroring what
UnitTests already does. That exclusion also suppresses compile assets across the
package's dependency subtree, so UnitTests now references
Microsoft.Data.SqlClient.Extensions.Abstractions directly - it had been relying
on TestCommon's unrestricted package reference to resolve the
SqlAuthenticationProvider type-forwards.
TestCommon also no longer honours MdsPackageVersion. Test projects support the
driver at the same repo commit, and an older released driver cannot grant
InternalsVisibleTo to this commit's test assemblies. A consumer that pins an
older driver must use the matching revision of these test projects.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Binding LocalAppContextSwitchesHelper to driver internals locks its
assembly to the driver at the same repo commit: an older released driver
neither grants InternalsVisibleTo to this assembly nor guarantees the
internal field shape. TestCommon is shared by every test project, so that
lock propagated to consumers that only ever touch public API.
PerformanceTests is the one that breaks. Its baseline pass pins a released
driver via MdsPackageVersion, which TestCommon must honour or restore fails:
PerformanceTests -> TestCommon -> Microsoft.Data.SqlClient (>= 8.0.0-preview1-dev)
PerformanceTests -> Microsoft.Data.SqlClient (>= 7.1.0) error NU1605
Move the two internals-bound helpers, LocalAppContextSwitchesHelper and
ConnectionPoolVersionScope, into a new Microsoft.Data.SqlClient.TestCommon
.Internals project that carries the IVT grant and the implementation-assembly
binding, and is never pinned. TestCommon keeps the other 20 files, touches
public API only, and honours MdsPackageVersion again, so a pinned baseline
resolves cleanly.
The split is by coupling, not by dependency: most of what stays behind
(the DatabaseObjects fixtures, SqlDataReaderExtensions) does use SqlConnection
and SqlCommand, but only their public surface, so it compiles against any
supported driver version.
Both helpers keep the Microsoft.Data.SqlClient.Tests.Common namespace, so
consumers need no source changes - only a project reference. UnitTests,
FunctionalTests and ManualTests get one; PerformanceTests deliberately does
not, since it uses neither helper.
Validated on net9.0: UnitTests 1313 passed in both Project and Package mode
(the one failure, IntegratedAuthConnectionTest, fails identically on main -
no Kerberos on the host); PerformanceTests builds in Package mode pinned to
7.1.0 and resolves Microsoft.Data.SqlClient/7.1.0 with the Internals assembly
absent from its output; the packed driver carries the retargeted IVT grant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The package-mode implementation binding and signed friend-assembly configuration warrant final human validation.
Review effort: Balanced Findings: None
What changed in this PR
Replaces reflection-based AppContext switch access with compile-time-checked internal access while isolating same-commit driver coupling from reusable test infrastructure.
Changes:
Adds InternalsVisibleTo access for directly manipulating cached switch values.
Moves internals-dependent helpers into a dedicated test assembly.
Updates test references and package-mode dependency resolution.
On non-Windows .NET, this setter now writes s_useManagedNetworking, while the constructor and Dispose deliberately skip capturing/restoring that field. Calling it there therefore leaks the altered cached value beyond the scope, contrary to the helper's restore contract. Guard the write on Windows (as the surrounding remarks describe), or capture and restore the field on every NET target.
Guard the managed networking cache setter on non-Windows platforms and add regression coverage for mutation and restoration behavior.
Refs #4765
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Source: review body, Copilot “Previously missed” finding — managed-networking cache state
Fixed in e136d6fbc. UseManagedNetworking now writes s_useManagedNetworking only on Windows, matching the helper’s capture/restore contract and the production switch behavior. Added regression coverage verifying that the cache is unchanged on non-Windows and restored after disposal on Windows.
Source: review body, Copilot validation concern — package binding and friend assembly
No additional code change was required. The internals helper builds successfully in Package mode against the implementation assembly with 0 warnings and 0 errors. The signed friend-assembly configuration was reviewed; the actual signing-key path remains exercised by CI because no CI signing key is available locally.
Validation:
Focused regression test: 1 passed.
LocalAppContextSwitchesTest: 2 passed.
Package-mode internals-helper build: succeeded with 0 warnings and 0 errors.
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
Area\TestsIssues that are targeted to tests or test projects
4 participants
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.
paulmedynski
added
the
Area\Tests
Summary
Replaces reflection with
InternalsVisibleToinLocalAppContextSwitchesHelper, and splits the internals-bound test helpers into their own assembly so that change does not lock every test project to the same-commit driver.Targets
maindirectly. This supersedes #4759, which was stacked on the now-abandoned #4763.Commit 1 — Use InternalsVisibleTo instead of reflection
The helper reached
LocalAppContextSwitches' cached switch fields by reflection, so switch names were stringly-typed and a rename in the driver would only fail at run time.SwitchValueand thes_*cache fields are nowinternal, with an IVT grant to the helper's assembly. The helper reads and writes them directly, so a rename is now a build break. All get/set logic lives in the helper; the driver keeps only the field declarations (net -112 lines there).Commit 2 — Split internals-bound helpers out of TestCommon
Binding to internals locks an assembly to the driver at the same repo commit.
TestCommonis shared by every test project, so that lock propagated to consumers that only touch public API.PerformanceTestsis the one that breaks. Its baseline pass pins a released driver viaMdsPackageVersion, whichTestCommonmust honour or restore fails:So the two internals-bound helpers —
LocalAppContextSwitchesHelperandConnectionPoolVersionScope— move to a newMicrosoft.Data.SqlClient.TestCommon.Internalsproject that carries the IVT grant and the implementation-assembly binding, and is never pinned.TestCommonkeeps the other 20 files, touches public API only, and honoursMdsPackageVersionagain.The split is by coupling, not by dependency. Most of what stays behind (the
DatabaseObjectsfixtures,SqlDataReaderExtensions) does useSqlConnection/SqlCommand— but only their public surface, so it compiles against any supported driver version. A "no dependencies vs. has dependencies" split would have left those fixtures on the locked side and fixed nothing.Both helpers keep the
Microsoft.Data.SqlClient.Tests.Commonnamespace, so consumers need no source changes — only a project reference.UnitTests,FunctionalTestsandManualTestsget one;PerformanceTestsdeliberately does not, since it uses neither helper.Validation
All on
net9.0:7.1.0Microsoft.Data.SqlClient/7.1.0The single failure is
SimulatedServerTests.ConnectionTests.IntegratedAuthConnectionTest, which fails identically onmain(no Kerberos/SSPI on the Linux host) — verified, not a regression.Package mode is the meaningful run: exercised against a locally packed driver, it confirms the internal field access resolves at run time through the packaged assembly (no
FieldAccessException). The packed DLL was confirmed to carryInternalsVisibleTo+Microsoft.Data.SqlClient.TestCommon.Internals.The perf baseline build was confirmed to resolve
Microsoft.Data.SqlClient/7.1.0withMicrosoft.Data.SqlClient.TestCommon.Internalsabsent from its output — i.e. the lock no longer reachesPerformanceTests.Reviewer notes
runtimes/win/...HintPath. Verified safe today — the win and unix payloads are byte-identical and the driver has zero OS-conditional compilation. Carries the same@TODOasUnitTests.csproj.TestSigningKeyPathis set by no pipeline, and the only pipeline settingSigningKeyPathruns no tests, so tests always match the unsigned grant. ThePublicKey=-qualified grant uses the same blob asUnitTests(same key). I could not verify the blob against the real key — it is an ADO secure file.TestCommon.Internals, since that config intentionally grants no IVT.UnitTestsalready had this constraint; the new project joins it.TestCommonitself is now free of it.8.0.0-preview1-dev), so a previously cached copy carrying the old IVT name yieldsCS0122in Package mode. Repack and clear~/.nuget/packages/microsoft.data.sqlclient/8.0.0-preview1-devwhen testing locally.Checklist
internalonly