Common | Wire Manual Tests to Common MDS (part 2) #3940

benrr101 · 2026-02-06T00:16:02Z

Description

This PR completes the work to wire up the manual tests project to the common MDS project. To help understand these changes, it's helpful to think about what the ManualTests project references:

MDS (separate)
AKVProvider
- MDS (separate)
Common
- MDS (separate)
CustomRetryLogicProvider
- MDS (separate)
UDTs
- MSS

As you can see, there are every reference to MDS (separate) needs to be changed. After these changes, the references look like:

MDS (common)
AKV Provider
- MDS (common)
TestCommon
- MDS (common)
CustomRetryLogicProvider
- MDS (common)
UDTs
- MSS - has no references to MDS, thankfully.

The build.proj file then has several changes that needed to be made

RunManualTests* - changed to build dependencies, just like Unit and Functional tests were updated to do
BuildManualTests* - became no-ops because they were built at test-time, so they were removed
BuildTests* - became no-ops because they all the targets they referenced were removed, so they themselves were removed
RestoreTests* - Were never being called and regardless were unnecessary due to restore occurring at test-time, so they were removed
Filter/Collect/Blame arguments for tests were consolidated

And if you give a build.proj a cookie, chances are a pipeline will be offended...

build-all-tests-step became a no-op since build for tests now happens at test-time, so it was removed
ci-run-tests-job referenced build-all-tests-step, so that reference was removed
build-and-run-tests-*-step
- These are technically no-op changes because the job that references this step is not being used - it was the official MDS pipeline test stage which is not supported at the moment.
- build-and-run-tests-*-step contained calls to BuildTests* so those steps were removed
- build-and-run-tests-*-step contained calls to build AKV provider before tests, this isn't necessary any longer because AKV is built as a dependency in the ManualTests project
- This file will need to changed because it directly references test projects via msbuild task. This isn't how we want to run these, so they must be changed if we ever re-enable testing during official build.

And some other msc changes to clean things up:

Added the recently added pipeline yaml files to the solution
Removed the no longer used Common.csproj file

Testing

Local testing shows this should work fairly well. However, the pipelines are impossible to decipher in any reasonable amount of time. I patched up what I could see was linked together, but expect failures on the package reference pipeline.

Copilot

Pull request overview

This PR completes the migration of the ManualTests project to reference a common Microsoft.Data.SqlClient (MDS) build instead of maintaining separate references. The changes consolidate the build system, simplify project dependencies, and remove redundant build targets.

Changes:

Consolidated ManualTests project dependencies to reference a single common MDS build instead of separate netfx/netcore builds
Removed obsolete build targets (BuildTests*, RestoreTests*, BuildManualTests*) from build.proj since tests now build at test-time
Streamlined test execution by removing pre-build steps from pipelines and consolidating test arguments
Removed the Common.csproj project (replaced by Microsoft.Data.SqlClient.TestCommon.csproj)
Updated project and resource file references to use consistent naming conventions

Reviewed changes

Copilot reviewed 25 out of 31 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
build.proj	Simplified test targets, consolidated test arguments (Filter, Blame, Collect), removed explicit build targets for tests
Microsoft.Data.SqlClient.ManualTests.csproj	Restructured project with TestSet-based compilation, updated references to use common MDS and TestCommon projects
SqlParameterTest_ReleaseMode.bsl	New baseline file added (1774 lines) - appears to be test output
DDDataTypesTest_Data.xml	Renamed/added data file for DDDataTypesTest
Various test files	Refactored tests (expression bodies, theory-based tests), updated query syntax (sysobjects→sys.objects)
SqlSetupStrategyCspProvider.cs	New file with concatenated license header issue on line 3
Pipeline YAML files	Removed build-all-tests-step and pre-build steps from test pipelines
Solution file	Updated project names, added pipeline files, removed Common.csproj reference
AKV Provider project	Simplified project structure, removed BuildProjectReferences property
Common.csproj	Deleted (replaced by TestCommon.csproj)

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/Batch/BatchTests.netcore.cs:84

The #if NET8_0_OR_GREATER conditional compilation directive has been removed, but the file is now wrapped with #if NET which is broader. This means the SqlBatchCanCreateParameter test will now run on all .NET versions (not just .NET 8.0+), which may cause issues if the API being tested is only available in .NET 8.0+.

Verify that the DbBatch.CreateBatchCommand() API and related functionality tested in SqlBatchCanCreateParameter is available in all .NET versions this file targets, or restore the #if NET8_0_OR_GREATER guard for that specific test.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:4

The @TODO comment on line 3 indicates that the assembly name should use the full name, but ManualTests is a very generic name that could conflict with other test assemblies.

Consider using a more specific assembly name like Microsoft.Data.SqlClient.ManualTests to:

Avoid potential naming conflicts
Make it clear which component this assembly belongs to
Follow the namespace convention already established in RootNamespace

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs

...icrosoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.Debug.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DDBasics/DDDataTypesTest/DDDataTypesTest.cs

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj

build.proj

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DDBasics/DDDataTypesTest/DDDataTypesTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/SqlServerTypesTest.cs

mdaigle · 2026-02-06T16:58:24Z

...icrosoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.Debug.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#if DEBUG


✏️ Similar to .Net vs Framework, I'd like to eventually set up a custom conditional test attribute so that we can show these tests as skipped when running in release config.

mdaigle · 2026-02-06T17:06:04Z

build.proj

    <PropertyGroup>
      <TestCommand>
        $(DotnetPath)dotnet test "@(ManualTestsProj)"
-        --no-build


If we remove this, make sure to update the buildguide to remove the test restore/compile steps.

…ssary

…cies

…set is only set if provided.

…pendency for manual tests.

… used anywhere

…orward.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

BUILDGUIDE.md

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj

codecov · 2026-02-09T21:31:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.93%. Comparing base (3ffbba7) to head (6ba2179).
⚠️ Report is 4 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (3ffbba7) and HEAD (6ba2179). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (3ffbba7) HEAD (6ba2179)

addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3940      +/-   ##
==========================================
- Coverage   74.53%   67.93%   -6.61%     
==========================================
  Files         266      260       -6     
  Lines       42915    65704   +22789     
==========================================
+ Hits        31987    44634   +12647     
- Misses      10928    21070   +10142

Flag	Coverage Δ
addons	`?`
netcore	`68.13% <ø> (-6.65%)`	⬇️
netfx	`66.71% <ø> (-7.01%)`	⬇️

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

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

paulmedynski

Looks great! Only one minor fix.

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj

build.proj

cheenamalhotra

Ready to check in after you've resolved conflicts :)

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

eng/pipelines/common/templates/steps/build-and-run-tests-netfx-step.yml

Copilot · 2026-02-11T20:22:47Z

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj

    <ProjectReference Include="SQL/UdtTest/UDTs/Shapes/Shapes.csproj" />
    <ProjectReference Include="SQL/UdtTest/UDTs/Utf8String/Utf8String.csproj" />
-    <ProjectReference Include="$(TestsPath)Common/Common.csproj" />
+    <ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />


In ReferenceType=Package mode, ManualTests references Microsoft.Data.SqlClient via a PackageReference, but Microsoft.Data.SqlClient.TestCommon is still brought in via ProjectReference and that project currently references Microsoft.Data.SqlClient.csproj unconditionally. This mixes package + project references to the same assembly and will either break the build (duplicate assembly resolution) or prevent package-mode tests from actually validating the packaged driver. Make TestCommon’s driver dependency conditional on ReferenceType (project ref for Project mode, package ref for Package mode), or avoid referencing TestCommon in package-mode runs.

Suggested change

<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />

<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj"

Condition="'$(ReferenceType)' != 'Package'" />

Hmm... seems like a legit problem.

Copilot · 2026-02-11T20:22:47Z

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj

    <ProjectReference Include="SQL/UdtTest/UDTs/Shapes/Shapes.csproj" />
    <ProjectReference Include="SQL/UdtTest/UDTs/Utf8String/Utf8String.csproj" />
-    <ProjectReference Include="$(TestsPath)Common/Common.csproj" />
+    <ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />


Same issue as netfx: in ReferenceType=Package mode, this ProjectReference to TestCommon transitively pulls in a project reference to Microsoft.Data.SqlClient.csproj, which conflicts with the ManualTests package reference to Microsoft.Data.SqlClient. Update TestCommon to honor ReferenceType (project ref vs package ref) so package-mode runs actually test the packaged driver and avoid duplicate references.

Suggested change

<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />

<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj"

Condition="'$(ReferenceType)' != 'Package'" />

Copilot · 2026-02-11T20:22:47Z

eng/pipelines/common/templates/steps/build-and-run-tests-netcore-step.yml

 steps:
- task: MSBuild@1
-  displayName: 'Build AKV Provider .NET'
-  inputs:
-    solution: build.proj
-    msbuildArchitecture: x64
-    msbuildArguments: >-
-      -p:Configuration=Release
-      -t:BuildAKVNetCore
-      -p:ReferenceType=Package
-      -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }}
-      -p:MdsPackageVersion=${{ parameters.mdsPackageVersion }}
-
- task: MSBuild@1
-  displayName: 'MSBuild Build Tests for ${{parameters.TargetNetCoreVersion }}'
-  inputs:
-    solution: build.proj
-    msbuildArchitecture: x64
-    msbuildArguments: >-
-      -t:BuildTestsNetCore
-      -p:ReferenceType=Package
-      -p:TargetNetCoreVersion=${{ parameters.TargetNetCoreVersion }}
-      -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }}
-      -p:MdsPackageVersion=${{ parameters.mdsPackageVersion }}
-      -p:Configuration=Release

 # Don't run unit tests using package reference. Unit tests are only run using project reference.


This template no longer builds the test projects (the MSBuild build steps were removed), but the remaining DotNetCoreCLI test invocations still pass --no-build. If this template is invoked, dotnet test will fail unless the build happens elsewhere. Either remove --no-build (and let dotnet test build) or add an explicit build step back in. Also, the Manual Tests project path used later in this file points to Microsoft.Data.SqlClient.ManualTesting.Tests.csproj, which doesn’t exist under src/Microsoft.Data.SqlClient/tests/ManualTests/ anymore.

Yeah, I'm letting the verification stage of the official mds pipeline languish right now, since it's completely commented out.

Copilot

Pull request overview

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

Copilot · 2026-02-12T19:38:09Z

NuGet.config


+    <!--
+      This package source is used by developers and our pipelines builds when we need to reference
+      packages that are not available on the public nuget.org feed, for example to test MDS that
+      depends on a version of Abstractions that was just built in the workspace.
+    -->
+    <add key="local" value="packages/" />


Adding the repo-local packages/ folder as an always-on NuGet package source in NuGet.config weakens the “governed feed only” guarantee: a PR could add a higher-version package into packages/ and have restores pull it instead of the vetted feed. Consider keeping the local source only for package-reference scenarios (e.g., via a separate config + --configfile), or add packageSourceMapping to ensure only the intended internal packages can come from local and all other IDs must come from governed.

Suggested change



<add key="local" value="packages/" />

Intriguing - would packageSourceMapping allow us to only pull our packages from local?

paulmedynski

Waiting to see if the Copilot suggestions are legit.

mdaigle · 2026-02-12T23:54:26Z

NuGet.config

+      packages that are not available on the public nuget.org feed, for example to test MDS that
+      depends on a version of Abstractions that was just built in the workspace.
+    -->
+    <add key="local" value="packages/" />


Leaving my note that I am against this approach. I won't block you because it's not significantly riskier than the state we were in with two files, but I want to change this in the future.

benrr101 requested a review from a team as a code owner February 6, 2026 00:16

Copilot AI review requested due to automatic review settings February 6, 2026 00:16

benrr101 marked this pull request as draft February 6, 2026 00:16

benrr101 added the Common Project 🚮 Things that relate to the common project project label Feb 6, 2026

Copilot started reviewing on behalf of benrr101 February 6, 2026 00:16 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

mdaigle reviewed Feb 6, 2026

View reviewed changes

benrr101 added 14 commits February 6, 2026 11:52

Cleanup AKV provider project file

ecdbdb0

Point AKV provider at common MDS project

a38293c

Point custom retry logic provider at common MDS

88f53ca

Point manual tests against CommonTest and Common MDS projects.

3a799e7

Remove BuildManualTests* build.proj targets - they are no longer nece…

97580fb

…ssary

RunManualTests* build.proj targets now build the project and dependen…

9014548

…cies

Cleanup filter/collect/blame arguments for test targets, ensure test …

1361d3e

…set is only set if provided.

Remove BuildTests* targets from build.proj, remove from pipelines.

8c33e24

Drop build-all-tests-step.yml since it no longer does anything

a6d9932

Remove AKV build from before test execution - it is now built as a de…

1ecb6c0

…pendency for manual tests.

Remove manual tests from the restoretests* targets

d67226f

Remove RestoreTests* targets as they were doing nothing and not being…

f968fa7

… used anywhere

Someone didn't add their new pipeline files to the solution... tsk tsk 😉

65e3f6c

Remove Common.csproj. Use Microsoft.Data.SqlClient.TestCommon going f…

e2aa797

…orward.

benrr101 force-pushed the dev/russellben/common/manual-tests-part2 branch from bb6c937 to e2aa797 Compare February 6, 2026 18:02

Update buildguide

4d0b490

Copilot AI review requested due to automatic review settings February 6, 2026 19:08

Copilot started reviewing on behalf of benrr101 February 6, 2026 19:08 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

benrr101 marked this pull request as ready for review February 6, 2026 19:20

benrr101 changed the title ~~[DRAFT] Common | Wire Manual Tests to Common MDS (part 2)~~ Common | Wire Manual Tests to Common MDS (part 2) Feb 6, 2026

Typos

fcafada

priyankatiwari08 self-assigned this Feb 9, 2026

benrr101 added this to the 7.0.0-preview4 milestone Feb 10, 2026

paulmedynski self-assigned this Feb 11, 2026

paulmedynski requested changes Feb 11, 2026

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj Show resolved Hide resolved

build.proj Show resolved Hide resolved

mdaigle previously approved these changes Feb 11, 2026

View reviewed changes

cheenamalhotra previously approved these changes Feb 11, 2026

View reviewed changes

Merge main and fix comment from Paul

6ba2179

Copilot AI review requested due to automatic review settings February 11, 2026 20:14

benrr101 dismissed stale reviews from cheenamalhotra and mdaigle via 6ba2179 February 11, 2026 20:14

Copilot started reviewing on behalf of benrr101 February 11, 2026 20:14 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

paulmedynski previously approved these changes Feb 11, 2026

View reviewed changes

Make nuget.config.local the nuget.config

618991c

benrr101 dismissed paulmedynski’s stale review via 618991c February 12, 2026 18:44

Remove nuget.config switches from pipelines

2a67d11

Copilot AI review requested due to automatic review settings February 12, 2026 19:28

Copilot started reviewing on behalf of benrr101 February 12, 2026 19:28 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

paulmedynski reviewed Feb 12, 2026

View reviewed changes

mdaigle reviewed Feb 12, 2026

View reviewed changes

	<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />
	<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj"
	Condition="'$(ReferenceType)' != 'Package'" />

Common | Wire Manual Tests to Common MDS (part 2) #3940

Are you sure you want to change the base?

Common | Wire Manual Tests to Common MDS (part 2) #3940

Uh oh!

Conversation

benrr101 commented Feb 6, 2026

Description

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cheenamalhotra left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 9, 2026 •

edited

Loading