DRAFT | Wire Manual Tests to Common MDS #3916

benrr101 · 2026-01-27T00:40:39Z

Wiring up manual tests project to common MDS. Since a lot of these tests fail locally for me on main, I'm going to run here to validate. This probably breaks all the things.

…ts, rework test set parameter, group the easy files to group.

…why this is just now a problem, but ok, I'll fix it.

Copilot

Pull request overview

Wires the ManualTests project to the common Microsoft.Data.SqlClient (MDS) build/test infrastructure, including OS-conditional compilation and updated build orchestration.

Changes:

Reworks Microsoft.Data.SqlClient.ManualTests.csproj to compile against common test infrastructure, support “test sets”, and add OS preprocessor constants.
Adds OS / framework guards (#if _WINDOWS, #if NET, #if NETFRAMEWORK, #if DEBUG) to prevent cross-OS/framework compilation issues.
Updates build/solution wiring (project rename/path changes, build target adjustments, test invocation changes).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/SqlServerTypesTest.cs	Refactors UDT read tests to parameterize behavior and centralize query/expected bytes.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.windows.cs	Wraps Windows-only test with `_WINDOWS` compilation guard.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlDSEnumeratorTest/SqlDataSourceEnumeratorTest.windows.cs	Wraps Windows-only enumerator test with `_WINDOWS` compilation guard and removes platform attributes.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs	Renames/reshapes test methods (TCP/NamedPipe/MARS variants) and modernizes async test signatures.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.netfx.cs	Wraps .NET Framework-only test code with `NETFRAMEWORK` guard.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.Debug.cs	Adds license header and gates debug-only helpers behind `#if DEBUG`.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/Batch/BatchTests.netcore.cs	Wraps netcore-only tests behind `#if NET` and adjusts conditional compilation.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj	Major restructure: OS constants, test-set compilation model, new reference wiring, and content/resource copy rules.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.windows.cs	Adds `_WINDOWS` guard for CSP setup fixture.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/CspProviderColumnMasterKey.windows.cs	Adds `_WINDOWS` guard for CSP CMK helper.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/DateOnlyReadTests.netcore.cs	Adds `NET` guard for netcore-only DateOnly tests.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/CspProviderExt.windows.cs	Adds `_WINDOWS` guard and removes runtime platform attributes.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj	Updates comments and changes how `xunit.runner.json` is included/copied.
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj	Reworks output/reference properties and reference-to-MDS strategy.
src/Microsoft.Data.SqlClient.sln	Renames ManualTests project entry to `Microsoft.Data.SqlClient.ManualTests`.
build.proj	Updates ManualTests project path and changes test build/run targets and command formatting.

Comments suppressed due to low confidence (3)

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

The netcore ItemGroup includes Microsoft.Data.SqlClient.TestCommon.csproj twice (once via $(RepoRoot) and once via $(TestsPath)). This can lead to duplicate compile items / assembly conflicts. Keep only one reference and standardize on a single path pattern for this repo.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.windows.cs:4
The license header line appears to have an accidental using System; appended ("...more information.using System;"). This looks like a copy/paste typo and should be split so the comment ends at the period and the using is on its own line (or removed if redundant).
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:381
Duplicate project reference: $(TestsPath)Common/Microsoft.Data.SqlClient.TestCommon.csproj is referenced here, but the same project is already referenced earlier in this ItemGroup via $(RepoRoot)/src/Microsoft.Data.SqlClient/tests/Common/.... Remove one to avoid duplicate builds/inputs.

Copilot · 2026-01-27T00:47:27Z

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

+            "  geometry::Parse('LINESTRING (100 100, 20 180, 180 180') AS col1, " +
+            "  geography::Parse('LINESTRING (-122.360 47.656, -122.343 47.656)' AS col2";


BuiltInUdtSelectQuery currently contains invalid SQL (missing closing parentheses/quotes in both geometry::Parse(...) and geography::Parse(...), and the SELECT never terminates). This will fail at runtime and break the UDT tests. Update the query string so each Parse call has balanced quotes/parentheses and the statement is complete.

Suggested change

" geometry::Parse('LINESTRING (100 100, 20 180, 180 180') AS col1, " +

" geography::Parse('LINESTRING (-122.360 47.656, -122.343 47.656)' AS col2";

" geometry::Parse('LINESTRING (100 100, 20 180, 180 180)') AS col1, " +

" geography::Parse('LINESTRING (-122.360 47.656, -122.343 47.656)') AS col2;";

Copilot · 2026-01-27T00:47:27Z

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


-                    SqlBytes sqlBytes = null;
+            using SqlDataReader reader = command.ExecuteReader(behavior);
+            reader.Read();


reader.Read() result is not checked. If the query returns no rows, subsequent GetSqlBytes calls will throw a less-informative exception. Prefer Assert.True(reader.Read()) (as in other tests in this file) to fail with a clear message.

Suggested change

reader.Read();

Assert.True(reader.Read());

It doesn't really matter. If the read fails, the subsequent actions will fail as well.

Copilot · 2026-01-27T00:47:28Z

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

-                {
-                    Assert.True(reader.Read());
+            using SqlDataReader reader = command.ExecuteReader(behavior);
+            reader.Read();


reader.Read() result is not checked. If the query returns no rows, subsequent GetBytes calls will throw a less-informative exception. Prefer Assert.True(reader.Read()) (consistent with other tests in this file).

Suggested change

reader.Read();

Assert.True(reader.Read());

It doesn't really matter. If the read fails, the subsequent actions will fail as well.

Copilot · 2026-01-27T00:47:28Z

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

-                {
-                    Assert.True(reader.Read());
+            using SqlDataReader reader = command.ExecuteReader(behavior);
+            reader.Read();


reader.Read() result is not checked. If the query returns no rows, GetStream will throw a less-informative exception. Prefer Assert.True(reader.Read()) for a clearer failure and consistency with other tests in this file.

Suggested change

reader.Read();

Assert.True(reader.Read());

It doesn't really matter. If the read fails, the subsequent actions will fail as well.

Copilot · 2026-01-27T00:47:28Z

build.proj

+  <Target Name="BuildTestsNetCore">
+    <Message Text=">>> No op. Will be removed." />
+  </Target>
+  <Target Name="BuildTestsNetFx">
+    <Message Text=">>> No op. Will be removed." />
+  </Target>


BuildTestsNetCore/BuildTestsNetFx have been turned into no-op targets, but they are still invoked by repo tooling (e.g., eng/pipelines/common/templates/steps/build-all-tests-step.yml and BUILDGUIDE.md). This will cause CI/build scripts to stop producing test binaries while downstream test runs still use --no-build in some pipelines. Either restore the build logic in these targets or update the pipelines/docs to stop calling them and ensure tests are built elsewhere.

Yes, this is temporary until I can validate the manual tests pass. Then I'll come back through and remove the build test targets and remove those steps from the pipelines.

Copilot · 2026-01-27T00:47:28Z

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

+  <PropertyGroup>
+    <!-- @TODO: $(BindFolder) is OS dependent but we're only ever building AnyOS, so ... can we simplify this? -->
+    <OutputPath>$(BinFolder)$(Configuration).$(Platform)\$(AddOnName)</OutputPath>
+    <IntermediateOutputPath>$(ObjFolder)$(Configuration).$(Platform)\$(AddOnName)</IntermediateOutputPath>
+    <DocumentationFile>$(BinFolder)$(Configuration).$(Platform)\$(AssemblyName).xml</DocumentationFile>


$(AddOnName) is used in OutputPath/IntermediateOutputPath, but the <AddOnName> property was removed from this project. Unless it is defined in an imported props/targets (could not find any), this will expand to empty and change output layout unexpectedly. Reintroduce <AddOnName>AzureKeyVaultProvider</AddOnName> or switch these paths to use a defined property (e.g., $(AssemblyName)).

Copilot · 2026-01-27T00:47:29Z

...crosoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj

+    <Content Include="$(TestsPath)tools/Microsoft.Data.SqlClient.TestUtilities/xunit.runner.json">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <TargetPath>xunit.runner.json</TargetPath>
-    </ContentWithTargetPath>
+    </Content>


Most test projects in this repo include xunit.runner.json via ContentWithTargetPath (e.g., tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj:22-25). Switching to plain Content here may change how TargetPath is honored and can cause the runner config not to be copied as xunit.runner.json. Consider using ContentWithTargetPath for consistency and to preserve the intended output path.

ContentWithTargetPath is an internal tag that isn't meant to be used. Content is the right tag to use.

Copilot · 2026-01-27T00:47:29Z

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

+            void ActAndAssert(int index, string expectedHexString)
+            {
+                // Act
+                MemoryStream buffer = new MemoryStream();


Disposable 'MemoryStream' is created but not disposed.

Suggested change

MemoryStream buffer = new MemoryStream();

using MemoryStream buffer = new MemoryStream();

…rverTypesTest

…ill come back to address pipeline and build.proj issues

Copilot

Pull request overview

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

Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.windows.cs:7

The header comment and a using directive appear to be accidentally concatenated: // ... information.using System;. This reads like a typo and can confuse readers.

Consider splitting this into a normal license header line and leaving using System; as an actual using directive below.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:36

ValidateOs compares $(TargetOS) to $(OS), but this project defines TargetOs (lowercase s) and there is no TargetOS property in the repo. As written, $(TargetOS) will be empty and the build will fail with the <Error> on every build.

Use $(TargetOs) (or $(NormalizedTargetOs)) in the condition so the check only triggers when the OS is intentionally overridden.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:379

The netcore ItemGroup includes Microsoft.Data.SqlClient.TestCommon.csproj twice (once via $(RepoRoot) and once via $(TestsPath)), and these paths resolve to the same file. Keeping both can cause duplicate ProjectReference items and makes dependency management harder.

Remove one of the two references and standardize on a single path style.

Copilot · 2026-01-27T20:12:12Z

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

+        [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
+        [InlineData(CommandBehavior.Default)]
+        [InlineData(CommandBehavior.SequentialAccess)]
+        public static void TestUdtSqlDataReaderGetBytesSequentialAccess(CommandBehavior behavior)


The method name TestUdtSqlDataReaderGetBytesSequentialAccess no longer matches what it tests: it now runs for both CommandBehavior.Default and CommandBehavior.SequentialAccess via [InlineData]. Renaming it to a behavior-agnostic name (or splitting into two methods again) would make test intent clearer.

Suggested change

public static void TestUdtSqlDataReaderGetBytesSequentialAccess(CommandBehavior behavior)

public static void TestUdtSqlDataReaderGetBytes(CommandBehavior behavior)

Copilot · 2026-01-27T20:12:12Z

build.proj

  <Target Name="BuildAKVNetFx" Condition="'$(IsEnabledWindows)' == 'true'">
-    <MSBuild Projects="@(AKVProvider)" Targets="restore" Properties="TestTargetOS=$(TestOS)netfx" />
-    <Message Text=">>> Building AKVNetFx [$(CI);TestTargetOS=$(TestOS)netfx;Platform=AnyCPU;$(TestProjectProperties)] ..." Condition="!$(ReferenceType.Contains('Package'))" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netfx;Platform=AnyCPU;$(TestProjectProperties);$(NugetPackProperties);" Condition="!$(ReferenceType.Contains('Package'))" />
-
-    <!-- Only build platform specific builds for Package reference types -->
-    <Message Text=">>> Building AKVNetFx [$(CI);TestTargetOS=$(TestOS)netfx;Platform=$(Platform);$(TestProjectProperties)] ..." Condition="$(ReferenceType.Contains('Package'))" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netfx;Platform=$(Platform);$(TestProjectProperties);$(NugetPackProperties);" Condition="$(ReferenceType.Contains('Package'))" />
+    <Message Text=">>> No op. This will be removed" />
  </Target>

  <Target Name="BuildAKVNetCore">
-    <MSBuild Projects="@(AKVProvider)" Targets="restore" Properties="TestTargetOS=$(TestOS)netcoreapp" />
-    <Message Text=">>> Building AKVNetCore [$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=AnyCPU;ReferenceType=$(ReferenceType);] ..." Condition="!$(ReferenceType.Contains('Package'))" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=AnyCPU;" Condition="!$(ReferenceType.Contains('Package'))" />
-
-    <!-- Only build platform specific builds for Package reference types -->
-    <Message Text=">>> Building AKVNetCore [$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=$(Platform);ReferenceType=$(ReferenceType);] ..." Condition="$(ReferenceType.Contains('Package'))" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=$(Platform);" Condition="$(ReferenceType.Contains('Package'))" />
+    <Message Text=">>> No op. This will be removed" />
  </Target>

  <Target Name="BuildAKVNetCoreAllOS">
-    <MSBuild Projects="@(AKVProvider)" Targets="restore" Properties="TestTargetOS=$(TestOS)netcoreapp" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=AnyCPU;OSGroup=Unix;" RemoveProperties="TargetsWindows;TargetsUnix;" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=AnyCPU;OSGroup=Windows_NT;" RemoveProperties="TargetsWindows;TargetsUnix;" Condition="'$(IsEnabledWindows)' == 'true'" />
-    <MSBuild Projects="@(AKVProvider)" Properties="$(CI);TestTargetOS=$(TestOS)netcoreapp;$(ProjectProperties);Platform=AnyCPU;OSGroup=AnyOS;" RemoveProperties="TargetsWindows;TargetsUnix;" />
+    <Message Text=">>> No op. This will be removed" />
  </Target>


BuildAKVNetFx, BuildAKVNetCore, and BuildAKVNetCoreAllOS are now no-ops. The pipeline explicitly calls BuildAKVNetCore before building/running tests (see eng/pipelines/common/templates/steps/build-and-run-tests-netcore-step.yml), so this will prevent the AKV provider from being built where required.

Either re-enable these targets (or make them call the existing BuildAkv target), or adjust the pipeline to build the AKV provider as part of the test build/test run.

Copilot · 2026-01-27T20:12:12Z

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

+  <PropertyGroup>
+    <!-- @TODO: $(BindFolder) is OS dependent but we're only ever building AnyOS, so ... can we simplify this? -->
+    <OutputPath>$(BinFolder)$(Configuration).$(Platform)\AzureKeyVaultProvider</OutputPath>
+    <IntermediateOutputPath>$(ObjFolder)$(Configuration).$(Platform)\AzureKeyVaultProvider</IntermediateOutputPath>
+    <DocumentationFile>$(BinFolder)$(Configuration).$(Platform)\AzureKeyVaultProvider.xml</DocumentationFile>
+  </PropertyGroup>


<DocumentationFile> is set to ...\AzureKeyVaultProvider.xml, but the repo’s nuspec (tools/specs/add-ons/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.nuspec) expects the XML doc file to be named Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.xml in the artifacts folder. With the current setting, NuGet packing will fail or omit XML docs.

Consider setting DocumentationFile to use $(AssemblyName).xml (or the fully-qualified expected filename) so it matches the nuspec expectations.

Copilot · 2026-01-27T20:12:13Z

build.proj

+  <Target Name="BuildTestsNetCore">
+    <Message Text=">>> No op. Will be removed." />
+  </Target>
+  <Target Name="BuildTestsNetFx">
+    <Message Text=">>> No op. Will be removed." />
+  </Target>


BuildTestsNetCore / BuildTestsNetFx are now no-ops, but the repo’s pipeline templates invoke these targets to build tests (e.g. eng/pipelines/common/templates/steps/build-all-tests-step.yml and build-and-run-tests-netcore-step.yml). This will break CI because subsequent test runs use --no-build and will expect outputs from these targets.

Either restore these targets to build the relevant test projects (and AKV provider, if needed), or update the pipeline templates + test invocations to build during dotnet test and stop calling these targets.

benrr101 added 12 commits January 15, 2026 17:29

Rename manual test project csproj

336ef5a

Start cleaning up the ManualTests project file - introduce OS constan…

fc241f3

…ts, rework test set parameter, group the easy files to group.

Windows only files for AE tests

c73ec06

More conditionally included files

d320804

fix casing on DistributedTransactionTest

94dec48

Rest of the conditional files.

8ccddd5

References and such

61eb660

Comments from *last* PR

15e61d9

Point AKV Provider at the common MDS project

4ac324f

Fix tests that had names that conflict with helper methods. Not sure …

71d2c73

…why this is just now a problem, but ok, I'll fix it.

Finish work on the Manual tests csproj

6a52161

Wire up manual test targets as per last two projects

94d21f7

Copilot AI review requested due to automatic review settings January 27, 2026 00:40

Copilot started reviewing on behalf of benrr101 January 27, 2026 00:41 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

benrr101 added 3 commits January 27, 2026 13:15

Remove unnecesary projects from build.proj, copilot comments on SqlSe…

1734137

…rverTypesTest

Replace $(AddonName) with AzureKeyVaultProvider

b1a43d6

I just want to see if I can get the manual tests running correctly. W…

e1fd1af

…ill come back to address pipeline and build.proj issues

Copilot AI review requested due to automatic review settings January 27, 2026 20:06

Copilot started reviewing on behalf of benrr101 January 27, 2026 20:06 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

		" geometry::Parse('LINESTRING (100 100, 20 180, 180 180') AS col1, " +
		" geography::Parse('LINESTRING (-122.360 47.656, -122.343 47.656)' AS col2";

	MemoryStream buffer = new MemoryStream();
	using MemoryStream buffer = new MemoryStream();

	public static void TestUdtSqlDataReaderGetBytesSequentialAccess(CommandBehavior behavior)
	public static void TestUdtSqlDataReaderGetBytes(CommandBehavior behavior)

DRAFT | Wire Manual Tests to Common MDS #3916

Are you sure you want to change the base?

DRAFT | Wire Manual Tests to Common MDS #3916

Conversation

benrr101 commented Jan 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

benrr101 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 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 Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants