-
Notifications
You must be signed in to change notification settings - Fork 316
Fix xevent test failures, avoid orphaned sessions #3775
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
Conversation
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 fixes XEvent test failures by preventing orphaned event sessions through two key improvements: adding automatic session expiration via MAX_DURATION on SQL Server 2017+ and incorporating test names into session names for better traceability.
Key Changes:
- Added version detection to conditionally set MAX_DURATION for XEvent sessions on SQL Server 2017+
- Refactored XEventScope to accept test names as session prefixes for improved identification
- Introduced ServerProperty enum for type-safe server property queries
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| DataTestUtility.cs | Added CurrentTestName() helper, ServerProperty enum, refactored GetSqlServerProperty() for type safety, and enhanced XEventScope with version detection and duration support |
| XEventsTracingTest.cs | Converted to instance class with constructor injection to pass test name to XEventScope |
| DataStreamTest.cs | Converted from static to instance class with constructor injection to pass test name to XEventScope |
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
.azuredevops/policies/approvercountpolicy.yml:1
- [nitpick] The original comment referencing a specific Merlinbot PR (line 6 in the old version) has been replaced with a generic documentation link. While the new link is useful, removing the historical reference to the PR that suggested this configuration may reduce traceability. Consider keeping both the original PR reference and the new documentation link.
# This file configures the Approver Count Policy Validator status check. Some
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
20a788e to
ec9bbd4
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 for reviewers.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs
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
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
- Added max event session duration where that feature is supported. - Added test name to event session name. - Restricting approve count policy to internal/main.
7adf0b3 to
e60f020
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 4 out of 4 changed files in this pull request and generated no new comments.
|
Ignore the Merge Code Coverage job failure - that is being worked on in #3798. |
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.
One request, otherwise looks good.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
aa5cd2d
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 4 out of 4 changed files in this pull request and generated 1 comment.
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/DataTestUtility.cs
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #3775 +/- ##
==========================================
- Coverage 76.61% 69.82% -6.79%
==========================================
Files 274 268 -6
Lines 43393 66432 +23039
==========================================
+ Hits 33247 46389 +13142
- Misses 10146 20043 +9897
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:
|
- Added IsNotManagedInstance() for XEvents tests.
Description