Refactor Test TDS Servers. Expand functional testing of connection pooling and transient failures. #3488

mdaigle · 2025-07-17T19:04:56Z

Description

The TDS.Servers project provides mocked server implementations for a variety of use cases. The construction and endpoint initialization flows for these mock servers was complicated and resulted in duplicated code across TDS.Servers, FunctionalTests, and ManualTests. This PR consolidates endpoint initialization and makes test server constructor parameters more explicit (no more defaults buried a few layers deep).

One downside of this approach is that it's now slightly harder to get the connection string for a test server. Before, some servers had a ConnectionString property you could access. The issue with these connection strings is they had a lot of non-standard default behavior coded into them at the time they were constructed. This makes it hard to understand which properties you're actually using when connecting to a test server. With my changes, you now need to manually construct a connection string using SqlConnectionStringBuilder and explicitly set connection parameters. Any parameters you don't set use the stock connection string defaults.

Testing

I added some testing around pool invalidation during failover and routing behavior during transient errors.

…ailover-tds-server

…ass.

paulmedynski

Looks good to me, with a few suggestions.

paulmedynski · 2025-07-18T11:05:16Z

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs

+            initialServer.SetErrorBehavior(true, errorCode, "Transient fault occurred.");
+            using SqlConnection failoverConnection = new(builder.ConnectionString);
+            // Should fail over to the failover server
+            failoverConnection.Open();


Don't we need to Assert() that the connection connected where we expect it to? Something like:

Assert.Equal(failoverConnection.RemoteEndpoint, failoverServer.Endpoint);

paulmedynski · 2025-07-18T11:07:41Z

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs

@@ -310,8 +397,14 @@ public void ConnectionTestValidCredentialCombination()
        public void ConnectionTimeoutTest(int timeout)
        {
            // Start a server with connection timeout from the inline data.
-            using TestTdsServer server = TestTdsServer.StartTestServer(false, false, timeout);
-            using SqlConnection connection = new SqlConnection(server.ConnectionString);
+            //TODO: do we even need a server for this test?


IMO we shouldn't need a server for most of these tests. We're missing a way to intercept outbound TDS packets and inject inbound TDS packets. If we had that, we wouldn't need TdsServer for these kinds of unit tests, and we could clearly see in each test exactly what was being tested. Right now, it isn't obvious what TdsServer is doing or how it is verifying our expectations. This is something I think we need to work towards - nothing for this PR to address.

paulmedynski · 2025-07-18T11:13:48Z

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionReadOnlyRoutingTests.cs

+            Assert.Equal(ConnectionState.Open, connection.State);
+
+            // Failures should prompt the client to return to the original server, resulting in a login count of 2
+            Assert.Equal(2, router.PreLoginCount);


We haven't verified what Open() actually did though - just what the side-effects on our test servers were. Did Open() make a bunch of unexpected failed connection attempts elsewhere? Which test server, if any, is it currently connected to? We don't know.

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/AuthenticatingTDSServer.cs

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTDSServer.cs

paulmedynski · 2025-07-18T11:22:57Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TdsServer.cs

+        public TdsServer() : this(new TdsServerArguments())
+        {
+        }
+        /// <summary>


Missing a blank line here.

paulmedynski · 2025-07-18T11:23:59Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TdsServer.cs

+        /// <summary>
+        /// Constructor with arguments
+        /// </summary>
+        public TdsServer(TdsServerArguments arguments) : base(arguments)


You won't need the default constructor if you:

public TdsServer(TdsServerArguments arguments = new()) : base(arguments)

default arguments need to be compile time constant or a ValType.

Frown - that's an unfortunate restriction. Can TdsServerArguments be a struct?

Not easily. You can't inherit from a struct, so we would have to flatten all of these server-type specific arguments down into one flat struct. It would work but wouldn't be super usable.

…ut. Clean up transient error server.

…p where they're supposed to, not just side effects.

paulmedynski

A few tidying-up suggestions.

paulmedynski · 2025-08-07T10:52:00Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs

@@ -79,7 +80,7 @@ public abstract class ServerEndPointConnection
        /// <summary>
        /// The flag indicates whether server is being stopped


This property isn't really a flag any more.

paulmedynski · 2025-08-07T10:57:48Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientFaultTdsServer.cs

-        /// <summary>
-        /// Handler for login request
-        /// </summary>
+            /// <summary>


Extra indent.

paulmedynski · 2025-08-07T11:00:26Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TdsServer.cs

+        /// <summary>
+        /// Constructor with arguments
+        /// </summary>
+        public TdsServer(TdsServerArguments arguments) : base(arguments)


Frown - that's an unfortunate restriction. Can TdsServerArguments be a struct?

paulmedynski · 2025-08-07T11:05:59Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientTimeoutTdsServer.cs

+        /// </summary>
+        public override TDSMessageCollection OnLogin7Request(ITDSServerSession session, TDSMessage request)
+        {
+            // Check if we're still going to raise transient error


How/where does this cause a transient error?

I think this server is intended to delay responses, eliciting a driver-side timeout. So this isn't really a timeout server - it's a delay server.

You could also make the count of delayed requests configurable instead of hardcoding it to 1. Then you could test a variety of retry counts/mechanisms in the driver.

paulmedynski · 2025-08-07T11:08:04Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientTimeoutTdsServerArguments.cs

+        /// </summary>
+        public TransientTimeoutTdsServerArguments()
+        {
+            SleepDuration = TimeSpan.FromSeconds(0);


Can you just set these on the properties, and avoid a constructor?

public TimeSpan SleepDuration { get; set; } = TimeSpan.FromSeconds(0);

That makes it easy to see the default value for each property.

…ailover-tds-server

mdaigle added 9 commits June 12, 2025 09:15

Add failover config options

8afcbc7

Test failovers

0007a10

Merge branch 'main' of github.com:dotnet/SqlClient into dev/mdaigle/f…

03b8e49

…ailover-tds-server

Test routing through a transient error.

2adf97c

Move startup to GenericTDSServer.

5113fa7

Remove duplicate server start code from ManualTests

522abba

Refactor failover param ownership. Fix pool invalidation test.

c41d0dc

Cleanup. Rename GenericTDSServer to differentiate from the generic cl…

922b3ef

…ass.

Rename classes to match style. Make GenericTdsServer<T> abstract.

6ef83d7

mdaigle added the Area\Tests Issues that are targeted to tests or test projects label Jul 17, 2025

paulmedynski requested changes Jul 18, 2025

View reviewed changes

mdaigle added 17 commits July 24, 2025 14:19

Fix connection string to use optional encryption.

47cbb48

Fix connection string to use optional encryption.

905bc5d

Add transient timeout server. Add regression test for transient timeo…

47aa3a3

…ut. Clean up transient error server.

Clean up tests. Add new retry/failover tests.

fe9e7e4

Add new retry/failover tests.

93ebf98

Added tests. Moved tests to UnitTests project. Removed failover repro.

04a4329

Restructure tests for readability.

ee9d157

Update tests to remove unneeded routing. Check that connections end u…

11d90db

…p where they're supposed to, not just side effects.

Review changes

846a493

Add TDS projects to compile set for UnitTests.

f08447a

fix tds projects inclusion

7780cdd

Fix test server disposal.

ed2285c

Remove tests for unsupported scenarios.

47b7bc2

Add comments for confusing behavior.

7532181

Rename files to match class names.

57aeb4d

Remove proj change.

12cbb82

Skip failing unit tests.

038a0cd

paulmedynski requested changes Aug 7, 2025

View reviewed changes

Rename test folder.

206216a

mdaigle added 7 commits August 14, 2025 15:26

Review changes

487fc26

Merge branch 'main' of github.com:dotnet/SqlClient into dev/mdaigle/f…

aeb0c29

…ailover-tds-server

Adjust tests based on multisubnetfailover setting.

767e99c

Improve test reliability.

4bda823

Make failover test more reliable.

a92dc51

Rename to fix missing file.

2638eca

Update tests to work on linux/mac

1492ead

		@@ -79,7 +80,7 @@ public abstract class ServerEndPointConnection
		/// <summary>
		/// The flag indicates whether server is being stopped

Refactor Test TDS Servers. Expand functional testing of connection pooling and transient failures. #3488

Are you sure you want to change the base?

Refactor Test TDS Servers. Expand functional testing of connection pooling and transient failures. #3488

Uh oh!

Conversation

mdaigle commented Jul 17, 2025 • edited by paulmedynski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdaigle commented Jul 17, 2025 •

edited by paulmedynski

Loading