Common Project | Unit Tests

[benrr101]

Description

This PR updates the Unit Test project to point at the common (merged) MDS project. There's some unconventional changes to make this work, though.

• Unit Test project now references the common MDS csproj
• Common MDS project updated to include strings/resx files
• Common MDS project updated to have InternalsVisibleTo attribute
• Unit Test project package references updated to match the common project format
• Fixed SNI dll issues in Unit Test project by replacing copy targets with a package reference to the SNI package
  • This might not be the "right" solution (ie, it's so simple, idk why we wouldn't have tried it already)
  • I posit it's valid b/c MDS has a package reference to SNI, the version number is locked in via the central feed stuff
  • I took this approach b/c the copy targets had the build output hard-coded to include the OS in the path. The copy targets only applied to netfx, but I still needed to copy them in for netcore. For netcore, the OS will vary, and I really did not want to bring the same song and dance for OS variables into the test projects.
• Temporarily introduce a new Test Common project that the Unit Test project references
  • This is the most controversial change, but I stress that it's temporary
  • The Test Common project maintains a reference to the MDS project, and all the test projects reference it.
  • We can't point Unit Test project at common MDS project without the Test Common project also pointin at common MDS project
  • We can't point the existing Test Common project at the common MDS project without breaking Functional/Manual tests
  • Thus, we either need to update all the test projects at once (big change, dangerous) or make a parallel version of the Test Common project.
  • I have chosen the latter option.
  • The plan for unwinding this is once the functional/manual test projects reference the common MDS project, they will also point to the parallel Test Common project. Once that happens, the existing test common can be deleted.

How does this impact our pipelines?

No changes were necessary to the CI/PR pipelines. The existing build.proj command for running the Unit Tests continues to work even after changing the MDS target. A small tweak was necessary to ensure that we weren't building unnecessary projects before running the tests, but that's no big issue, afaik. Pipelines should continue to Just Work™️

I considered making new targets in the build2.proj but decided to just get this out first. There's some cruft in the way we call the Unit Tests today, so leaving it as-is isn't making anything worse.

Issues

N/A

Testing

Locally the build.proj targets and running the unit tests inside the IDE still work.

[Copilot]

Pull request overview

This PR migrates the Unit Test project to reference the common (merged) Microsoft.Data.SqlClient project instead of the separate platform-specific projects (netfx/netcore). The changes introduce a temporary parallel Test Common project to avoid breaking existing Functional and Manual test projects during the migration.

Key Changes:

• Updated Unit Test project to reference the unified MDS project and resolve SNI dependencies via package references
• Added InternalsVisibleTo attribute and resource file configurations to the common MDS project
• Created a temporary Microsoft.Data.SqlClient.TestCommon project that references the unified MDS project
• Refactored test code to improve clarity and remove unnecessary try-catch blocks

Reviewed changes

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

Show a summary per file

File	Description
build.proj	Removed redundant project references from UnitTests ItemGroup since the dependencies are now managed by the test project itself
src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj	Added InternalsVisibleTo for UnitTests assembly and configured embedded resource handling for Strings.resx files
src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj	New temporary test common project that references the unified MDS project with necessary test dependencies
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj	Restructured to reference the unified MDS project, use SNI package references instead of copy targets, and separate dependencies by target framework
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs	Improved test code clarity by extracting actions and removing unnecessary try-catch blocks
src/Microsoft.Data.SqlClient.sln	Added the new Microsoft.Data.SqlClient.TestCommon project to the solution with build configurations

[Copilot]

The package reference duplicates are being added across both conditional ItemGroups (netframework and netcore). Consider consolidating common package references (Microsoft.DotNet.XUnitExtensions, Microsoft.NET.Test.Sdk, Microsoft.SqlServer.Server, System.Configuration.ConfigurationManager, xunit, xunit.runner.console, xunit.runner.visualstudio) into a shared ItemGroup to reduce duplication and improve maintainability.

[benrr101]

"improve maintainability" is arguable, hence why I did it in the first place

[Copilot]

The TDS-related project references (TDS.Servers, TDS.EndPoint, TDS) are duplicated in both the net462 and netcore conditional ItemGroups. These common test dependencies should be moved to a shared ItemGroup to reduce duplication.

[cheenamalhotra]

Move these to props file.

[benrr101]

I disagree, at this point I want to reduce the usage of props files - they are difficult to discover and unless we have several projects using them, it's not beneficial to put things in a props file. The TDS projects are only being referenced by the unit test project, so moving those to a props file specifically isn't beneficial.

[Copilot]

Similarly, the package references to Azure.Identity, Azure.Security.KeyVault.Keys, Microsoft.DotNet.XUnitExtensions, and xunit are duplicated in both conditional ItemGroups. Consider consolidating these common dependencies into a shared ItemGroup to improve maintainability.

Suggested change

	<!-- References for netfx -->
	<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
	<PackageReference Include="Azure.Identity" />
	<PackageReference Include="Azure.Security.KeyVault.Keys" />
	<PackageReference Include="Microsoft.DotNet.XUnitExtensions" />
	<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies"
	IncludeAssets="runtime; build; native; contentfiles; analyzers; buildtransitive"
	PrivateAssets="all" />
	<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" />
	<PackageReference Include="xunit" />
	</ItemGroup>
	<!-- References for netcore -->
	<ItemGroup Condition="'$(TargetFramework)' != 'net462'">
	<PackageReference Include="Azure.Identity" />
	<PackageReference Include="Azure.Security.KeyVault.Keys" />
	<PackageReference Include="Microsoft.DotNet.XUnitExtensions" />
	<PackageReference Include="xunit" />
	</ItemGroup>
	<!-- Common test package references -->
	<ItemGroup>
	<PackageReference Include="Azure.Identity" />
	<PackageReference Include="Azure.Security.KeyVault.Keys" />
	<PackageReference Include="Microsoft.DotNet.XUnitExtensions" />
	<PackageReference Include="xunit" />
	</ItemGroup>
	<!-- References for netfx -->
	<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
	<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies"
	IncludeAssets="runtime; build; native; contentfiles; analyzers; buildtransitive"
	PrivateAssets="all" />
	<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" />
	</ItemGroup>

[Copilot]

An extra blank line has been added here which is unnecessary. Consider removing it for cleaner code formatting.

Suggested change

[Copilot]

The TODO comment indicates that the AssemblyName should be changed to use the full assembly name (likely "Microsoft.Data.SqlClient.UnitTests") to match project naming conventions. This should be addressed before merging to ensure consistency with other projects in the solution.

Suggested change

	<!-- @TODO: Use full assembly name here -->
	<AssemblyName>UnitTests</AssemblyName>
	<AssemblyName>Microsoft.Data.SqlClient.UnitTests</AssemblyName>

[benrr101]

The TODO Is here for later. Not today.

[cheenamalhotra]

MsBuild system prefers props files more than empty shell projects to architect shared references, and common properties. Props files can be built with the same XML schema so your code wouldn't change much.

You can convert the TestCommon project into a .props file that every test project includes - and make sure it contains shared references, such that project files can stay lightweight and all dependencies/common properties can be pulled into the props file.

[benrr101]

It's not an empty shell project, it is the same as the existing Common project, just with a reference to the common MDS project. The files compiled files are included by default as is the norm in modern csproj files.

[cheenamalhotra]

We need this reference only for Project Reference testing and only for .NET Framework targets, and not for package reference testing or .NET targets as it should flow as a transitive dependency in all other cases.

[cheenamalhotra]

Make sure you include the dependency version in Directory.Package.Props

[benrr101]

Well the good news then is that the Unit Test only runs in project reference mode. And the SNI is already in Directory.Package.props because MDS takes a dependency on it.

[cheenamalhotra]

Should not be referenced, allow transitive dependency to include automatically.

[benrr101]

Doesn't work, though. Does work when referenced.

[cheenamalhotra]

Move all repetitive dependencies to the base props file (test common as of now), and include the props file directly.

[benrr101]

As described above, TestCommon is not an empty proj just being used for props.

[cheenamalhotra]

Why was the try-catch removed?

[paulmedynski]

If Open() throws, it will fail the test anyway. The xUnit diagnostics will look a bit different, but the file/line numbers will be evident.

[benrr101]

Assert.Fail will only report whatever the message is, in this case a little message and the message property of the exception. Letting the uncaught exception will fail the test and emit the entire exception - message, stack trace, inner exceptions (if any). Not only that, but removing the try/catch cleans it up quite a bit.

Was this necessary for this PR? No, but this test was failing in my local testing for the branches, and to debug it better I removed the try/catch as per the reasons above. It's a small change, so I left it in this PR.

[mdaigle]

I had to include these so that the referenced projects are built and available for the proper target when we build the unit tests project. I'd be happy to get rid of them, but try building unit tests after cleaning the project to make sure everything still works.

[benrr101]

Yes, this is something I'm working through right now in the failing build. I ignored the "--no-build" on the dotnet command and it tripped me up.

[Copilot]

Pull request overview

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

[Copilot]

The new Microsoft.Data.SqlClient.TestCommon project doesn't include any compile files. Looking at the existing Common.csproj in the same directory, it contains files like DisposableArray.cs, LocalAppContextSwitchesHelper.cs, and SqlDataReaderExtensions.cs. These files should be included in the new TestCommon project either through explicit Compile elements or by ensuring EnableDefaultCompileItems is true (which is the default for SDK-style projects). Without including these source files, the TestCommon project will be empty and won't provide the shared test utilities that other test projects depend on.

[benrr101]

No they should not be included - by default all files in the folder are compiled, there's no need to manually include them.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

The TDS project references (TDS.Servers, TDS.EndPoint, TDS) are duplicated in both the net462 and netcore conditional ItemGroups. These references should be moved to a common ItemGroup that applies to all target frameworks to avoid duplication and simplify maintenance.

[Copilot]

The InternalsVisibleTo attribute is only added for non-official builds (when CDP_BUILD_TYPE != 'Official'). This means unit tests won't be able to access internal members in official builds, which could cause test failures in production build pipelines. Consider whether this restriction is intentional or if the tests should always have access to internals.