Tests | Expand UDT serialization code coverage #3423

edwardneal · 2025-06-16T17:19:43Z

Description

I've been looking at a set of performance improvements and code health improvements to the serialization of CLR UDTs - SqlNormalizer.cs and SqlSer.cs. At present though, there isn't sufficient test coverage to prevent regressions. This PR widens that coverage, adding a set of new unit tests for the serialization and deserialization of CLR UDTs. By the end of this, I think we cover every reachable branch.

This is also the first set of additional tests in the new UnitTests project, and the first tests which test current library functionality. I've got no objection if anyone wants to start the new project with a consistent code style.

Issues

None.

Testing

All tests pass, all values match existing behaviour.

cheenamalhotra · 2025-06-16T23:29:38Z

/azp run

azure-pipelines · 2025-06-16T23:30:00Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski

Mostly style questions/comments.

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj

...lClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/NativeSerializationTest.cs

....Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/SerializedTypes.cs

...Client/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/InvalidSerializationTest.cs

...lClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/NativeSerializationTest.cs

...Client/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/InvalidSerializationTest.cs

...lClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/NativeSerializationTest.cs

...nt/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/UserDefinedSerializationTest.cs

Also enforce this via editorconfig

Also mandating this by generating a documentation XML file

paulmedynski · 2025-06-25T19:20:07Z

/azp run

azure-pipelines · 2025-06-25T19:20:30Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2025-06-27T20:48:51Z

/azp run

azure-pipelines · 2025-06-27T20:49:13Z

Azure Pipelines successfully started running 2 pipeline(s).

codecov · 2025-07-02T14:04:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.16%. Comparing base (1765de5) to head (a868f5e).
⚠️ Report is 3 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (1765de5) and HEAD (a868f5e). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (1765de5) HEAD (a868f5e)

addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3423      +/-   ##
==========================================
- Coverage   69.61%   61.16%   -8.46%     
==========================================
  Files         276      270       -6     
  Lines       61714    61398     -316     
==========================================
- Hits        42964    37554    -5410     
- Misses      18750    23844    +5094

Flag	Coverage Δ
addons	`?`
netcore	`64.27% <ø> (-9.03%)`	⬇️
netfx	`63.08% <ø> (-5.17%)`	⬇️

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.

edwardneal · 2025-07-21T21:33:53Z

I've merged main, with the unreachable code paths removed. Could someone re-run CI so we can verify the up-to-date coverage please?

benrr101

Glad to have more tests! A handful of requests, but nothing too crazy

...lient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs

src/Microsoft.Data.SqlClient/tests/UnitTests/InternalsVisibleToTest.cs

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj

...lClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/NativeSerializationTest.cs

src/Microsoft.Data.SqlClient/tests/UnitTests/.editorconfig

* Switch to using TheoryData * Nit: => movement * Rename tests * Split delegate for Assert.Throws into separate arrange/act/assert segment of each method

benrr101

Would still like to see the comment requirement backed out, use of the TheoryData (without analyzer enforcement). The rest is just if you feel like it :)

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj

...lClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/NativeSerializationTest.cs

....Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtSerialization/SerializedTypes.cs

paulmedynski · 2025-09-12T19:14:31Z

/azp run

azure-pipelines · 2025-09-12T19:14:54Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2025-09-16T13:20:20Z

/azp run

azure-pipelines · 2025-09-16T13:20:40Z

Azure Pipelines successfully started running 2 pipeline(s).

benrr101

Still going to request we remove the requirement for comments on unit tests. And will get back with results of poll for variable declaration style.

paulmedynski · 2025-09-17T18:52:03Z

/azp run

azure-pipelines · 2025-09-17T18:52:24Z

Azure Pipelines successfully started running 2 pipeline(s).

benrr101

Thanks for putting up with my requests 👍

Expand code coverage for UDT serialization

95e811e

edwardneal requested a review from a team as a code owner June 16, 2025 17:19

cheenamalhotra added the Area\Tests Issues that are targeted to tests or test projects label Jun 16, 2025

Address test failures on 32-bit OSes

21e5fd4

paulmedynski requested changes Jun 20, 2025

View reviewed changes

edwardneal added 7 commits June 20, 2025 19:30

Make Microsoft.SqlServer.Server a PackageReference

01f711f

Reformat SerializedTypes.cs

4db8883

Switch to implicit object creation

bd49514

Also enforce this via editorconfig

Add and correct comments

9f195c9

Also mandating this by generating a documentation XML file

Enable nullable for project

62edb70

Declare and assign variables together

f9136f9

Moved MemoryStream to an instance variable

6e80cc5

paulmedynski previously approved these changes Jun 25, 2025

View reviewed changes

edwardneal added 2 commits June 27, 2025 21:39

Merge branch 'main' into feat/udt-serialization/tests

60e074c

Post-merge build corrections

fa46855

edwardneal dismissed paulmedynski’s stale review via fa46855 June 27, 2025 20:47

paulmedynski previously approved these changes Jun 27, 2025

View reviewed changes

edwardneal mentioned this pull request Jun 29, 2025

Tests | Move various unit tests to UnitTests project #3458

Merged

edwardneal mentioned this pull request Jul 6, 2025

Cleanup | Remove unused UDT serialization code #3462

Merged

edwardneal added 2 commits July 21, 2025 22:23

Merge branch 'main' into feat/udt-serialization/tests

3f054c7

Reapply IDE0090 recommendations, disable CS1591 on newly-added tests

ab82654

edwardneal dismissed paulmedynski’s stale review via ab82654 July 21, 2025 21:32

paulmedynski previously approved these changes Jul 21, 2025

View reviewed changes

mdaigle self-assigned this Aug 26, 2025

paulmedynski self-assigned this Aug 28, 2025

benrr101 requested changes Sep 3, 2025

View reviewed changes

Next code review (first response)

b6e11f8

* Switch to using TheoryData * Nit: => movement * Rename tests * Split delegate for Assert.Throws into separate arrange/act/assert segment of each method

edwardneal dismissed paulmedynski’s stale review via b6e11f8 September 3, 2025 21:58

paulmedynski approved these changes Sep 4, 2025

View reviewed changes

benrr101 requested changes Sep 9, 2025

View reviewed changes

paulmedynski previously approved these changes Sep 12, 2025

View reviewed changes

edwardneal added 2 commits September 12, 2025 20:48

Merge branch 'main' into feat/udt-serialization/tests

67cbb7b

Disable CS1591 on new tests

ad7f07b

edwardneal dismissed paulmedynski’s stale review via ad7f07b September 12, 2025 19:54

paulmedynski previously approved these changes Sep 16, 2025

View reviewed changes

benrr101 self-assigned this Sep 16, 2025

benrr101 requested changes Sep 16, 2025

View reviewed changes

edwardneal added 3 commits September 17, 2025 06:44

Merge branch 'main' into feat/udt-serialization/tests

90d83ab

Disable mandatory XML documentation

61109eb

Change formatting of test data

643d8dc

edwardneal dismissed paulmedynski’s stale review via 643d8dc September 17, 2025 06:09

Remove trailing XML documentation override

a868f5e

paulmedynski approved these changes Sep 17, 2025

View reviewed changes

benrr101 approved these changes Sep 18, 2025

View reviewed changes

benrr101 merged commit 2655083 into dotnet:main Sep 18, 2025
236 checks passed

edwardneal deleted the feat/udt-serialization/tests branch September 18, 2025 17:01

Tests | Expand UDT serialization code coverage #3423

Tests | Expand UDT serialization code coverage #3423

Uh oh!

Conversation

edwardneal commented Jun 16, 2025

Description

Issues

Testing

Uh oh!

cheenamalhotra commented Jun 16, 2025

Uh oh!

azure-pipelines bot commented Jun 16, 2025

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Jun 25, 2025

Uh oh!

azure-pipelines bot commented Jun 25, 2025

Uh oh!

paulmedynski commented Jun 27, 2025

Uh oh!

azure-pipelines bot commented Jun 27, 2025

Uh oh!

codecov bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

edwardneal commented Jul 21, 2025

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Sep 12, 2025

Uh oh!

azure-pipelines bot commented Sep 12, 2025

Uh oh!

paulmedynski commented Sep 16, 2025

Uh oh!

azure-pipelines bot commented Sep 16, 2025

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

paulmedynski commented Sep 17, 2025

Uh oh!

azure-pipelines bot commented Sep 17, 2025

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Jul 2, 2025 •

edited

Loading