[DRAFT] Add tests for HasRows + Mutliple INFO tokens#3744
Draft
paulmedynski wants to merge 4 commits intomainfrom
Draft
[DRAFT] Add tests for HasRows + Mutliple INFO tokens#3744paulmedynski wants to merge 4 commits intomainfrom
paulmedynski wants to merge 4 commits intomainfrom
Conversation
- Added tests for SqlDataReader.HasRows for responses with and without INFO tokens.
- Added comments.
paulmedynski
commented
Nov 5, 2025
Contributor
Author
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| /// </summary> | ||
| public QueryEngine(TdsServerArguments arguments) | ||
| { | ||
| Log = arguments.Log; |
Contributor
Author
There was a problem hiding this comment.
This ensures the engine has its Log set, regardless of what TdsServer may decide.
| TDSInfoToken infoToken = new TDSInfoToken(2012, 2, 0, lowerBatchText); | ||
| // Create an info token that contains the verbatim query text we | ||
| // received (not the lower-cased version). | ||
| TDSInfoToken infoToken = new TDSInfoToken(2012, 2, 0, batchRequest.Text); |
Contributor
Author
There was a problem hiding this comment.
Now the query text in the INFO matches what the test actually sent.
| /// Constructor with a query engine. Uses the engine's servers arguments. | ||
| /// </summary> | ||
| /// <param name="queryEngine">Query engine</param> | ||
| public TdsServer(QueryEngine queryEngine) |
Contributor
Author
There was a problem hiding this comment.
Convenience constructor to avoid having to specify ServerArguments twice in the test.
| { | ||
| Log(log, string.Format("{0}[{1}]", prefix, index++), o); | ||
| SerializedWriteLineToLog(log, string.Format("{0}: <null>", prefix)); | ||
| log.Flush(); |
Contributor
Author
There was a problem hiding this comment.
Added missing Flush() calls.
| log.WriteLine(string.Format("[{0}] {1}", DateTime.Now, text)); | ||
| } | ||
| var now = DateTime.UtcNow; | ||
| log.WriteLine($"[{now:yyyy-MM-dd hh:mm:ss.fff}Z] {text}"); |
Contributor
Author
There was a problem hiding this comment.
Better timestamp formatting, and changed to UTC.
|
|
||
| // We should have read past the INFO tokens and determined that there | ||
| // are row results. | ||
| Assert.True(reader.HasRows); |
Contributor
Author
There was a problem hiding this comment.
I was expecting this to fail for infoCount > 1 based on #3018, but it passes. I will investigate further.
- Added placement of INFO tokens as start, middle, and end of the response stream to elicit failures.
- Pinpointed the failing cases and have allowed them to pass for now, with TODOs to address if/when we make a fix to SqlDataReader.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description