Categorize Entra integrated test as interactive - #4510
Conversation
paulmedynski
added
the
Area\Tests
There was a problem hiding this comment.
Pull request overview
Updates the inline commentary above the ADIntegratedUsingSSPI test to more accurately explain why the test was previously skipped in certain CI jobs and why it fails when it does run (missing Entra-integrated user context in CI). This fits within the SqlClient Extensions Azure test suite by clarifying expected CI limitations without changing behavior.
Changes:
- Replaces the prior brief explanation with a more detailed, pipeline-configuration-focused explanation of the skip/failure conditions.
- Retains the existing
ActiveIssueannotation and associated failure-output context.
| // This test was skipped in the 6.0/6.1 CI jobs because SupportsIntegratedSecurity | ||
| // was passed as a runtime variable to a compile-time pipeline expression. After that | ||
| // configuration was fixed, the test began running and exposed that the CI environment | ||
| // does not provide the Entra-integrated user context required by Active Directory Integrated: |
There was a problem hiding this comment.
We don't have env to run this test, and it was correctly controlled by the SupportsIntegratedSecurity property. It should be possible to run this manually in a supported Windows env by setting this config to true.
Why not fix the test source code such that it continues to skip as before?
There was a problem hiding this comment.
It was being skipped because SupportsIntegratedSecurity was always false due to a bug. By the time I fixed that bug, I had already marked the test as ActiveIssue. Now that we know we don't have a CI environment suitable for running this test, we can re-classify it as "Must be run by a human", which we already have a trait called "Interactive" for. I will update the trait. From what I can tell SupportsIntegratedSecurity is meant to gate Windows Integrated auth secnarios. This test needs Entra integration, which is a separate beast entirely.
There was a problem hiding this comment.
No, Active Directory Integrated authentication can be triggered by SSPI (windows auth) workflow when targeting Azure SQL, this is a supported driver scenario. That's why this test is called "ADIntegratedUsingSSPI".'
cheenamalhotra
left a comment
cheenamalhotra
left a comment
There was a problem hiding this comment.
This is a manual test. It should be skipped as before using the same config.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
paulmedynski
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs:90
- PR description says the test comment change retains the ActiveIssue annotation, but the diff removes
[ActiveIssue(...)]entirely. If the work item link is still meant to be tracked on the test, re-add the attribute (it can coexist with the Interactive category) or update the PR description to match the new approach.
// This test requires a signed-in user identity configured for Entra Integrated
// authentication and is excluded from non-interactive test runs by default.
[Trait("Category", "Interactive")]
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Category", "Interactive")] |
There was a problem hiding this comment.
For consistency with the casing of our other traits.
| [ActiveIssue("https://sqlclientdrivers.visualstudio.com/ADO.Net/_workitems/edit/45941")] | ||
| // This test requires a signed-in user identity configured for Entra Integrated | ||
| // authentication and is excluded from non-interactive test runs by default. | ||
| [Trait("Category", "interactive")] |
There was a problem hiding this comment.
build.proj skips interactive tests by default, so this will not run in CI.
Pull request was converted to draft
|
Putting back into Draft while I track down which SSPI/Integrated Security tests can run in CI, and which cannot. PR #3887 enabled |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4510 +/- ##
==========================================
- Coverage 64.71% 62.83% -1.89%
==========================================
Files 288 283 -5
Lines 44088 67041 +22953
==========================================
+ Hits 28532 42124 +13592
- Misses 15556 24917 +9361
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Categorizes
ADIntegratedUsingSSPIasinteractiveusing the Azure test project's existing trait convention. The standardcategory!=interactivefilters now exclude the test from non-interactive runs.The failed PR build exposed a casing mismatch: the repository filters use lowercase
interactive, while the two Azure interactive tests usedInteractive. Both trait values are now normalized to lowercase for reliable hosted-agent filtering.The test requires a signed-in Windows user identity that can acquire a Microsoft Entra token and is authorized on an Entra-enabled SQL target.
SupportsIntegratedSecurityonly establishes Windows Integrated Security readiness and is not a proxy for that environment.The durable Entra Integrated CI environment remains tracked separately; this change classifies the current execution requirement rather than closing that coverage gap.