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.
Stack: this is PR 2 of 2, stacked on #4763. This PR targets dev/paul/perf-source-baseline, so the diff shown here is just the IVT commit. Merge #4763 first; this PR will retarget to main automatically.
Why stacked
The dependency runs one way. This PR stops TestCommon honouring MdsPackageVersion, because a test project supports the driver at the same repo commit and an older released driver cannot grant InternalsVisibleTo to this commit's test assemblies. But PerformanceTests still set that property, so landing this alone produces:
#4763 removes the setting side, so with it in first this lands cleanly.
The change
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 Microsoft.Data.SqlClient.TestCommon. 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).
Two build consequences, both verified:
TestCommon must bind to the implementation assembly. In Package mode NuGet hands the compiler the ref/ assembly (public API only, ~74 KB vs ~1.9 MB), where internals and the IVT attribute don't exist. ExcludeAssets="compile" plus an explicit Reference fixes that, mirroring what UnitTests already does.
UnitTests now references Abstractions directly. That exclusion also suppresses compile assets across the package's dependency subtree, which is where Microsoft.Data.SqlClient.Extensions.Abstractions came from. UnitTests had been free-riding on TestCommon's unrestricted package reference to resolve the SqlAuthenticationProvider type-forwards.
Validation
Scenario
Result
UnitTests net8.0 — Project mode
1293 passed, 0 failed
UnitTests net8.0 — Package mode
1293 passed, 0 failed
TestCommon — Project / Package / Package + stale pin
0 errors (pin ignored)
FunctionalTests, ManualTests, PerformanceTests
0 errors
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.
Reviewer notes
runtimes/win/... HintPath. Verified safe today — the win and unix payloads are byte-identical and the driver has zero OS-conditional compilation (no #if _WINDOWS/_UNIX, no .windows.cs/.unix.cs). 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. TestCommon already had the test-signing block, and its PublicKey=-qualified grant uses the same blob as UnitTests (same key), so it is covered symmetrically if that path is ever wired up. I could not verify the blob against the real key — it is an ADO secure file.
Signed + Project mode now cannot compile TestCommon, since that config intentionally grants no IVT. UnitTests already had this constraint; TestCommon joins it.
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
Replaces reflective AppContext-switch access with compile-time internal access and moves performance baselines from released packages to tagged source builds.
Changes:
Adds InternalsVisibleTo access for TestCommon and direct cached-switch access.
Adjusts package-mode references for implementation assemblies and dependencies.
Reworks performance baselines to build release tags from source.
Correct — both helpers fetched --no-tags with a branch-only refspec, so refs/heads/v7.1.0 never resolved and the origin path could not succeed for any tagged baseline, sending every run to the public fallback clone even where origin was reachable.
Both now try refs/heads/<ref> first and fall back to refs/tags/<ref>, mapping either into the same private refs/remotes/perfbaseline/<ref> namespace so the existing isolation from a same-named local branch is preserved. The public clone stays as a last resort.
Two issues found by Copilot review on #4759, both in the move to
source-built perf baselines.
Baseline acquisition built a branch-only refspec, so a 'v<version>' tag
never resolved from the checkout's origin and every tagged run silently
fell through to cloning the public fallback URL - breaking runs where
the ADO origin is reachable but GitHub egress is not. Both helpers now
try refs/heads first and then refs/tags, mapping either into the same
private perfbaseline namespace.
The default baseline of 7.0.2 also predated the perf harness protocol.
Interleaved execution drives the benchmark app one unit at a time via
PERF_LIST_BENCHMARKS/PERF_BENCHMARK, which arrived in v7.1.0-preview3;
every v7.0.x Program ignores both variables and runs the whole suite, so
each requested unit would have run everything and the baseline/candidate
pairing would have been meaningless. Raise the default to 7.1.0 and have
both scripts check the materialised baseline tree for the protocol,
failing fast instead of emitting an invalid comparison or timing out.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update README to describe the source-based baseline
eng/pipelines/perf/README.md:16
The updated script description now says the baseline is source-based, but this README still describes the overall comparison and the main pipeline row as using a released NuGet package. Update those introductory references so the documented architecture is consistent.
…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>
Superseded by #4765, which targets main directly. This PR could not be retargeted or reopened after its base branch (#4763) was abandoned and deleted.
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\EngineeringUse this for issues that are targeted for changes in the 'eng' folder or build systems.Area\TestsIssues that are targeted to tests or test projects
2 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
Area\Tests
Summary
Replaces reflection with
InternalsVisibleToinLocalAppContextSwitchesHelper.Stack: this is PR 2 of 2, stacked on #4763. This PR targets
dev/paul/perf-source-baseline, so the diff shown here is just the IVT commit. Merge #4763 first; this PR will retarget tomainautomatically.Why stacked
The dependency runs one way. This PR stops
TestCommonhonouringMdsPackageVersion, because a test project supports the driver at the same repo commit and an older released driver cannot grantInternalsVisibleToto this commit's test assemblies. ButPerformanceTestsstill set that property, so landing this alone produces:#4763 removes the setting side, so with it in first this lands cleanly.
The change
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 toMicrosoft.Data.SqlClient.TestCommon. 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).Two build consequences, both verified:
ref/assembly (public API only, ~74 KB vs ~1.9 MB), where internals and the IVT attribute don't exist.ExcludeAssets="compile"plus an explicitReferencefixes that, mirroring whatUnitTestsalready does.Microsoft.Data.SqlClient.Extensions.Abstractionscame from.UnitTestshad been free-riding on TestCommon's unrestricted package reference to resolve theSqlAuthenticationProvidertype-forwards.Validation
net8.0— Project modenet8.0— Package modePackage 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.Reviewer notes
runtimes/win/...HintPath. Verified safe today — the win and unix payloads are byte-identical and the driver has zero OS-conditional compilation (no#if _WINDOWS/_UNIX, no.windows.cs/.unix.cs). Carries the same@TODOasUnitTests.csproj.TestSigningKeyPathis set by no pipeline, and the only pipeline settingSigningKeyPathruns no tests, so tests always match the unsigned grant. TestCommon already had the test-signing block, and itsPublicKey=-qualified grant uses the same blob as UnitTests (same key), so it is covered symmetrically if that path is ever wired up. I could not verify the blob against the real key — it is an ADO secure file.UnitTestsalready had this constraint; TestCommon joins it.Checklist
internalonly