Common MDS | Cleanup Manual Tests by benrr101 · Pull Request #3932 · dotnet/SqlClient

benrr101 · 2026-02-03T20:19:38Z

Description

Changing the manual tests project to point to the common MDS project is a bit more complex than the other test projects. Specifically, manual tests include tests for the AKV provider - which means there's an entire dependency chain that needs updating. Since AKV provider is tied into several pipelines, build.proj, etc, detangling it just for manual tests is gnarly and I'm getting lost. When I get lost, I know everyone else will get lost, and the best way to fix that is to break it up into a couple PRs of related work. So here it is, part 1.

This PR primarily focuses on cleaning up the Manual Tests project:

Rename ManualTesting.Tests project to ManualTests
Sort and organize included files
- Make sure tests are only included in test set groups, not the "common" group
Conditional files based on OS or target framework are now done via #if directives like every other project
Follow grouping conventions for package/project references as per functional/unit tests projects
Move test data files to root of project to remove need for "link" tags that mess up file locations in solution explorer
Clean up the SqlCommandCancelTest and SqlServerTypesTest files
- Main reason was to fix xUnit method name conflicts
- Kinda went cowboy coder on this one 🤠

Testing

Testing via #3916 shows these changes should work correctly. Local runs of modified tests show the tests work correctly. CI for this PR should verify

...
Now onto wiring up the entire chain....

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

Rewrite VerifyGetCommand in adapter test to no longer use reflection/dynamic invocation

Copilot

Pull request overview

Refactors and reorganizes the Manual Tests project as a first step toward wiring it to the common Microsoft.Data.SqlClient (MDS) project, including project renaming, file grouping, OS/TFM-conditional compilation via #if, and some targeted test cleanups.

Changes:

Renames the Manual Tests project (project/assembly/solution/build entry points) and restructures the .csproj to use explicit test-set groupings.
Moves/normalizes content/test data files (e.g., baseline .bsl, XML test data) to simplify inclusion and avoid linked-file path issues.
Cleans up several manual tests (UDT tests, cancel tests) and applies OS/TFM guards via preprocessor directives.

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/ManualTests/SqlParameterTest_ReleaseMode.bsl	Adds/moves baseline output used by TVP/parameter manual tests.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/SqlServerTypesTest.cs	Refactors UDT reader tests into theories and centralizes query/expected values.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.windows.cs	Wraps Windows-only distributed transaction tests in `_WINDOWS` preprocessor guard.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlDSEnumeratorTest/SqlDataSourceEnumeratorTest.windows.cs	Wraps enumerator tests in `_WINDOWS` guard and removes per-TFM platform attributes.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs	Renames tests to avoid xUnit name conflicts and modernizes some patterns (incl. async Task tests).
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.netfx.cs	Ensures netfx-only compilation via `#if NETFRAMEWORK`.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.Debug.cs	Adds license header and wraps debug-only test code in `#if DEBUG`.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/Batch/BatchTests.netcore.cs	Ensures net-only compilation via `#if NET` and adjusts conditional compilation blocks.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj	Major project cleanup: test-set grouping, OS constants, content moves, and updated references.
src/Microsoft.Data.SqlClient/tests/ManualTests/DDDataTypesTest_Data.xml	Adds XML test data at project root for easier copying/consumption.
src/Microsoft.Data.SqlClient/tests/ManualTests/DDBasics/DDDataTypesTest/DDDataTypesTest.cs	Updates XML reader to use the new root-level XML data file name.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.windows.cs	Wraps Windows-only CSP setup strategy in `_WINDOWS` guard.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/CspProviderColumnMasterKey.windows.cs	Wraps Windows-only CMK setup in `_WINDOWS` guard.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/DateOnlyReadTests.netcore.cs	Wraps .NET-only tests in `#if NET`.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/CspProviderExt.windows.cs	Moves “Windows-only” gating to `_WINDOWS` preprocessor guard.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj	Updates commentary and adjusts xunit runner json item type.
src/Microsoft.Data.SqlClient.sln	Renames the Manual Tests project entry and updates path/name.
build.proj	Updates manual test project path to the renamed `.csproj`.

Comments suppressed due to low confidence (1)

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

The netcore references section includes the same TestCommon project twice (once via $(RepoRoot)/src/... and once via $(TestsPath)...). This can lead to duplicate project references / duplicate outputs during build. Remove one reference and keep a single canonical path (preferably the $(TestsPath) one for consistency with other references in this file).

Copilot · 2026-02-03T22:03:34Z

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


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


These tests call reader.Read() without asserting the result. If the query returns no rows, the failure will be a less-informative exception later (e.g., from GetSqlBytes/GetBytes/GetStream). Consider changing this to Assert.True(reader.Read()) so the test fails with a clear message when no row is returned.

Suggested change

reader.Read();

Assert.True(reader.Read());

Copilot · 2026-02-03T22:03:35Z

build.proj

+    <ManualTests Include="**/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj" />
+    <ManualTestsProj Include="**/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj" />


The ManualTests project file was renamed, but some build/CI entry points still reference the old path src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj (e.g., eng/pipelines/common/templates/steps/build-and-run-tests-netcore-step.yml:115,137 and ...netfx-step.yml:117,139). Update those to Microsoft.Data.SqlClient.ManualTests.csproj to avoid CI failures.

Copilot · 2026-02-03T22:03:35Z

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


                    cmd2.Parameters.Add("@x", SqlDbType.Xml);
-                    XmlReader xr = XmlReader.Create("data.xml");
+                    XmlReader xr = XmlReader.Create("DDDataTypesTest_Data.xml");


Disposable 'XmlReader' is created but not disposed.

Copilot · 2026-02-03T22:03:35Z

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();

edwardneal

No major comments - just a couple of thoughts on SqlCommandCancelTest, and some ideas to simplify the testing in the future.

edwardneal · 2026-02-03T21:37:55Z

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

        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
-        public static void PlainCancelTest()
-        {
+        public static void PlainCancelTest_Tcp() =>
            PlainCancel(tcp_connStr);
-        }

        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))]
        [PlatformSpecific(TestPlatforms.Windows)]
-        public static void PlainCancelTestNP()
-        {
+        public static void PlainCancelTest_NamedPipe() =>
            PlainCancel(np_connStr);
-        }


This is a persistent pattern - we have tests for TCP (which don't apply to Azure Synapse) and named pipes (which don't apply to Azure SQL). Can we lift that logic out into a TheoryData and eliminate half of the test methods?

I wanted to, and then I put it back after I realized they were different conditions and one is platform specific. I think our conditional test ideas will change soon enough, and I didn't want to invest any more in inventing a new attribute for this PR.

edwardneal · 2026-02-03T21:39:25Z

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

-        }

        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))]
        [PlatformSpecific(TestPlatforms.Windows)]


Are the named pipe tests really only applicable to Windows?

Yep - named pipes are not supported on SQL Server on Linux https://learn.microsoft.com/en-us/sql/linux/sql-server-linux-management-overview?view=sql-server-ver17

Thanks - no problem here then.

edwardneal · 2026-02-03T21:53:53Z

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

+    <!-- This is only required today because of the separate test sets -->
+    <EnableDefaultCompileItems>false</EnableDefaultCompileItems>


Do you have any thoughts on replacing this notion of a per-compilation $(TestSet) property with an XUnit trait and corresponding filters in the pipeline? It may be clearer to leave the code files as-is, (since most of their conditional inclusions would be removed anyway) and to start applying attributes instead. It also lays the groundwork to reduce the number of times we need to compile the same project.

Yes. Personally I'd like to get rid of the test set ideas altogether, but not sure how to do that just yet. This PR is just work to cleanup the manual tests project in preparation for wiring it up to the common MDS project.

edwardneal · 2026-02-03T22:13:20Z

...Client/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.windows.cs

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.using System;

+#if _WINDOWS


There's no actionable comment here, but: the more I see this, the less I like it. When combined with the four test set combinations and the four target-specific combinations, we're compiling the same project dozens of times. It seems far more natural to only have target-specific combinations, compiled before any test stages start.

Is there a pipeline-side reason why we need this, or is this solely because the core library is compiled on a per-OS basis?

Yeah, it's not good. But we don't have a solution yet today. This will be post-common project work.

This only exists today because of the per-OS compilation of MDS. The tests reference classes that don't exist outside of a windows build.

Sounds good. Once the common project and pipelines have settled, #3844 makes a start on making the MDS build OS-independent. After the process started there has completed, hopefully this'll shake out...

Copilot

Pull request overview

Copilot reviewed 19 out of 24 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

Header comment line appears to have an accidental using System; appended (...more information.using System;). This is likely a copy/paste artifact and makes the license header look malformed; it can just end at the period since a real using System; already follows.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:35
ValidateOs compares $(TargetOS) to $(OS), but this project only defines TargetOs. If $(TargetOS) is unset (likely), this condition will always be true and the build will error on every OS. Use $(TargetOs) (or $(NormalizedTargetOs) with a normalized comparison) in the condition so the target only fails when the dev override is left on.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj:84
ManualTests no longer includes/copies xunit.runner.json into the test output. Other test projects in this repo include it (e.g., tests/Common/Common.csproj:43-46, tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj:22-25) to control runner behavior (shadow copy, diagnostics, parallelization). Consider adding it back here to keep ManualTests consistent and avoid behavior changes on netfx.

Copilot · 2026-02-04T20:57:00Z

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

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


reader.Read() return value is ignored here. If the query returns no rows, subsequent GetBytes calls will throw. Use Assert.True(reader.Read()) to fail with a clearer assertion.

Suggested change

reader.Read();

Assert.True(reader.Read());

Copilot · 2026-02-04T20:57:00Z

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

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


reader.Read() return value is ignored here. If the query returns no rows, subsequent GetStream calls will throw. Use Assert.True(reader.Read()) to fail with a clearer assertion.

Suggested change

reader.Read();

Assert.True(reader.Read());

Copilot · 2026-02-04T20:57:00Z

src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj


+    <!-- Used by SqlMetaDataFactory to construct its DataSet -->
+    <EmbeddedResource Include="Resources\Microsoft.Data.SqlClient.SqlMetaData.xml">
+      <Link>Resources\Microsoft.Data.SqlCLient.SqlMetaData.xml</Link>


The <Link> path contains a casing typo (SqlCLient). While this doesn’t affect the embedded resource name (since LogicalName is set), it makes the project view inconsistent/confusing. Consider renaming the link to Resources\Microsoft.Data.SqlClient.SqlMetaData.xml for consistency.

Suggested change

<Link>Resources\Microsoft.Data.SqlCLient.SqlMetaData.xml</Link>

<Link>Resources\Microsoft.Data.SqlClient.SqlMetaData.xml</Link>

Copilot · 2026-02-04T20:57:01Z

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

+                    XmlReader xr = XmlReader.Create("DDDataTypesTest_Data.xml");
                    cmd2.Parameters[0].Value = new SqlXml(xr);
                    cmd2.ExecuteNonQuery();


Disposable 'XmlReader' is created but not disposed.

Suggested change

XmlReader xr = XmlReader.Create("DDDataTypesTest_Data.xml");

cmd2.Parameters[0].Value = new SqlXml(xr);

cmd2.ExecuteNonQuery();

using (XmlReader xr = XmlReader.Create("DDDataTypesTest_Data.xml"))

{

cmd2.Parameters[0].Value = new SqlXml(xr);

cmd2.ExecuteNonQuery();

}

codecov · 2026-02-04T22:35:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.26%. Comparing base (a0357b2) to head (a1f0218).

❗ There is a different number of reports uploaded between BASE (a0357b2) and HEAD (a1f0218). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (a0357b2) HEAD (a1f0218)

addons 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3932       +/-   ##
===========================================
- Coverage   90.82%   67.26%   -23.56%     
===========================================
  Files           6      263      +257     
  Lines         316    66198    +65882     
===========================================
+ Hits          287    44530    +44243     
- Misses         29    21668    +21639

Flag	Coverage Δ
addons	`?`
netcore	`67.51% <ø> (?)`
netfx	`66.23% <ø> (?)`

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.

benrr101 added 5 commits February 3, 2026 13:16

Rename manual test project csproj

edcdf90

Cleanup ManualTests project file, make conditional files conditional

213cf19

Comments from *last* PR

721e97f

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

0f88fb5

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

Move test data files, fix references

18fe343

benrr101 requested a review from a team as a code owner February 3, 2026 20:19

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

Can't reference the new test common project yet...

e5663e2

Copilot AI review requested due to automatic review settings February 3, 2026 21:56

Copilot started reviewing on behalf of benrr101 February 3, 2026 21:57 View session

SqlMetaData resolution in common MDS project

a97cd89

Rewrite VerifyGetCommand in adapter test to no longer use reflection/dynamic invocation

Copilot AI reviewed Feb 3, 2026

View reviewed changes

edwardneal reviewed Feb 3, 2026

View reviewed changes

priyankatiwari08 self-assigned this Feb 4, 2026

Fix test Common project references in ManualTests project

a1f0218

Copilot AI review requested due to automatic review settings February 4, 2026 20:50

Copilot started reviewing on behalf of benrr101 February 4, 2026 20:51 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

		<ManualTests Include="**/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj" />
		<ManualTestsProj Include="**/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj" />

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

		<!-- This is only required today because of the separate test sets -->
		<EnableDefaultCompileItems>false</EnableDefaultCompileItems>

	<Link>Resources\Microsoft.Data.SqlCLient.SqlMetaData.xml</Link>
	<Link>Resources\Microsoft.Data.SqlClient.SqlMetaData.xml</Link>

Conversation

benrr101 commented Feb 3, 2026

Description

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

edwardneal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 4, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants