Skip to content

Conversation

apoorvdeshmukh
Copy link
Contributor

Description

Ports #3399 to release/5.1

Issues

#3397

Testing

Port contains the testcase to validate the fix

@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 06:30
Copy link
Contributor

@Copilot Copilot AI left a 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 in TdsParser and replaces all Encoding.UTF8 usages for UTF-8 collations to prevent emitting a BOM.
  • Adds a new CreateTable helper in DataTestUtility and a manual test TestBulkCopyWithUTF8.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 to TestBulkCopyWithUtf8.cs or the class to TestBulkCopyWithUTF8 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)

Copy link

codecov bot commented Jun 11, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.18%. Comparing base (6af24bc) to head (ab72f93).
⚠️ Report is 2 commits behind head on release/5.1.

Files with missing lines Patch % Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 60.00% 2 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 60.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6af24bc) and HEAD (ab72f93). Click for more details.

HEAD has 42 uploads less than BASE
Flag BASE (6af24bc) HEAD (ab72f93)
netfx 15 1
netcore 15 1
addons 15 1
Additional details and impacted files
@@               Coverage Diff                @@
##           release/5.1    #3409       +/-   ##
================================================
- Coverage        71.83%   42.18%   -29.65%     
================================================
  Files              293      293               
  Lines            61647    61649        +2     
================================================
- Hits             44283    26008    -18275     
- Misses           17364    35641    +18277     
Flag Coverage Δ
addons 0.00% <ø> (-92.39%) ⬇️
netcore 44.64% <60.00%> (-31.31%) ⬇️
netfx 40.99% <60.00%> (-28.57%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benrr101
benrr101 previously approved these changes Jun 11, 2025
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

CI test failures.

@paulmedynski paulmedynski added this to the 5.1.8 milestone Jul 16, 2025
@David-Engel David-Engel changed the title Port #3399 to release/5.1 Backport 5.1 | Use UTF8 instance that doesn't emit BOM Jul 16, 2025
@David-Engel David-Engel changed the title Backport 5.1 | Use UTF8 instance that doesn't emit BOM [5.1] Use UTF8 instance that doesn't emit BOM Jul 16, 2025
* Fix helpers not included correctly
@paulmedynski paulmedynski self-assigned this Sep 4, 2025
@apoorvdeshmukh
Copy link
Contributor Author

Too many merge conflicts. Raising new PR.

@apoorvdeshmukh apoorvdeshmukh deleted the dev/ad/rel51port3399 branch September 13, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants