fix: skip e2e live connection tests when SQL auth credentials unavailable #659
+24
−6
Conversation
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
The Azure DevOps pipeline runs tests twice: 1. SQL2022: Uses SQL auth (sa user) - tests will run 2. SQLDB: Uses Azure AD auth (service principal) - tests should skip The live connection e2e tests require SQL auth credentials (SQLCMDUSER/SQLCMDPASSWORD) which aren't available in the Azure AD auth scenario. Added skipIfNoSQLAuth() to properly skip these tests instead of failing with 'Login failed for user ''.
There was a problem hiding this comment.
Pull request overview
This PR fixes pipeline failures from PR #642 by adding proper test skipping logic for end-to-end tests that require SQL authentication credentials. The SQLDB pipeline test runs use Azure AD authentication without SQLCMDUSER/SQLCMDPASSWORD environment variables, causing these tests to fail inappropriately.
Changes:
- Added
hasSQLAuthCredentials()helper function to check for SQL authentication credentials (SQLCMDUSER and SQLCMDPASSWORD) - Added
skipIfNoSQLAuth()helper function that skips tests when SQL authentication is not available - Updated three live connection tests to use the new skip function instead of just checking for SQLCMDSERVER
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
PR #642 added e2e tests that require SQL authentication credentials (SQLCMDUSER/SQLCMDPASSWORD). The ADO pipeline runs tests twice with different authentication methods:
The SQLDB run intentionally omits SQL credentials because it tests Azure AD/Entra authentication via service principal (using
AZURESUBSCRIPTION_CLIENT_IDandAZURESUBSCRIPTION_TENANT_ID).PR #642's tests used
skipIfNoLiveConnection()which only checksSQLCMDSERVER. Since both runs have that set, both tried to run the tests. The SQLDB run failed with:Solution
Added
skipIfNoSQLAuth()helper that checks for SQL authentication credentials before running tests that require them:hasSQLAuthCredentials()- checks both SQLCMDUSER and SQLCMDPASSWORD are setskipIfNoSQLAuth()- chains withskipIfNoLiveConnection(), then checks SQL auth credentialsResult
The SQL2022 coverage is unchanged - tests still execute against a real SQL Server. Tests are skipped on SQLDB where they can't work anyway (no SQL auth credentials available).
Alternative Considered
Making the e2e tests work with Azure AD auth was considered, but these tests verify query functionality (piped input, -Q flag, input files), not authentication methods. Skipping when SQL auth isn't available is the pragmatic solution.
Files Changed
cmd/modern/e2e_test.go: Added SQL auth check helpers, updated 3 tests to useskipIfNoSQLAuth()