Skip to content

Draft | Some more test cleanup #3860

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
benrr101 wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Draft | Some more test cleanup #3860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
25d570a
Move SqlErrorTest to unit test project
benrr101 Dec 19, 2025
95855ed
Start on moving SqlErrorCollectionTests
benrr101 Dec 20, 2025
65c5407
Improve tests for SqlErrorCollection (part 2)
benrr101 Dec 22, 2025
3c10e3c
A bunch of junk
benrr101 Dec 22, 2025
394e255
Remove more Assert.Fail
benrr101 Dec 22, 2025
ea468ff
More junk
benrr101 Dec 23, 2025
a8b7ee4
Addressing comments from copilot
benrr101 Dec 26, 2025
39d3839
Fix issue with sqlerrorcollectiontests
benrr101 Dec 29, 2025
72bf546
Remove old SqlErrorCollectionTest
benrr101 Dec 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions src/Microsoft.Data.SqlClient/tests/Common/SqlDataReaderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.Data.SqlClient.Tests.Common
{
/// <summary>
Expand All @@ -13,14 +16,55 @@ public static class SqlDataReaderExtensions
/// Reads all result sets in the provided <paramref name="dataReader"/> and discards them.
/// </summary>
/// <param name="dataReader">Reader to flush results from.</param>
public static void FlushAllResults(this SqlDataReader dataReader)
/// <param name="flushResults">
/// If <c>true</c>, the records in each result set will be flushed, too. If <c>false</c>
/// only <see cref="SqlDataReader.NextResult"/> will be the only method called.
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "will be the only method called" is redundant. Consider simplifying to "only will be called."

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@benrr101 benrr101 Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not doing it.

/// </param>
Comment on lines +19 to +22
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation has redundant wording. The phrase "will be the only method called" is stated twice in the sentence. Consider revising to: "If false, only SqlDataReader.NextResult will be called."

Copilot uses AI. Check for mistakes.
public static void FlushAllResults(this SqlDataReader dataReader, bool flushResults = true)
{
do
{
dataReader.FlushResultSet();
if (flushResults)
{
dataReader.FlushResultSet();
}
} while (dataReader.NextResult());
}

/// <summary>
/// Reads all result sets in the provided <paramref name="dataReader"/> and discards them.
/// </summary>
/// <param name="dataReader">Reader to flush results from.</param>
/// <param name="flushResults">
/// If <c>true</c>, the records in each result set will be flushed, too. If <c>false</c>
/// only <see cref="SqlDataReader.NextResultAsync()"/> will be the only method called.
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "will be the only method called" is redundant. Consider simplifying to "only will be called."

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@benrr101 benrr101 Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah

/// </param>
Comment on lines +38 to +41
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation has redundant wording. The phrase "will be the only method called" is stated twice in the sentence. Consider revising to: "If false, only SqlDataReader.NextResultAsync() will be called."

Copilot uses AI. Check for mistakes.
public static Task FlushAllResultsAsync(this SqlDataReader dataReader, bool flushResults = true) =>
FlushAllResultsAsync(dataReader, CancellationToken.None, flushResults);

/// <summary>
/// Reads all result sets in the provided <paramref name="dataReader"/> and discards them.
/// </summary>
/// <param name="dataReader">Reader to flush results from.</param>
/// <param name="cancellationToken">Token to use for premature cancellation of the task.</param>
/// <param name="flushResults">
/// If <c>true</c>, the records in each result set will be flushed, too. If <c>false</c>
/// only <see cref="SqlDataReader.NextResultAsync()"/> will be the only method called.
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "will be the only method called" is redundant. Consider simplifying to "only will be called."

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@benrr101 benrr101 Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah

/// </param>
Comment on lines +50 to +53
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation has redundant wording. The phrase "will be the only method called" is stated twice in the sentence. Consider revising to: "If false, only SqlDataReader.NextResultAsync() will be called."

Copilot uses AI. Check for mistakes.
public static async Task FlushAllResultsAsync(
this SqlDataReader dataReader,
CancellationToken cancellationToken,
bool flushResults = true)
{
do
{
if (flushResults)
{
await dataReader.FlushResultSetAsync(cancellationToken).ConfigureAwait(false);
}
} while (await dataReader.NextResultAsync(cancellationToken).ConfigureAwait(false));
}

/// <summary>
/// Reads all results in the current result set of the provided <paramref name="dataReader"/>
/// and discards them.
Expand All @@ -33,5 +77,27 @@ public static void FlushResultSet(this SqlDataReader dataReader)
// Discard results.
}
}

/// <summary>
/// Reads all results in the current result set of the provided <paramref name="dataReader"/>
/// and discards them.
/// </summary>
/// <param name="dataReader">Reader to flush results from.</param>
public static Task FlushResultSetAsync(this SqlDataReader dataReader) =>
FlushResultSetAsync(dataReader, CancellationToken.None);

/// <summary>
/// Reads all results in the current result set of the provided <paramref name="dataReader"/>
/// and discards them.
/// </summary>
/// <param name="dataReader">Reader to flush results from.</param>
/// <param name="cancellationToken">Token to use for premature cancellation of the task.</param>
public static async Task FlushResultSetAsync(this SqlDataReader dataReader, CancellationToken cancellationToken)
{
while (await dataReader.ReadAsync(cancellationToken).ConfigureAwait(false))
{
// Discard results.
}
}
}
}
2 changes: 0 additions & 2 deletions ...soft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,12 @@
<Compile Include="BaseProviderAsyncTest\MockDataReader.cs" />
<Compile Include="SqlCredentialTest.cs" />
<Compile Include="SqlDataRecordTest.cs" />
<Compile Include="SqlErrorTest.cs" />
<Compile Include="SqlExceptionTest.cs" />
<Compile Include="SqlFacetAttributeTest.cs" />
<Compile Include="SqlNotificationRequestTest.cs" />
<Compile Include="SqlParameterCollectionTest.cs" />
<Compile Include="SqlParameterTest.cs" />
<Compile Include="SqlClientFactoryTest.cs" />
<Compile Include="SqlErrorCollectionTest.cs" />
<Compile Include="SqlBulkCopyColumnMappingCollectionTest.cs" />
<Compile Include="SqlBulkCopyColumnOrderHintCollectionTest.cs" />
<Compile Include="AmbientTransactionFailureTest.cs" />
Expand Down
74 changes: 32 additions & 42 deletions src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public static void PassThroughSinglePacket(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -67,7 +67,7 @@ public static void PassThroughMultiplePacket(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -81,7 +81,7 @@ public static void PassThroughMultiplePacketWithShortEnd(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -96,7 +96,7 @@ public static void ReconstructSinglePacket(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -115,7 +115,7 @@ public static void Reconstruct2Packets_Part_PartFull(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -134,7 +134,7 @@ public static void Reconstruct2Packets_Full_FullPart_Part(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -154,7 +154,7 @@ public static void ReconstructMultiplePacketSequence(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -173,7 +173,7 @@ public static void ReconstructMultiplePacketSequenceWithShortEnd(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalTheory(nameof(IsUsingModernProcessSni)), MemberData(nameof(IsAsync))]
Expand All @@ -189,7 +189,7 @@ public static void Reconstruct3Packets_PartPartPart(bool isAsync)

var output = MultiplexPacketList(isAsync, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalFact(nameof(IsUsingModernProcessSni))]
Expand All @@ -208,7 +208,7 @@ public static void TrailingPartialPacketInSnapshotNotDuplicated()

var output = MultiplexPacketList(true, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ConditionalFact(nameof(IsUsingModernProcessSni))]
Expand Down Expand Up @@ -252,7 +252,7 @@ public static void MultipleFullPacketsInRemainderAreSplitCorrectly()

var output = MultiplexPacketList(false, dataSize, input);

ComparePacketLists(dataSize, expected, output);
ComparePacketLists(expected, output);
}

[ExcludeFromCodeCoverage]
Expand All @@ -269,16 +269,14 @@ private static List<PacketData> MultiplexPacketList(bool isAsync, int dataSize,

if (stateObject._inBytesRead > 0)
{
if (
stateObject._inBytesRead < TdsEnums.HEADER_LEN
||
stateObject._inBytesRead != (TdsEnums.HEADER_LEN +
Packet.GetDataLengthFromHeader(
stateObject._inBuff.AsSpan(0, TdsEnums.HEADER_LEN)))
)
{
Assert.Fail("incomplete packet exposed after call to ProcessSniPacket");
}
// At least the header must be read
Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN);

// The full packet must be read, too
Span<byte> header = stateObject._inBuff.AsSpan(0, TdsEnums.HEADER_LEN);
int packetLength = Packet.GetDataLengthFromHeader(header);
int expectedLength = TdsEnums.HEADER_LEN + packetLength;
Assert.Equal(expectedLength, stateObject._inBytesRead);

if (!isAsync)
{
Expand All @@ -299,17 +297,14 @@ private static List<PacketData> MultiplexPacketList(bool isAsync, int dataSize,

if (stateObject._inBytesRead > 0)
{
if (
stateObject._inBytesRead < TdsEnums.HEADER_LEN
||
stateObject._inBytesRead != (TdsEnums.HEADER_LEN +
Packet.GetDataLengthFromHeader(
stateObject._inBuff.AsSpan(0, TdsEnums.HEADER_LEN)))
)
{
Assert.Fail(
"incomplete packet exposed after call to ProcessSniPacket with usePartialPacket");
}
// Header must at least have been read
Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN);
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion logic is correct but the comment is misleading. The comment says "Header must at least have been read" which implies the assertion should ensure '_inBytesRead >= TdsEnums.HEADER_LEN', but Assert.False on a '<' check means we're asserting '_inBytesRead >= TdsEnums.HEADER_LEN', which is correct. However, using Assert.True with '>=' would be clearer and more consistent with the comment.

Suggested change
Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN);
Assert.True(stateObject._inBytesRead >= TdsEnums.HEADER_LEN);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@benrr101 benrr101 Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't have it both ways, my dude. Assert false is what you asked for above, so you will get both scenarios using Assert false.


// Full packet must have been read
Span<byte> header = stateObject._inBuff.AsSpan(0, TdsEnums.HEADER_LEN);
int packetLength = Packet.GetDataLengthFromHeader(header);
int expectedLength = TdsEnums.HEADER_LEN + packetLength;
Assert.Equal(expectedLength, stateObject._inBytesRead);

output.Add(PacketData.Copy(stateObject._inBuff, stateObject._inBytesUsed,
stateObject._inBytesRead));
Expand All @@ -326,23 +321,18 @@ private static List<PacketData> MultiplexPacketList(bool isAsync, int dataSize,
}

[ExcludeFromCodeCoverage]
private static void ComparePacketLists(int dataSize, List<PacketData> expected, List<PacketData> output)
private static void ComparePacketLists(List<PacketData> expected, List<PacketData> output)
{
Assert.NotNull(expected);
Assert.NotNull(output);
Assert.Equal(expected.Count, output.Count);

for (int index = 0; index < expected.Count; index++)
{
var a = expected[index];
var b = output[index];

var compare = a.AsSpan().SequenceCompareTo(b.AsSpan());

if (compare != 0)
{
Assert.Fail($"expected data does not match output data at packet index {index}");
}
Span<byte> a = expected[index].AsSpan();
Span<byte> b = output[index].AsSpan();

Assert.True(a.SequenceEqual(b), $"Packet data was not equal at index {index}");
Comment on lines +332 to +335
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison logic using SequenceCompareTo has been changed to SequenceEqual. While both can validate equality, SequenceCompareTo returns an integer (0 for equal sequences), whereas SequenceEqual returns a boolean directly. The new implementation correctly uses SequenceEqual with Assert.True, which is more idiomatic and readable. However, the assertion message format has changed from using Assert.Fail with string interpolation to Assert.True with a message parameter. While both work, consider using Assert.Equal with custom comparer or Assert.True consistently across similar test patterns.

Copilot uses AI. Check for mistakes.
}
}

Expand Down
Loading
Loading