-
Notifications
You must be signed in to change notification settings - Fork 319
Move ActiveDirectoryAuthenticationProvider Tests #3717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/azure-split
Are you sure you want to change the base?
Conversation
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary for reviewers.
| #pragma warning disable 0618 // Type or member is obsolete | ||
| [InlineData(SqlAuthenticationMethod.ActiveDirectoryPassword)] | ||
| #pragma warning restore 0618 // Type or member is obsolete | ||
| [InlineData(SqlAuthenticationMethod.ActiveDirectoryInteractive)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interactive was missing from the original test, so I added it here.
| namespace Microsoft.Data.SqlClient.ManualTesting.Tests | ||
| { | ||
| public class AADConnectionsTest | ||
| public class AADConnectionTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test class name didn't match the filename.
| // Clear token cache for code coverage. | ||
| ActiveDirectoryAuthenticationProvider.ClearUserTokenCache(); | ||
| using (SqlConnection connection = new SqlConnection(DataTestUtility.AADPasswordConnectionString)) | ||
| #pragma warning disable 0618 // Type or member is obsolete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests were unnecessarily using our auth provider, so I updated them to use the UsernamePasswordProvider, which fetches real tokens via MSAL itself.
1903ecc to
b17155b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/azure-split #3717 +/- ##
===================================================
Coverage ? 76.45%
===================================================
Files ? 271
Lines ? 43029
Branches ? 0
===================================================
Hits ? 32896
Misses ? 10133
Partials ? 0
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary for reviewers.
| public class AADConnectionsTest | ||
| public class AADConnectionTest | ||
| { | ||
| class CustomSqlAuthenticationProvider : SqlAuthenticationProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to UsernamePasswordProvider.cs so it could be shared.
| --verbosity ${{ parameters.verbosity }} | ||
| --verbosity ${{ parameters.dotnetVerbosity }} | ||
| -p:ReferenceType=${{ parameters.referenceType }} | ||
| -p:ForceMdsAssemblyNameSuffix=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is explained in my comments on Directory.Build.props.
| # to stress test. | ||
| - name: pipelineArtifactName | ||
| displayName: Pipeline Artifact Name | ||
| # The verbosity level for the dotnet CLI commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted parameters alphabetically and added some debug/verbosity.
| Apply an MDS assembly name suffix, if necessary. See the top-level | ||
| Directory.Build.props for more information. | ||
| --> | ||
| <DefineConstants Condition="'$(ApplyMdsAssemblyNameSuffix)' == 'true'">$(DefineConstants);APPLY_MDS_ASSEMBLY_NAME_SUFFIX</DefineConstants> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstractions performs reflection against the MDS assembly, so it needs to know how to form the assembly name. Explained in Directory.Build.props.
| Append an assembly name suffix, if necessary. See the top-level | ||
| Directory.Build.props for more information. | ||
| --> | ||
| <AssemblyName>Microsoft.Data.SqlClient</AssemblyName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 ref projects didn't previously specify <AssemblyName>. Not sure why, or if this new addition will cause problems. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assembly attributes aren't well documented, but it seems like this just overrides the default (the name of the project file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented here:
https://learn.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-properties
That page doesn't explicitly say that the assembly name will be chosen from the project name if not specified in an <AssemblyName> property, but you're correct that does seem to be the behaviour.
I think it's best to explicitly set <AssemblyName> to avoid surprises.
| <ProjectReference | ||
| Condition="'$(TargetGroup)'=='netfx'" | ||
| Include="$(NetFxSource)src\Microsoft.Data.SqlClient.csproj" /> | ||
| <ProjectReference Include="../../../Microsoft.Data.SqlClient.Extensions/Azure/src/Azure.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that none of the Functional (or Manual) tests are depending on our auth provider by accident.
| // | ||
| // TODO: Figure out which ones and install on-demand rather than | ||
| // globally. | ||
| SqlAuthenticationProvider.SetProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire test suite is now serviced either by this provider, or by the UsernamePasswordProvider on-demand.
No other auth methods have a provider installed.
| } | ||
| finally | ||
| { | ||
| if (original is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason there is no unset-provider mechanism, so we're polluting the global scope in the event that no provider was registered before this test.
You will see this pattern elsewhere as well. SetProvider() throws an NRE if you pass null.
| <group targetFramework="net462"> | ||
| <reference file="Microsoft.Data.SqlClient.dll" /> | ||
| <reference file="Microsoft.Data.SqlClient.xml" /> | ||
| <reference file="Microsoft.Data.SqlClient$AssemblyNameSuffixNetFx$.dll" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our project-reference based builds probably shouldn't be attempting to generate NuGet packages, but since they do, I made it work. The generated packages aren't used for anything (see CI-SqlClient pipeline), so there is no harm here.
As mentioned in the Directory.Build.props commentary, this is all temporary code anyway and should disappear before 7.0.0 GA.
| project to determine whether or not to apply the MDS assembly name suffix. | ||
| --> | ||
| <ApplyMdsAssemblyNameSuffix>false</ApplyMdsAssemblyNameSuffix> | ||
| <ApplyMdsAssemblyNameSuffix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary measure to get the Abstractions and Azure projects (and their tests) to build without having to employ the confusing tricks found in the FunctionalTests, ManualTests, and AKV projects, where we only support building for one type of framework at a time (ick!). This will all disappear when we finally complete the .NET and .NET Framework codebase merge, and we have a single MDS project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single assembly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that part since it was confusing. We will produce the same number of MDS assembly files (DLLs), but will no longer be prevented from building them all at once via project references due to multiple projects specifying the same assembly name.
5d4f942 to
1e27ff4
Compare
1e27ff4 to
21ab32e
Compare
648d928 to
540c2c6
Compare
Abstractions Package - Pipeline Changes (#3628) Move AAD/Entra Authentication into new Azure package (#3680) User Story 39839: Move existing MDS tests to Azure package - Identified and moved tests specific to ActiveDirectoryAuthenticationProvider into the Azure Test project. - Updated some MDS tests that unnecessarily used ActiveDirectoryAuthenticationProvider. - Added tests to Abstractions Test project to ensure AAD/Entra auth fails when the Azure package isn't present. - Various build and pipeline changes to support the test changes. - Fixed project-based builds so we can restore MDS netcore and netfx projects at the same time. - Added caching to the managed identity provider for the ManualTest project. - Moved username/password Entra auth provider into its own file for sharing across tests. - Added ADO service connection for test tasks that require access to Azure resources. - Added workload identity federation tests. - Solved the mystery of SNI DLLs missing for .NET Framework project-reference-based builds. - Added ADO-CI-1ES pool jobs to test the Azure package on Linux and Windows. - Fixed Azure package pipelines to support pools in the ADO.Net and Public projects. - Added some new pipeline variables to handle the Workload Identity Federation test.
21ab32e to
6f7f621
Compare
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary and things for myself to fix.
src/Microsoft.Data.SqlClient.Extensions/Azure/test/DefaultAuthProviderTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Azure/test/WorkloadIdentityFederationTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Azure/test/WorkloadIdentityFederationTests.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/xunit.runner.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates ActiveDirectoryAuthenticationProvider (ADAP) tests from the Microsoft.Data.SqlClient (MDS) test suite to the Azure package. The migration includes infrastructure changes to support project-reference builds with different assembly names for .NET Core and .NET Framework, along with new test utilities and comprehensive pipeline updates.
Key Changes:
- Build infrastructure updated to handle assembly name suffixes for project-reference builds
- ActiveDirectoryAuthenticationProvider tests moved from MDS ManualTests to Azure package tests
- New test provider implementations (UsernamePasswordProvider, ManagedIdentityProvider) added to MDS tests
- Azure package tests now have comprehensive configuration and pipeline support
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/targets/GenerateMdsPackage.targets | Added assembly name suffix properties for project builds |
| tools/targets/CopySniDllsForNetFxProjectReferenceBuilds.targets | New target file to copy SNI DLLs for .NET Framework project builds |
| tools/specs/Microsoft.Data.SqlClient.nuspec | Updated to support optional assembly name suffixes in file paths |
| src/Microsoft.SqlServer.Server/StringsHelper.cs | Added braces for single-line if statements (code style fix) |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs | Removed ADAP-dependent tests, added test provider usage for remaining tests |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs | New test provider for username/password authentication |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ManagedIdentityProvider.cs | New test provider for managed identity authentication |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/AADAuthenticationTests.cs | Removed ADAP constructor tests (moved to Azure package) |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlAuthenticationProviderTest.cs | Deleted - tests moved to Azure package |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs | New file with migrated ADAP connection tests |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADAuthenticationTests.cs | New file with migrated ADAP provider constructor tests |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/DefaultAuthProviderTests.cs | New test verifying ADAP is installed for all AAD methods |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/Config.cs | New configuration class for Azure package tests |
| src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProviderInternal.cs | Enhanced to support assembly name suffixes and improved logging |
| src/Directory.Build.props | Added ApplyMdsAssemblyNameSuffix property logic |
| eng/pipelines/* | Multiple pipeline files updated for consistent parameter naming and new test infrastructure |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Abstractions/test/SqlAuthenticationProviderTest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Outdated
Show resolved
Hide resolved
| Append an assembly name suffix, if necessary. See the top-level | ||
| Directory.Build.props for more information. | ||
| --> | ||
| <AssemblyName>Microsoft.Data.SqlClient</AssemblyName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assembly attributes aren't well documented, but it seems like this just overrides the default (the name of the project file).
| // Overridden by app.config in this project | ||
| [ConditionalTheory(typeof(TestUtility), nameof(TestUtility.IsNetFramework))] | ||
| [InlineData(SqlAuthenticationMethod.ActiveDirectoryInteractive)] | ||
| public void DefaultAuthenticationProviders_Interactive(SqlAuthenticationMethod method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test got dropped. It's covering the behavior of providing an auth provider via app.config and should be preserved as we recently had a regression related to this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking for this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a new file and test to cover it here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I saw it deleted in a subsequent commit, but that must have been the original file.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ManagedIdentityProvider.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Show resolved
Hide resolved
540c2c6 to
da3b96a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 10 comments.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProviderInternal.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProviderInternal.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
- Removed unnecessary package ref to Microsoft.Win32.Registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 3 comments.
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/SqlAuthenticationProviderInternal.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
| <PackageReference Include="Microsoft.DotNet.XUnitExtensions" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" /> | ||
| <ContentWithTargetPath Include="..\tools\Microsoft.Data.SqlClient.TestUtilities\xunit.runner.json"> | ||
| <Content Include="..\tools\Microsoft.Data.SqlClient.TestUtilities\xunit.runner.json"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContentWithTarget isn't meant to be called directly. The Content task is the public API.
| "parallelizeAssembly": true, | ||
| "shadowCopy": false, | ||
|
|
||
| "_v3_culture": "invariant", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the existing options are only used by v3 (ignored by v2), and I didn't want to lose them completely by removing them (since we don't use v3 yet). So I made it obvious they are v3 only, and we can simply remove the prefix if/when we move to xUnit v3.
| { | ||
| StringsHelper sys = GetLoader(); | ||
| if (sys == null) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't resist adding the { } blocks - sorry!
…ffs. - Added .NET 10 Runtime to the Abstractions and Azure package tests. - Added the feat/* branches to CodeQL config. - Found a few leftover relative paths for templates and made them absolute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 59 out of 60 changed files in this pull request and generated 1 comment.
mdaigle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one request remaining regarding the missing test.
| // Overridden by app.config in this project | ||
| [ConditionalTheory(typeof(TestUtility), nameof(TestUtility.IsNetFramework))] | ||
| [InlineData(SqlAuthenticationMethod.ActiveDirectoryInteractive)] | ||
| public void DefaultAuthenticationProviders_Interactive(SqlAuthenticationMethod method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking for this test.
Description
This PR moves all existing MDS tests that use the ActiveDirectoryAuthenticationProvider (ADAP) into the Azure project. The tests are moved as-is with non-functional changes to get them to build/run.
A few MDS tests were unnecessarily using ADAP when they should have been using a test provider, so I updated them to do so.
A full suite of tests for the Azure package will be created in future PRs.
Testing
Existing CI for the Azure and MDS packages should confirm everything.