-
Notifications
You must be signed in to change notification settings - Fork 310
Port #3399 to release/5.1 #3409
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/5.1
Are you sure you want to change the base?
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.
Pull Request Overview
Ports the UTF-8 bulk copy fix (#3399) to the 5.1 release branch and adds a manual test to verify UTF-8 behavior during SqlBulkCopy.
- Introduces a
s_utf8EncodingWithoutBom
static field inTdsParser
and replaces allEncoding.UTF8
usages for UTF-8 collations to prevent emitting a BOM. - Adds a new
CreateTable
helper inDataTestUtility
and a manual testTestBulkCopyWithUTF8.cs
to validate that UTF-8 data round-trips correctly. - Updates the manual tests project file to include the new test.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/*/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs | New manual test class for UTF-8 bulk copy |
src/*/ManualTesting.Tests.csproj | Includes the new test file in the test project |
src/*/DataCommon/DataTestUtility.cs | Adds CreateTable helper for manual tests |
src/*/netfx/src/*/TdsParser.cs and netcore/src/*/TdsParser.cs | Introduces s_utf8EncodingWithoutBom and updates encoding logic |
Comments suppressed due to low confidence (3)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:2
- This using directive is unused in the test. Consider removing it to keep the imports clean.
using Microsoft.Extensions.Options;
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/TestBulkCopyWithUTF8.cs:13
- [nitpick] The class name casing (
Utf8
) does not match the file name (UTF8
). Rename either the file toTestBulkCopyWithUtf8.cs
or the class toTestBulkCopyWithUTF8
for consistency.
public sealed class TestBulkCopyWithUtf8 : IDisposable
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs:473
- The new helper method
CreateTable
lacks XML documentation. Consider adding a<summary>
and parameter descriptions to match the style of the existing utility methods.
public static void CreateTable(SqlConnection sqlConnection, string tableName, string createBody)
catch (Exception ex) | ||
{ | ||
// If bulk copy fails, fail the test with the exception message | ||
Assert.Fail($"Bulk copy failed: {ex.Message}"); |
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.
Xunit does not provide Assert.Fail
. Replace with Assert.True(false, ...)
or another Xunit-friendly assertion to fail the test with the exception message.
Assert.Fail($"Bulk copy failed: {ex.Message}"); | |
Assert.True(false, $"Bulk copy failed: {ex.Message}"); |
Copilot uses AI. Check for mistakes.
catch (Exception ex) | ||
{ | ||
// If bulk copy fails, fail the test with the exception message | ||
Assert.Fail($"Bulk copy failed: {ex.Message}"); |
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.
Xunit does not provide Assert.Fail
. Replace with Assert.True(false, ...)
or another Xunit-friendly assertion to fail the test with the exception message.
Assert.Fail($"Bulk copy failed: {ex.Message}"); | |
Assert.True(false, $"Bulk copy failed: {ex.Message}"); |
Copilot uses AI. Check for mistakes.
public static void CreateTable(SqlConnection sqlConnection, string tableName, string createBody) | ||
{ | ||
DropTable(sqlConnection, tableName); | ||
string tableCreate = "CREATE TABLE " + tableName + createBody; |
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.
[nitpick] It may be clearer to interpolate or explicitly include a space before the column definition, e.g.: var tableCreate = $"CREATE TABLE {tableName} {createBody}"
.
string tableCreate = "CREATE TABLE " + tableName + createBody; | |
string tableCreate = $"CREATE TABLE {tableName} {createBody}"; |
Copilot uses AI. Check for mistakes.
f2392d8
to
1ccd0fa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/5.1 #3409 +/- ##
================================================
- Coverage 71.78% 55.77% -16.02%
================================================
Files 293 293
Lines 61647 61649 +2
================================================
- Hits 44255 34385 -9870
- Misses 17392 27264 +9872
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Ports #3399 to release/5.1
Issues
#3397
Testing
Port contains the testcase to validate the fix