-
Notifications
You must be signed in to change notification settings - Fork 310
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
Tests | Move fixtures to common project #3402
Conversation
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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?
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. |
There was a problem hiding this 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.
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 It highlights some slightly odd test placement: I'd have expected some functional tests for |
I discussed with the team, and we're ok with these moves. We can make further changes once the project merging is complete. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.