Refactor Test TDS Servers. Expand functional testing of connection pooling and transient failures. #3488
Conversation
…ailover-tds-server
| initialServer.SetErrorBehavior(true, errorCode, "Transient fault occurred."); | ||
| using SqlConnection failoverConnection = new(builder.ConnectionString); | ||
| // Should fail over to the failover server | ||
| failoverConnection.Open(); |
There was a problem hiding this comment.
Don't we need to Assert() that the connection connected where we expect it to? Something like:
Assert.Equal(failoverConnection.RemoteEndpoint, failoverServer.Endpoint);
| @@ -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? | |||
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Don't we need to rename these files to match their classes?
|
|
||
| /// <summary> | ||
| /// Query engine instance | ||
| /// </summary> | ||
| protected QueryEngine Engine { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Default constructor | ||
| /// Counts unique pre-login requests to the server. |
There was a problem hiding this comment.
The term unique here implies that duplicates are a thing, and they're not counted. Does this just count each pre-login packet received, or does it ignore some of them?
| @@ -158,7 +179,7 @@ public virtual TDSMessageCollection OnPreLoginRequest(ITDSServerSession session, | |||
| TDSPreLoginToken preLoginToken = new TDSPreLoginToken(Arguments.ServerVersion, serverResponse, false); // TDS server doesn't support MARS | |||
|
|
|||
| // Cache the received Nonce into the session | |||
| (session as GenericTDSServerSession).ClientNonce = preLoginRequest.Nonce; | |||
| (session as GenericTdsServerSession).ClientNonce = preLoginRequest.Nonce; | |||
There was a problem hiding this comment.
Should we cast this once and re-use it?
| public TdsServer() : this(new TdsServerArguments()) | ||
| { | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
Missing a blank line here.
| /// <summary> | ||
| /// Constructor with arguments | ||
| /// </summary> | ||
| public TdsServer(TdsServerArguments arguments) : base(arguments) |
There was a problem hiding this comment.
You won't need the default constructor if you:
public TdsServer(TdsServerArguments arguments = new()) : base(arguments)
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.