[7.0] Test | Fix Diagnostic Tests and remove Remote Executor#4116
[7.0] Test | Fix Diagnostic Tests and remove Remote Executor#4116paulmedynski wants to merge 1 commit intorelease/7.0from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR (cherry-pick of #4078 onto release/7.0) updates the manual diagnostics tracing tests to run successfully again by removing the RemoteExecutor dependency and tightening up how diagnostic events are observed during async connection open.
Changes:
- Refactored
DiagnosticTestto run in-proc (noRemoteExecutor) and added sync/async test groupings with shared helper validation. - Removed
Microsoft.DotNet.RemoteExecutorpackage references from the ManualTests project and centralized test package versions. - Adjusted
SqlConnection.InternalOpenAsynccontinuation registration to ensure the diagnostic completion continuation runs deterministically (even if the original operation is cancelled).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/DiagnosticTest.cs | Reworks diagnostics tests to run in-process, adds collection-based serialization, and simplifies diagnostic payload validation. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj | Removes RemoteExecutor dependency and tweaks project reference metadata. |
| src/Microsoft.Data.SqlClient/tests/Directory.Packages.props | Removes the Microsoft.DotNet.RemoteExecutor package version entry. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs | Updates the async-open continuation wiring for diagnostic “after/error” events. |
| using SqlCommand cmd = new("SELECT *, baddata = 1 / 0 FROM sys.objects FOR xml auto, xmldata;", conn); | ||
| #endif | ||
| conn.Open(); | ||
| Assert.Throws<SqlException>(() => cmd.ExecuteXmlReader()); |
There was a problem hiding this comment.
ExecuteXmlReaderAsyncErrorTest is intended to validate the async XML-reader error path, but it currently asserts on the sync ExecuteXmlReader() (and doesn’t use Assert.ThrowsAsync). This makes the test not cover the async API and may miss regressions in ExecuteXmlReaderAsync diagnostics.
| Assert.Throws<SqlException>(() => cmd.ExecuteXmlReader()); | |
| await Assert.ThrowsAsync<SqlException>(() => cmd.ExecuteXmlReaderAsync()); |
| // Serialized execution: DiagnosticListener is global state, so these tests | ||
| // must not run in parallel with each other. | ||
| [Collection("DiagnosticTests")] | ||
| public class DiagnosticTest |
There was a problem hiding this comment.
These tests subscribe to the global SqlClientDiagnosticListener and enable/disable it via shared state. [Collection("DiagnosticTests")] only serializes tests within the collection; other test collections can still run in parallel and emit SqlClient diagnostics while the observer is enabled, which can cause flakiness/false positives now that RemoteExecutor isolation was removed. Consider adding a [CollectionDefinition("DiagnosticTests", DisableParallelization = true)] to prevent this collection from running concurrently with other collections.
Cherry-pick of #4078 to release/7.0
Original PR Description
Fixes AB#39873 and enables Diagnostics Test to run successfully.