Skip to content

Tests | Move various unit tests to UnitTests project #3458

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

Merged

Conversation

edwardneal
Copy link
Contributor

Description

This is another PR which is a little smaller than it looks. It locates some of the smaller tests which made heavy use of reflection to access internal SqlClient types, moves them from FunctionalTests to UnitTests, removes the reflection and adds descriptions to the test cases. The steps are visible if reviewed commit-by-commit.

Besides some basic test movements, it also lays the groundwork for expanding the test coverage of SqlBuffer. At the moment, we only have explicit coverage of Guid and SqlGuid. A future PR will cover the other types.

There's a little overlap between this and #3423; I've tried to maintain the same standards as there, and I'll revisit them if #3423 is merged first.

More broadly here, there are around 1200 tests in FunctionalTests. I've assumed that the new UnitTests project is eventually going to contain most of the tests in FunctionalTests, is that correct?

Issues

None.

Testing

All unit tests pass.

@edwardneal edwardneal requested a review from a team as a code owner June 29, 2025 19:47
@apoorvdeshmukh
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented Jul 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.08%. Comparing base (df35633) to head (82e60e4).
Report is 9 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (df35633) and HEAD (82e60e4). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (df35633) HEAD (82e60e4)
netfx 2 1
netcore 2 1
addons 2 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3458      +/-   ##
==========================================
- Coverage   66.91%   61.08%   -5.83%     
==========================================
  Files         280      274       -6     
  Lines       62386    62073     -313     
==========================================
- Hits        41745    37920    -3825     
- Misses      20641    24153    +3512     
Flag Coverage Δ
addons ?
netcore 63.38% <ø> (-5.59%) ⬇️
netfx 63.55% <ø> (-5.70%) ⬇️

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.

paulmedynski
paulmedynski previously approved these changes Jul 2, 2025
mdaigle
mdaigle previously approved these changes Jul 2, 2025
Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Most tests from FunctionalTests can move to UnitTests. We're trying to set a good example in the UnitTests project for how unit tests should behave without the cruft of the existing project. When adding new tests to this project or moving tests over, please do your best to adhere to [industry best practices]. A big piece of that will be removing all of the brittle reflection.

I do think we could go a bit farther removing reflection from SqlCommandSetTest. A lot of the tests are parameterized with type and property names that could get swapped for direct references.

@edwardneal
Copy link
Contributor Author

Thanks @mdaigle - that all makes sense. I'll put new tests there and port over older ones as I touch them.

I wasn't sure about fixing that part of the SqlCommandSetTests, but I've changed the visibility of those members from private to internal and removed the reflection.

@edwardneal edwardneal dismissed stale reviews from mdaigle and paulmedynski via af251dc July 2, 2025 21:59
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

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.

I think we should remove an unnecessary check in SqlCommandSet and the associated test.

mdaigle
mdaigle previously approved these changes Jul 3, 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.

Looks great - and one less test to worry about :)

@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski requested a review from mdaigle July 9, 2025 11:05
@mdaigle mdaigle merged commit ad01286 into dotnet:main Jul 9, 2025
237 checks passed
@edwardneal edwardneal deleted the tests/internal-type-functional-tests-pt1 branch July 9, 2025 17:59
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.

4 participants