Tests | Cleanup AAD Password auth from tests + code cleanup#4390
Draft
cheenamalhotra wants to merge 6 commits into
Draft
Tests | Cleanup AAD Password auth from tests + code cleanup#4390cheenamalhotra wants to merge 6 commits into
cheenamalhotra wants to merge 6 commits into
Conversation
cheenamalhotra
added
the
Area\Tests
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the Entra ID “password auth” connection-string configuration from the test suite (and CI templates) following the pipeline change in #4288, replacing it with a single “bare” AzureSqlConnectionString that tests extend at runtime to exercise specific authentication modes.
Changes:
- Renamed
AADPasswordConnectionString→AzureSqlConnectionStringacross test config (Config.cs/config.default.jsonc), docs, and Azure DevOps pipeline templates; updated pipeline variables accordingly. - Added shared test helpers under
tests/Common/(CommonUtils,StringExtensions) to compose auth/credential keywords onto a base connection string. - Updated ManualTests + Azure extensions tests to remove
ActiveDirectoryPasswordcoverage and to use managed identity / token-based flows with the new helpers.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TESTGUIDE.md | Updates documented test config property name/meaning for AAD/Azure SQL tests. |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Microsoft.Data.SqlClient.TestUtilities.csproj | References the new shared test helper project. |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/config.default.jsonc | Renames config field and clarifies Azure SQL base conn string expectation (no creds). |
| src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs | Renames the config field to AzureSqlConnectionString. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs | Removes AAD password tests and rewrites AAD scenarios to use helpers and AzureSqlConnectionString. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs | Updates pooled AAD/token tests to use AzureSqlConnectionString and async token retrieval. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AADFedAuthTokenRefreshTest/AADFedAuthTokenRefreshTest.cs | Switches the fed-auth refresh scenario to managed identity-based auth/provider usage. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/ManagedIdentityProvider.cs | Renames the custom provider class to clarify it targets user-assigned MI. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs | Renames config fields, adds MI-based token retrieval APIs, and removes AAD password token generation. |
| src/Microsoft.Data.SqlClient/tests/Common/StringExtensions.cs | Adds fluent helpers for composing auth/credential keywords and for removing auth/credential keywords. |
| src/Microsoft.Data.SqlClient/tests/Common/CommonUtils.cs | Adds shared random/name helpers used by updated tests. |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/Config.cs | Renames the Azure test config field read from config.jsonc. |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs | Updates Azure extension tests to build auth modes from the base Azure SQL conn string. |
| eng/pipelines/jobs/test-azure-package-ci-job.yml | Switches pipeline config-writing to AzureSqlConnectionString and updates secret scoping. |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Switches CI template config-writing to AzureSqlConnectionString (WestUS2/EastUS). |
| eng/pipelines/common/templates/steps/update-config-file-step.yml | Renames template parameter and output property to AzureSqlConnectionString. |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Renames config pass-through property to AzureSqlConnectionString. |
| .github/instructions/testing.instructions.md | Updates the test-config docs to match the renamed Azure SQL config property. |
Comment on lines
41
to
43
| if (object.ReferenceEquals(resourceMan, null)) { | ||
| global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("Microsoft.Data.SqlClient.Resources.Strings", typeof(Strings).Assembly); | ||
| global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("Resources.Strings", typeof(Strings).Assembly); | ||
| resourceMan = temp; |
Comment on lines
+32
to
+35
| private static bool IsAccessTokenSetup() => DataTestUtility.IsAccessTokenAsyncSetup().GetAwaiter().GetResult(); | ||
| private static bool IsAzureSqlConnStringSetup() => DataTestUtility.IsAzureConnStringSetup() && DataTestUtility.IsUserManagedIdentitySupported; | ||
| private static bool IsManagedIdentitySetup() => DataTestUtility.IsUserManagedIdentitySupported; | ||
| private static bool SupportsSystemAssignedManagedIdentity() => DataTestUtility.IsSystemManagedIdentitySupported; |
Comment on lines
+39
to
+41
| string path = Path.GetRandomFileName(); | ||
| path = path.Replace(".", ""); // Remove period. | ||
| return string.Concat(prefix, path.Substring(0, length)); |
Comment on lines
+135
to
+142
| foreach (var keyToRemove in keysToRemove) | ||
| { | ||
| if (key.Trim().ToLower().StartsWith(keyToRemove.Trim().ToLower(), StringComparison.Ordinal)) | ||
| { | ||
| removeKey = true; | ||
| break; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up changes to test suite after #4288 where AAD Password auth was disabled from pipelines. This PR removes the AAD Password Conn String parameter completely to make it easier for AAD tests to be managed.
What Changed
Pipeline / config
AAD_PASSWORD_CONN_STR(_eastus)withAZURE_SQL_CONN_STRING_WestUS2/AZURE_SQL_CONN_STRING_EastUS(bare conn string, no credentials).AADPasswordConnectionString→AzureSqlConnectionStringacrossConfig.cs,config.default.jsonc, and pipeline templates (update-config-file-step.yml,ci-run-tests-job.yml,dotnet-sqlclient-ci-core.yml,test-azure-package-ci-job.yml).AADServicePrincipalIdinside theIsFork == 'False'guard (matches existing policy forAADServicePrincipalSecret).New shared test helpers (
tests/Common/)CommonUtils.cs:EnsureSeparator,GenerateRandomSecureString,GenerateRandomCharacters,GenerateObjectName.StringExtensions.cs: fluentAdd*AuthenticationToConnStringextensions (Integrated, Interactive, DeviceCodeFlow, ServicePrincipal, WorkloadIdentity, MSI, Default) plusAddUserToConnString/AddPasswordToConnString. Tests now compose auth ontoAzureSqlConnectionStringdynamically — required for broker-aware test legs.Test updates
ManualTests/SQL/ConnectivityTests/AADConnectionTest.cs: removed allActiveDirectoryPasswordcases; rewrote MSI / Workload Identity / Device Code / Default / token-callback tests against the new helpers; applied target-typednew(), collection expressions,using SecureString,Assert.Equal(ConnectionState.Open, …).DataTestUtility.cs,AADFedAuthTokenRefreshTest,ConnectionPoolTest, andMicrosoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs+Config.csupdated to match.TESTGUIDE.mdand.github/instructions/testing.instructions.mdupdated.Breaking change (CI only)
Downstream pipelines /
config.user.jsoncmust renameAADPasswordConnectionString→AzureSqlConnectionStringand supply a bare Azure SQL connection string (noAuthentication=,User ID=,Password=). The variablesAAD_PASSWORD_CONN_STR/_eastusare no longer consumed and can be retired.Validation
dotnet buildManualTests / net9.0 — 0 warnings, 0 errors.dotnet testUnitTests / net9.0 — 792 pass; 4 failures are pre-existing macOS-env issues (Windows-only SSPI + keychain-restricted AE fixtures), not regressions.