- Notifications
You must be signed in to change notification settings - Fork311
Refactor Test TDS Servers. Expand functional testing of connection pooling and transient failures.#3488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me, with a few suggestions.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The termunique 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 asGenericTDSServerSession).ClientNonce = preLoginRequest.Nonce; | |||
| (session asGenericTdsServerSession).ClientNonce = preLoginRequest.Nonce; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we cast this once and re-use it?
| public TdsServer() : this(new TdsServerArguments()) | ||
| { | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missing a blank line here.
| /// <summary> | ||
| /// Constructor with arguments | ||
| /// </summary> | ||
| public TdsServer(TdsServerArguments arguments) : base(arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You won't need the default constructor if you:
public TdsServer(TdsServerArguments arguments = new()) : base(arguments)
Uh oh!
There was an error while loading.Please reload this page.
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.