Skip to content

Tests | Move fixtures to common project #3402

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
merged 8 commits into from
Jun 24, 2025

Conversation

edwardneal
Copy link
Contributor

Description

This PR performs some simple cleanup of the tests. #3371 created a new Common project which contains common code for test projects. We add this project to the Visual Studio solution, and move the encryption-based fixtures to it. Indentation changes make up the vast majority of this PR...

I've also rolled one changes which caused headaches in #3204 by allowing Visual Studio to run tests against the net8.0 target. This caused some pain when working with certificates - the recommended API changed between net8.0 and net9.0.

There was also one piece of feedback on #3204 about preferring string interpolation. This was fixed in one place, but there was another instance of it. I've made the spot-fix.

Issues

Relates to #3371 and #3204.

Testing

All automated tests continue to pass, on all three targets.

@edwardneal edwardneal requested a review from a team as a code owner June 7, 2025 21:16
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski
Copy link
Contributor

Hey @edwardneal - Thanks for starting to clean up some of these test helpers.

My goal with the Common test project was for it to contain things that are shared between other test projects. I see that some of the fixtures you moved are only used in a single test project, so maybe they're better off moving into those projects?

  • AzureKeyVaultKeyFixtureBase - used only in ManualTests.
  • CertificateFixtureBase - used in FunctionalTests and ManualTests, so it belongs in Common as you have moved it.
  • ColumnEncryptionCertificateFixture - used only in FunctionalTests.
  • ColumnMasterKeyCertificateFixture - used only in ManualTests.
  • CspCertificateFixture - used only in ManualTests.

I see that ManualTests already has a TestFixtures sub-directory. Maybe FunctionalTests needs the same (preferrably both named just Fixtures)?

I will discuss with the team and report back before asking for any changes here.

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.

Marking my review progress.

@edwardneal
Copy link
Contributor Author

Thanks @paulmedynski. The tests are in an odd middle ground at the moment - I've moved the fixtures from one "common-ish" project to the purpose-built Common project, but introducing a convention of a project-specific Fixtures namespace would work just as well for our purposes.

It highlights some slightly odd test placement: I'd have expected some functional tests for CspCertificateFixture and AzureKeyVaultKeyFixtureBase to make sure they can round-trip values. It might be a good idea to keep these two fixtures in Common, ready for some future test restructuring.

paulmedynski
paulmedynski previously approved these changes Jun 11, 2025
@paulmedynski
Copy link
Contributor

I discussed with the team, and we're ok with these moves. We can make further changes once the project merging is complete.

@paulmedynski paulmedynski added this to the 6.1-preview2 milestone Jun 11, 2025
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski
paulmedynski previously approved these changes Jun 12, 2025
Copy link

codecov bot commented Jun 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.16%. Comparing base (95253ac) to head (15c5860).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3402      +/-   ##
==========================================
- Coverage   67.21%   62.16%   -5.06%     
==========================================
  Files         220      288      +68     
  Lines       45629    64912   +19283     
==========================================
+ Hits        30671    40354    +9683     
- Misses      14958    24558    +9600     
Flag Coverage Δ
netcore 67.02% <ø> (?)
netfx 60.67% <ø> (-6.55%) ⬇️

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.

@cheenamalhotra
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@cheenamalhotra cheenamalhotra added the Area\Tests Issues that are targeted to tests or test projects label Jun 24, 2025
@cheenamalhotra cheenamalhotra merged commit 1fadbd9 into dotnet:main Jun 24, 2025
237 checks passed
@edwardneal edwardneal deleted the tests/common-project-fixtures branch June 24, 2025 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Tests Issues that are targeted to tests or test projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants