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
{{ message }}
Repository navigation
Use InternalsVisibleTo instead of reflection in LocalAppContextSwitchesHelper - #4765
Replace reflection in LocalAppContextSwitchesHelper with typed access through InternalsVisibleTo. Renaming or removing a switch member now produces a compile-time error instead of a runtime reflection failure.
Make LocalAppContextSwitches.SwitchValue and its cached switch fields internal, and grant IVT to the existing Microsoft.Data.SqlClient.TestCommon assembly.
Preserve nullable switch state capture/restoration and the helper's existing serialization.
Preserve platform behavior: the managed networking cache is changed and restored only on Windows; on other platforms the helper leaves it untouched.
Support package-mode compilation by referencing the implementation DLL under lib/{tfm} rather than the public-only reference assembly. Map modern tests to the shipped net10.0 implementation.
Retain the existing signing conditions and add the public-key-qualified TestCommon IVT grant for signed package validation.
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.
Merging main brought in #4673 (Remove RID-specific SqlClient DLLs), which
deleted every runtimes/ entry from the SqlClient nuspec. The new
TestCommon.Internals project still pointed its Package-mode HintPath at
runtimes/win/lib/<tfm>, which no longer exists. Because ExcludeAssets="compile"
drops the ref assembly, this left no resolvable SqlClient reference and broke
Package-mode builds with MSB3245 followed by CS0234.
Point the HintPath at lib/<tfm>, matching the fix#4673 applied to
UnitTests.csproj, and drop the @todo hack comment that PR also removed, since
the RID-specific binaries it referred to no longer exist.
Verified by packing the driver from this commit (0 runtimes/ entries) and
building TestCommon.Internals in Package mode for net8.0 and net9.0, plus the
consuming UnitTests project. Reverting the HintPath reproduces MSB3245.
Addresses Copilot review feedback on PR #4765.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot — "package-mode implementation binding ... warrants final human validation" (review, review) Correcting my previous answer. My last run reported that no code change was required and that Package mode built cleanly. That was validated against a locally cached package built before the main merge, so it was wrong. Re-validating against a package packed from the current commit shows the binding genuinely was broken: Remove RID-specific SqlClient DLLs #4673 removed the runtimes/ payload the HintPath relied on. Fixed in 89ed5a5.
Copilot — "Fix invalid SqlClient HintPath in Package mode" (review, review)
Duplicate of inline thread r4156204815; fixed in 89ed5a5 and answered in that thread.
Discussion comments
No actionable discussion comments. The only human comment is my own prior summary; the Codecov post is operational noise and was set aside.
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why there's a new project. Feel free to push back and explain, but for now I'd request that the common internals project get rolled into the common project.
The reason will be displayed to describe this comment to others. Learn more.
Sorry ... why do we need a new project for this? Can we just leave this stuff the TestCommon project? It seems to me we have three "common" projects for tests with this change. I'd prefer to reduce that number than increase it :)
Replace reflection with typed access to cached AppContext switches. Grant internals access to TestCommon, retain signed package-mode support, and preserve managed networking behavior across platforms.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
benrr101
left a comment
Description
Replace reflection in
LocalAppContextSwitchesHelperwith typed access throughInternalsVisibleTo. Renaming or removing a switch member now produces a compile-time error instead of a runtime reflection failure.LocalAppContextSwitches.SwitchValueand its cached switch fields internal, and grant IVT to the existingMicrosoft.Data.SqlClient.TestCommonassembly.lib/{tfm}rather than the public-only reference assembly. Map modern tests to the shippednet10.0implementation.