Skip to content

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Dec 4, 2025

Converted XEventScope to a class because ref struct types are only available starting in c# v13 and this branch uses v9.

Includes various fixes to enable compilation on linux.

Copilot AI review requested due to automatic review settings December 4, 2025 18:17
@mdaigle mdaigle changed the base branch from main to release/6.0 December 4, 2025 18:18
Copilot finished reviewing on behalf of mdaigle December 4, 2025 18:21
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 PR backports fixes from the main branch (#3775, #3819) to the 6.0 branch, addressing XEvent test failures and adding CodeQL workflow support. The key change involves converting XEventScope from a ref struct to a class to maintain C# v9 compatibility (ref struct interfaces require C# v13).

  • Adds comprehensive XEventScope utility class to manage XEvent sessions with automatic cleanup
  • Refactors test infrastructure to use instance methods with ITestOutputHelper for proper test name extraction
  • Implements a type-safe ServerProperty enum for querying SQL Server properties
  • Adds CodeQL security scanning workflow for the repository

Reviewed changes

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

File Description
XEventsTracingTest.cs New test validating that client-side activity IDs are correctly passed to server-side XEvent sessions
DataStreamTest.cs Converts test class from static to instance-based; refactors XEvent streaming test to use XEventScope utility; contains unresolved merge conflict
DataTestUtility.cs Adds XEventScope helper class, CurrentTestName method, and ServerProperty enum; refactors GetSqlServerProperty for type safety
codeql.yml New GitHub Actions workflow for CodeQL security scanning; configured for 'main' branch instead of '6.0' branch

Copilot AI review requested due to automatic review settings December 4, 2025 18:56
Copilot finished reviewing on behalf of mdaigle December 4, 2025 19:01
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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.

// Flush data
}

ids = TraceListener.ActivityIDs;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The property ActivityIDs does not exist on MDSEventListener or its base class BaseEventListener. The BaseEventListener class only has IDs (of type List<int>) and EventData properties. This code will fail to compile.

Either:

  1. Add an ActivityIDs property to BaseEventListener or MDSEventListener, or
  2. Change this line to use the correct property name (likely IDs if tracking event IDs)
Suggested change
ids = TraceListener.ActivityIDs;
ids = new HashSet<string>(TraceListener.IDs.ConvertAll(id => id.ToString()));

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 4, 2025 19:41
Copilot finished reviewing on behalf of mdaigle December 4, 2025 19:45
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

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

mdaigle and others added 3 commits December 4, 2025 16:26
* Upgrade / align packages in test projects /

* Reference System.ServiceProcess on netfx

* Always include System.Transactions as well

* Remove references to xunit.runner.console and xunit.runner.utility to enable discovery of netfx tests again. See dotnet/runtime#94183

* Revert nuget.org addition

* Upgrade Versions
Remove VersionsNet8OrLater, not needed anymore

* Force build

* Remove dummy

* Add back console runners

* Remove System.ServiceProcess, should not be needed

* I think we do actually still need the net9 versions

---------

Co-authored-by: Michel Zehnder <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 00:43
Copilot finished reviewing on behalf of mdaigle December 5, 2025 00:47
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

Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.

Copilot AI review requested due to automatic review settings December 5, 2025 18:56
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

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings December 5, 2025 19:33
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

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

$version = "${{parameters.targetFramework }}".Substring(3, "${{parameters.targetFramework }}".Length-3)
}
# Install targetFramework specific .NET runtime (and sdk)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like other test tasks, we want to test against specific runtime versions depending on the the target framework.

.\dotnet-install.ps1 -Channel $version -Architecture x86 -InstallDir "$(dotnetx86RootPath)"
# Install globally required .NET sdk
.\dotnet-install.ps1 -Architecture x86 -InstallDir "$(dotnetx86RootPath)" -JSonFile global.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that we have installed the SDK version required by the global.json file.

</Compile>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlMetadataFactory.cs">
<Link>Microsoft\Data\SqlClient\SqlMetadataFactory.cs</Link>
<Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SqlMetaDataFactory.cs">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case change to support building on linux.

run: |
mkdir packages
dotnet build src/
dotnet build src/Microsoft.Data.SqlClient.sln
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think our solution file on this branch is old and doesn't play nicely with certain .NET versions, so it doesn't get recognized as the default target. Specifying the file explicitly helps dotnet determine what to build when there are multiple targets available.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because there is a .dcproj file in src/ too, so dotnet doesn't know which one you want to build.

@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.Net.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case change for linux

<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</EmbeddedResource>
</ItemGroup>
<ItemGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the lead of #3403

<ErrorReport>None</ErrorReport>
<CodeAnalysisRuleSet>MinimumRecommendedRules.ruleset</CodeAnalysisRuleSet>
<!-- This code analysis ruleset only exists with the .NET Framework toolset. -->
<CodeAnalysisRuleSet Condition="$(MSBuildRuntimeType) == 'Full'">MinimumRecommendedRules.ruleset</CodeAnalysisRuleSet>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the lead of #3754

<Compile Include="Common\Microsoft\Data\Common\NameValuePermission.cs" />
<Compile Include="Microsoft\Data\Common\DbConnectionOptions.cs" />
<Compile Include="Microsoft\Data\Common\DbConnectionString.cs" />
<Compile Include="Microsoft\Data\Common\DBConnectionString.cs" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case change for linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case change for linux

<MicrosoftWin32RegistryVersion>5.0.0</MicrosoftWin32RegistryVersion>
<NewtonsoftJsonVersion>13.0.3</NewtonsoftJsonVersion>
<SystemDataOdbcVersion>8.0.1</SystemDataOdbcVersion>
<SystemFormatsAsn1Version>6.0.1</SystemFormatsAsn1Version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidentally deleted this in the last commit.

{
"sdk": {
"version": "9.0.0",
"version": "9.0.308",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifying a feature version allows this file to be used by the install-dotnet powershell script to install this version.

@mdaigle mdaigle changed the title [6.0] Fix xevent and codeql (#3775)(#3819) [6.0] Fix xevent and codeql (#3775)(#3819), add global.json Dec 5, 2025

#nullable enable

public sealed class XEventScope : IDisposable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted XEventScope to a class because ref struct types are only available starting in c# v13 and this branch uses v9.

@mdaigle mdaigle marked this pull request as ready for review December 5, 2025 20:02
@mdaigle mdaigle requested a review from a team as a code owner December 5, 2025 20:02
paulmedynski
paulmedynski previously approved these changes Dec 5, 2025
cheenamalhotra
cheenamalhotra previously approved these changes Dec 5, 2025
@mdaigle mdaigle dismissed stale reviews from cheenamalhotra and paulmedynski via b666719 December 5, 2025 22:01
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.

5 participants