Skip to content

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Oct 23, 2025

Backports #3488 to 6.1. We anticipate adding more simulated server tests as we address bugs, so this change will make the bugfix backports to 6.1 easier.

@mdaigle mdaigle changed the title [6.1] Refactor Test TDS Servers. Expand functional testing of connection po… [6.1] Refactor Test TDS Servers Oct 23, 2025
@mdaigle mdaigle marked this pull request as ready for review October 28, 2025 20:54
@mdaigle mdaigle requested a review from a team as a code owner October 28, 2025 20:54
Copilot AI review requested due to automatic review settings October 28, 2025 20:54
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 refactors the TDS test server infrastructure to modernize class naming conventions, improve testability, and consolidate test utilities. It renames classes from TDS suffix to Tds (PascalCase), converts GenericTDSServer to a generic abstract base class, removes redundant constructors in favor of property initializers, and consolidates test utilities.

Key Changes

  • Renamed all TDS server classes to use PascalCase (TDSTds)
  • Converted GenericTdsServer to generic abstract class with typed arguments
  • Removed constructors in server argument classes, using property initializers instead
  • Added new specialized servers: TransientTdsErrorTdsServer, TransientDelayTdsServer, TdsServer
  • Consolidated test utilities from multiple test projects into centralized infrastructure
  • Added comprehensive connection tests covering transient faults, routing, and failover scenarios

Reviewed Changes

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

Show a summary per file
File Description
TdsServerArguments.cs Removed constructor, added property initializers and FailoverPartner property
GenericTdsServer.cs Converted to generic abstract base class with IDisposable, added Start() method
TransientTdsErrorTdsServer.cs New server for simulating transient error responses
TransientDelayTdsServer.cs New server for simulating network delays
TDSServerEndPointConnection.cs Changed from Thread to Task-based async processing
ConnectionTests.cs New comprehensive connection test suite
AdapterUtil.cs Changed s_azureSqlServerEndpoints from array to List for test manipulation
Comments suppressed due to low confidence (15)

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:135

  • The Start method is not thread-safe. If called concurrently, multiple threads could pass the null check before _endpoint is assigned, leading to multiple endpoints being created. Consider adding a lock or using Interlocked operations to ensure thread safety.
    src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs:1
  • Changing s_azureSqlServerEndpoints from an immutable array to a mutable List exposes production code to potential modification in non-test scenarios. Consider using ImmutableList or creating a separate test-only mechanism (e.g., virtual method for tests to override) to avoid compromising production code safety.
// Licensed to the .NET Foundation under one or more agreements.

src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:322

  • Variable factory may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:188
  • Variable genericTdsServerSession may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:274
  • Variable sessionStateOption may be null at this access because of this assignment.
    Variable sessionStateOption may be null at this access because of this assignment.
    Variable sessionStateOption may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:296
  • Variable federatedAuthenticationOption may be null at this access because of this assignment.
    Variable federatedAuthenticationOption may be null at this access because of this assignment.
    Variable federatedAuthenticationOption may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServer.cs:242
  • Variable ServerArguments may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServer.cs:52
  • Variable serverArguments may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:804
  • Variable federatedAuthenticationOption may be null at this access because of this potential null argument.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServer.cs:87
  • Variable ServerArguments may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServer.cs:145
  • Variable ServerArguments may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServer.cs:197
  • Variable serverArguments may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/QueryEngine.cs:1906
  • Variable genericSession may be null at this access because of this assignment.
    src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:559
  • This assignment to connectionString is useless, since its value is never read.
    src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs:770
  • Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

@codecov
Copy link

codecov bot commented Oct 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.68%. Comparing base (7a7b54e) to head (efe8334).
⚠️ Report is 18 commits behind head on release/6.1.

Additional details and impacted files
@@               Coverage Diff               @@
##           release/6.1    #3715      +/-   ##
===============================================
- Coverage        69.69%   66.68%   -3.01%     
===============================================
  Files              281      279       -2     
  Lines            62413    61751     -662     
===============================================
- Hits             43500    41180    -2320     
- Misses           18913    20571    +1658     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 68.51% <100.00%> (-4.25%) ⬇️
netfx 69.81% <100.00%> (+0.56%) ⬆️

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.

@mdaigle
Copy link
Contributor Author

mdaigle commented Oct 29, 2025

Bypassing checks. The one enclave job that's failing is due to agent provisioning issues in the 1ES hosted pool.

@mdaigle mdaigle merged commit 19f9aab into release/6.1 Oct 29, 2025
256 of 258 checks passed
@mdaigle mdaigle deleted the dev/mdaigle/6.1-backport-test-tds-server branch October 29, 2025 18:27
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.

4 participants