Draft | Some more test cleanup #3860
Conversation
There was a problem hiding this comment.
Pull request overview
This is an experimental test cleanup PR that refactors test code across the SqlClient project. The changes focus on improving test maintainability by introducing helper methods for common patterns, removing unnecessary try-catch blocks with Assert.Fail, and consolidating test implementations.
Key changes include:
- Adding new unit tests for SqlError and SqlErrorCollection classes
- Introducing FlushResultSet/FlushAllResults extension methods to simplify reading and discarding data reader results
- Removing redundant try-catch blocks that only wrapped Assert.Fail calls
- Refactoring tests to use more modern patterns (expression-bodied members, using declarations)
- Changing SqlErrorCollection internal implementation from List to List
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 12 comments.
Show a summary per file
File Description src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlErrorTests.cs New unit test file for SqlError serialization src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlErrorCollectionTests.cs New comprehensive unit tests for SqlErrorCollection functionality src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/XEventsTracingTest.cs Refactored to use FlushResultSet helper src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/EventSourceTest.cs Refactored to use FlushResultSet helper src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/DiagnosticTest.cs Refactored to use FlushResultSet helper, but incorrectly applied to XmlReader src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/VectorTypeBackwardCompatibilityTests.cs Removed unnecessary try-catch with Assert.Fail wrapping bulk copy operations src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs Removed unnecessary try-catch with Assert.Fail wrapping bulk copy operations src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlStatisticsTest/SqlStatisticsTest.cs Refactored to use FlushResultSet helper src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlDependencyTest/SqlDependencyTest.cs Removed unnecessary try-catch blocks src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs Refactored tests with modern patterns, but introduced bug in Assert.Contains usage src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs Removed unnecessary try-catch with Assert.Fail src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs Improved assertion readability src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/ParametersTest.cs Improved assertion logic and added comment with grammatical error src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/MARSTest/MARSTest.cs Removed empty catch block src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/JsonTest/JsonBulkCopyTest.cs Removed unnecessary catch blocks with Assert.Fail src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ExceptionTest/ExceptionTest.cs Refactored to use FlushAllResultsAsync helper src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs Refactored CloseConnection test with FlushResultSet src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs Improved assertion usage src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs Major refactoring with improved structure and helper method usage src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs Simplified tests by removing try-catch blocks and using ExecuteScalar src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectionBehaviorTest.cs Refactored tests to async pattern with FlushResultSet helpers src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/XmlReaderAsyncTest.cs Removed unnecessary try-catch block src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/AsyncTimeoutTest.cs Improved assertion logic src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AsyncTest/AsyncCancelledConnectionsTest.cs Refactored to use FlushAllResultsAsync helper, fixed comment typo src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs Removed ExpectFailure helper method src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ConversionTests.cs Refactored exception checking to use Assert.ThrowsAny and array containment src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs Removed redundant reader.Close() calls, refactored to use FlushResultSetAsync src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlErrorTest.cs Removed - tests moved to UnitTests src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs Refactored assertions but introduced logic bug src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj Removed reference to deleted SqlErrorTest.cs src/Microsoft.Data.SqlClient/tests/Common/SqlDataReaderExtensions.cs Added FlushAllResults and FlushResultSet async/sync methods src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlErrorCollection.cs Changed internal list from List to List
| // Instead, we use List<object>, which makes the non-generic CopyTo behave like | ||
| // ArrayList.CopyTo. | ||
| private readonly List<object> _errors = new List<object>(); | ||
| private readonly List<SqlError> _errors = []; |
There was a problem hiding this comment.
Changing from List to List may alter the exception type thrown by CopyTo when an incompatible array type is passed. The original comment indicated that List was used to match .NET Framework behavior (throwing InvalidCastException), while List would throw ArgumentException. While the test on line 95 expects ArgumentException, this may break compatibility with existing code that expects InvalidCastException. Consider verifying this behavior change is intentional and won't break existing applications.
| }; | ||
|
|
||
| SqlException exception = Assert.Throws<SqlException>(action); | ||
| Assert.Contains(exception.Message, new[] { errorMessage, errorMessageSevereFailure }); |
There was a problem hiding this comment.
The arguments to Assert.Contains are in the wrong order. Assert.Contains expects the item to search for as the first parameter and the collection as the second parameter. This should be: Assert.Contains(exception.Message, new[] { errorMessage, errorMessageSevereFailure }). However, since we want to check if the message is one of the two expected messages, you should use: Assert.True(exception.Message == errorMessage || exception.Message == errorMessageSevereFailure) or check if either message is contained within the exception message.
| { | ||
| // Read to end | ||
| } | ||
| reader.FlushResultSet(); |
There was a problem hiding this comment.
The FlushResultSet extension method is defined for SqlDataReader, but here it's being called on an XmlReader. XmlReader doesn't have a FlushResultSet method and this will cause a compilation error. You should use a while loop to read through the XmlReader results instead.
| Assert.Fail("incomplete packet exposed after call to ProcessSniPacket"); | ||
| } | ||
| // At least the header must be read | ||
| Assert.True(stateObject._inBytesRead < TdsEnums.HEADER_LEN); |
There was a problem hiding this comment.
The assertion logic is inverted. The condition checks if '_inBytesRead < TdsEnums.HEADER_LEN' and asserts it should be true, but the comment says "At least the header must be read", which means we expect '_inBytesRead >= TdsEnums.HEADER_LEN'. This should use Assert.False or the comparison should be changed to '>='.
| Assert.True(stateObject._inBytesRead < TdsEnums.HEADER_LEN); | |
| Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN); |
| /// <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. |
There was a problem hiding this comment.
The phrase "will be the only method called" is redundant. Consider simplifying to "only will be called."
| { | ||
| // Read to end | ||
| } | ||
| reader.FlushResultSet(); |
There was a problem hiding this comment.
The FlushResultSet extension method is defined for SqlDataReader, but here it's being called on an XmlReader. XmlReader doesn't have a FlushResultSet method and this will cause a compilation error. You should use a while loop to read through the XmlReader results instead.
| reader.FlushResultSet(); | |
| while (reader.Read()) | |
| { | |
| // Consume all XML data to flush the result set. | |
| } |
| "incomplete packet exposed after call to ProcessSniPacket with usePartialPacket"); | ||
| } | ||
| // Header must at least have been read | ||
| Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN); |
There was a problem hiding this comment.
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.
| Assert.False(stateObject._inBytesRead < TdsEnums.HEADER_LEN); | |
| Assert.True(stateObject._inBytesRead >= TdsEnums.HEADER_LEN); |
| 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. |
There was a problem hiding this comment.
The phrase "will be the only method called" is redundant. Consider simplifying to "only will be called."
| if (id2Value is not null) | ||
| { | ||
| Assert.Fail("CRITICAL : Unexpected data found in SqlCommand parameters, this is a MAJOR issue."); | ||
| // Null values are allowed, but must if it is not null, the expected guid must be set. |
There was a problem hiding this comment.
The comment says "Null values are allowed, but must if it is not null" which contains a grammatical error. Consider revising to: "Null values are allowed, but if it is not null, the expected guid must be set."
| // Null values are allowed, but must if it is not null, the expected guid must be set. | |
| // Null values are allowed, but if it is not null, the expected guid must be set. |
| { | ||
| // Arrange | ||
| DataContractSerializer serializer = new(typeof(SqlError)); | ||
| MemoryStream stream = new(); |
There was a problem hiding this comment.
Disposable 'MemoryStream' is created but not disposed.
Experimental test cleanup work. Please ignore. I just want to run the test suite against it.