Tests | SqlError, SqlErrorCollection #3869
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reorganizes test files by moving SqlError and SqlErrorCollection tests from the FunctionalTests project to the UnitTests project, making them truly isolated unit tests that don't require a SQL Server connection.
- Migrates SqlError serialization test from FunctionalTests to UnitTests with improved test structure
- Adds comprehensive SqlErrorCollection unit tests covering collection operations, indexing, enumeration, and copy operations
- Removes dependency on SQL Server connection for these tests by using internal constructors directly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlErrorTests.cs | New unit test file for SqlError with serialization round-trip test |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlErrorCollectionTests.cs | New comprehensive unit test file for SqlErrorCollection with tests for Add, CopyTo, GetEnumerator, and Indexer operations |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlErrorTest.cs | Removes old functional test file that required SQL Server connection |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj | Removes reference to deleted SqlErrorTest.cs file |
| { | ||
| // Arrange | ||
| DataContractSerializer serializer = new(typeof(SqlError)); | ||
| MemoryStream stream = new(); |
There was a problem hiding this comment.
The MemoryStream should be disposed after use to properly release resources. Consider wrapping it in a using statement or using declaration.
| MemoryStream stream = new(); | |
| using MemoryStream stream = new(); |
| exception: new Exception(), | ||
| batchIndex: 345); |
There was a problem hiding this comment.
Each test creates a new Exception object for the SqlError, which could be replaced with a shared static Exception instance or null if the exception value isn't being tested. This would make the tests more efficient and clearer about what's actually being tested.
| errorMessage: "bar", | ||
| procedure: "baz", | ||
| lineNumber: 234, | ||
| exception: new Exception(), |
There was a problem hiding this comment.
The Exception created here doesn't need to be a new instance since its actual content isn't being meaningfully tested. Consider using null or a shared static Exception instance to make it clearer that the exception value itself isn't the focus of this serialization test.
Description
Pulling together a few PRs from some test work I did over the holidays. This PR:
Issues
N/A
Testing
Tests run locally and seem to work in the draft runs I did previously