Common Project | Functional Tests #3890
Conversation
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
Pull request overview
This PR migrates the FunctionalTests project to reference the common MDS project, following the pattern established in PR #3870 for UnitTests. The changes modernize the project structure and simplify the build process.
Changes:
- Migrated FunctionalTests project to reference the unified/common MDS project with conditional logic for Package vs Project mode
- Modernized FunctionalTests.csproj by removing manual file inclusions, restructuring references into clear netfx/netcore sections, and adding comprehensive documentation comments
- Simplified build.proj by removing separate BuildFunctionalTests*/RestoreFunctionalTests* targets and updating RunFunctionalTests* targets to build the project directly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Microsoft.Data.SqlClient.UnitTests.csproj | Reordered TDS-related project references for consistency |
| SslOverTdsStreamTest.cs | Added #if NET conditional compilation and updated namespace from SNI to ManagedSni |
| Microsoft.Data.SqlClient.FunctionalTests.csproj | Complete restructure with conditional MDS reference, organized netfx/netcore sections, and removal of manual file inclusions |
| build.proj | Removed FunctionalTests build/restore targets, updated RunFunctionalTests* targets to use simplified command structure |
| MethodInfo method = stream.GetType().GetMethod("FinishHandshake", BindingFlags.Public | BindingFlags.Instance); | ||
| method.Invoke(stream, null); | ||
| } | ||
|
|
There was a problem hiding this comment.
The namespace change from 'Microsoft.Data.SqlClient.SNI.SslOverTdsStream' to 'Microsoft.Data.SqlClient.ManagedSni.SslOverTdsStream' is a significant change that reflects the common project structure. Consider adding a comment explaining this is the namespace in the common/unified MDS project to help future maintainers understand why this differs from the previous implementation.
| // Note: In earlier implementations SslOverTdsStream lived in the | |
| // 'Microsoft.Data.SqlClient.SNI' namespace. In the common/unified | |
| // Microsoft.Data.SqlClient (MDS) project, the managed SNI implementation | |
| // is under 'Microsoft.Data.SqlClient.ManagedSni', so this test | |
| // intentionally reflects on 'ManagedSni.SslOverTdsStream' instead. |
| <Target Name="BuildTestsNetCore" DependsOnTargets="RestoreTestsNetCore;BuildManualTestsNetCore"/> | ||
| <Target Name="BuildTestsNetFx" DependsOnTargets="RestoreTestsNetFx;BuildManualTestsNetFx" Condition="$(IsEnabledWindows) == 'true'"/> |
There was a problem hiding this comment.
The BuildTestsNetCore and BuildTestsNetFx targets no longer build the FunctionalTests explicitly. While this aligns with the PR's goal of letting 'dotnet test' handle the build, the target names 'BuildTestsNetCore' and 'BuildTestsNetFx' suggest they build all tests (including Functional tests), but now they only build Manual tests. Consider renaming these targets to 'BuildManualTestsNetCore' and 'BuildManualTestsNetFx' or adding a comment explaining that Functional tests are built on-demand during test execution.
| <!-- @TODO: Use full assembly name here --> | ||
| <AssemblyName>FunctionalTests</AssemblyName> |
There was a problem hiding this comment.
The TODO comment suggests the assembly name should be changed to use the full name (likely 'Microsoft.Data.SqlClient.FunctionalTests'), but the assembly name remains as 'FunctionalTests'. This inconsistency should be addressed - either update the assembly name to match the pattern used in other projects or remove the TODO if the current naming is intentional.
| <!-- @TODO: Use full assembly name here --> | |
| <AssemblyName>FunctionalTests</AssemblyName> | |
| <AssemblyName>Microsoft.Data.SqlClient.FunctionalTests</AssemblyName> |
Description
Following the pattern from the last PR (#3870), this PR migrates the FunctionalTests to reference the common MDS project. This affords us an opportunity to clean up a fair bit of the project files, so here's an overview of the changes:
Change reference in FunctionalTests from separate MDS projects to common MDS project
Clean up references section in FunctionalTests csproj as per the common project and unit test projects
Remove manual inclusion of all files in the project and use the modern, implicit inclusion mechanism. Only external files and resource files need explicit inclusion.
SslOverTdsStream tests have been #if included to only run on netcore.
Remove RestoreFunctionalTests* and BuildFunctionalTests* targets from build.proj
Change RunFunctionalTests* targets in build.proj to build the project, same procedure that enabled unit test project update to work.
Issues
N/A
Testing
Local test runs work - in IDE, same failures as always happen; in console via build.proj, everything seems to be working. CI runs will validate pipeline integration is working correctly
Guidelines
Please review the contribution guidelines before submitting a pull request: