Skip to content

Conversation

@paulmedynski
Copy link
Contributor

Description

Port of #3775 to release/6.1

  • Added max event session duration where that feature is supported.
  • Added test name to event session name.
  • Flattened using blocks to statements.
  • Applied single-branch config to the approver policy (an unrelated change).

@paulmedynski paulmedynski added this to the 6.1.4 milestone Dec 1, 2025
Copilot AI review requested due to automatic review settings December 1, 2025 18:10
@paulmedynski paulmedynski requested a review from a team as a code owner December 1, 2025 18:10
Copilot finished reviewing on behalf of paulmedynski December 1, 2025 18:14
Copy link
Contributor

Copilot AI left a 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 pull request ports fixes from #3775 to the release/6.1 branch, addressing XEvent test failures and preventing orphaned XEvent sessions in test environments. The changes refactor the XEventScope test utility to include test names in session identifiers, add automatic session expiration for supported SQL Server versions, and convert affected test classes from static to instance-based to enable test name tracking.

Key changes:

  • Refactored XEventScope to accept test name as first parameter and added MAX_DURATION support for Azure SQL and SQL Server 2025+
  • Converted XEventsTracingTest and DataStreamTest from static to instance-based classes with ITestOutputHelper constructors
  • Introduced CurrentTestName() utility method using reflection to extract test names from xUnit's internal structures
  • Refactored GetSqlServerProperty() to use a ServerProperty enum instead of string parameters

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/XEventsTracingTest.cs Converted class to instance-based, added constructor with ITestOutputHelper, updated XEventScope usage to pass test name, added nullable annotations
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs Converted class to instance-based, added constructor with ITestOutputHelper, updated XEventScope usage with test name and explicit connection opening, flattened using statements
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs Added CurrentTestName() method with regex parsing, added ServerProperty enum, refactored GetSqlServerProperty() to use enum, added IsNotManagedInstance() method, enhanced XEventScope with test name parameter, MAX_DURATION support, and improved documentation

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +1374 to +1375
// Both Azure SQL and SQL Server 2025+ support setting a maximum
// duration for the XEvent session.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "Both Azure SQL and SQL Server 2025+ support setting a maximum duration" but the condition checks for version 17+, not specifically 2025+. While SQL Server 2025 is version 17, this could be confusing. Consider clarifying the comment:

// Both Azure SQL and SQL Server version 17 (SQL Server 2025) or higher
// support setting a maximum duration for the XEvent session.

This makes it clear that version 17 corresponds to SQL Server 2025.

Suggested change
// Both Azure SQL and SQL Server 2025+ support setting a maximum
// duration for the XEvent session.
// Both Azure SQL and SQL Server version 17 (SQL Server 2025) or higher
// support setting a maximum duration for the XEvent session.

Copilot uses AI. Check for mistakes.
private static readonly Regex TestNameRegex = new(
// Capture the ClassName.MethodName portion, which may terminate
// the name, or have (args...) appended.
@"\.((?:[^.]+)\.(?:[^.\(]+))(?:\(.*\))?$",
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern @"\.((?:[^.]+)\.(?:[^.\(]+))(?:\(.*\))?$" uses a non-capturing group (?:[^.]+) for the class name and (?:[^.\(]+) for the method name, which is good. However, the pattern may not handle edge cases where:

  1. The test display name doesn't follow the expected format
  2. Class or method names contain escaped parentheses or other special characters

Consider adding more robust error handling around the regex matching logic, or documenting the assumptions about test name format more explicitly. The current assertions will cause the test to fail, but a more descriptive error message would help diagnose issues faster if xUnit changes its display name format.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.82%. Comparing base (b21442d) to head (0988f1b).
⚠️ Report is 1 commits behind head on release/6.1.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/6.1    #3817       +/-   ##
================================================
+ Coverage        65.27%   90.82%   +25.54%     
================================================
  Files              279        6      -273     
  Lines            61765      316    -61449     
================================================
- Hits             40319      287    -40032     
+ Misses           21446       29    -21417     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore ?
netfx ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants