Tests | Move fixtures to common project

[edwardneal]

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.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[paulmedynski]

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.

[paulmedynski]

Marking my review progress.

[edwardneal]

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]

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

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[codecov]

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]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).