Common | Wire Manual Tests to Common MDS (part 2) #3940
Conversation
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
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_GREATERconditional compilation directive has been removed, but the file is now wrapped with#if NETwhich is broader. This means theSqlBatchCanCreateParametertest 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
@TODOcomment on line 3 indicates that the assembly name should use the full name, butManualTestsis 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
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.Debug.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DDBasics/DDDataTypesTest/DDDataTypesTest.cs
Show resolved
Hide resolved
.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DDBasics/DDDataTypesTest/DDDataTypesTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/SqlServerTypesTest.cs
Show resolved
Hide resolved
| // 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 |
There was a problem hiding this comment.
✏️ 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.
| <PropertyGroup> | ||
| <TestCommand> | ||
| $(DotnetPath)dotnet test "@(ManualTestsProj)" | ||
| --no-build |
There was a problem hiding this comment.
If we remove this, make sure to update the buildguide to remove the test restore/compile steps.
…set is only set if provided.
…pendency for manual tests.
benrr101
force-pushed
the
dev/russellben/common/manual-tests-part2
branch
from
bb6c937 to
e2aa797
Compare
.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj
Show resolved
Hide resolved
benrr101
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests.
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Looks great! Only one minor fix.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj
Show resolved
Hide resolved
| <!-- Convert more than one whitespace character into one space --> | ||
| <TestCommand>$([System.Text.RegularExpressions.Regex]::Replace($(TestCommand), "\s+", " "))</TestCommand> | ||
| </PropertyGroup> | ||
| <Message Text=">>> Running Manual test for Unix via command: $(TestCommand)" /> |
There was a problem hiding this comment.
The other test targets emit the $(TestCommand) - was this removed by accident?
There was a problem hiding this comment.
Yep, must've been an accident somewhere. It should be added back now
cheenamalhotra
left a comment
cheenamalhotra
left a comment
There was a problem hiding this comment.
Ready to check in after you've resolved conflicts :)
benrr101
dismissed stale reviews from cheenamalhotra and mdaigle
via
6ba2179
| steps: | ||
| - task: MSBuild@1 | ||
| displayName: 'Build AKV Provider .NET Framework' | ||
| inputs: | ||
| solution: build.proj | ||
| msbuildArchitecture: x64 | ||
| msbuildArguments: >- | ||
| -p:Configuration=Release | ||
| -t:BuildAKVNetFx | ||
| -p:ReferenceType=Package | ||
| -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }} | ||
| -p:MdsPackageVersion=${{ parameters.mdsPackageVersion }} | ||
|
|
||
| - task: MSBuild@1 | ||
| displayName: 'MSBuild Build Tests for ${{parameters.TargetNetFxVersion }}' | ||
| inputs: | ||
| solution: build.proj | ||
| msbuildArguments: >- | ||
| -t:BuildTestsNetFx | ||
| -p:ReferenceType=Package | ||
| -p:AbstractionsPackageVersion=${{ parameters.abstractionsPackageVersion }} | ||
| -p:MdsPackageVersion=${{ parameters.mdsPackageVersion }} | ||
| -p:TargetNetFxVersion=${{ parameters.TargetNetFxVersion }} | ||
| -p:Configuration=Release | ||
| -p:Platform=${{ parameters.platform }} | ||
|
|
||
| # Don't run unit tests using package reference. Unit tests are only run using project reference. |
There was a problem hiding this comment.
This template no longer builds the test projects (the MSBuild build steps were removed), but the DotNetCoreCLI test invocations later still pass --no-build. If this template is used, dotnet test will fail unless something else built the projects first. Either remove --no-build or restore a build step. Also, the Manual Tests project path later references Microsoft.Data.SqlClient.ManualTesting.Tests.csproj, which is not present under src/Microsoft.Data.SqlClient/tests/ManualTests/.
| <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" /> |
There was a problem hiding this comment.
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.
| <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'" /> |
| <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" /> |
There was a problem hiding this comment.
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.
| <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'" /> |
| 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. |
There was a problem hiding this comment.
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.
5 participants
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:
As you can see, there are every reference to MDS (separate) needs to be changed. After these changes, the references look like:
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 doBuildManualTests*- became no-ops because they were built at test-time, so they were removedBuildTests*- became no-ops because they all the targets they referenced were removed, so they themselves were removedRestoreTests*- Were never being called and regardless were unnecessary due to restore occurring at test-time, so they were removedAnd if you give a build.proj a cookie, chances are a pipeline will be offended...
build-all-tests-stepbecame a no-op since build for tests now happens at test-time, so it was removedci-run-tests-jobreferencedbuild-all-tests-step, so that reference was removedbuild-and-run-tests-*-stepbuild-and-run-tests-*-stepcontained calls toBuildTests*so those steps were removedbuild-and-run-tests-*-stepcontained calls to build AKV provider before tests, this isn't necessary any longer because AKV is built as a dependency in the ManualTests projectAnd some other msc changes to clean things up:
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.