-
Notifications
You must be signed in to change notification settings - Fork 232
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: Improved parametrization #964
Conversation
Reviewer's Guide by SourceryThis pull request improves test parameterization by converting test fixtures to use NamedTuple classes. This change enhances test readability, maintainability, and type safety across multiple test files. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #964 +/- ##
==========================================
- Coverage 73.11% 73.06% -0.06%
==========================================
Files 26 26
Lines 1856 1856
Branches 352 352
==========================================
- Hits 1357 1356 -1
- Misses 395 396 +1
Partials 104 104 ☔ View full report in Codecov by Sentry. |
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.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- The new test fixtures are a great way to organize the test data and make the tests more readable.
- Consider adding a short description to each test fixture to explain its purpose.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3f418be
to
82ee5cd
Compare
82ee5cd
to
2ff09e3
Compare
@sourcery-ai review |
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.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief explanation of why NamedTuple was chosen over other alternatives like dataclasses.
- The test IDs are very helpful, but ensure they are descriptive and follow a consistent naming convention.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
74c385a
to
cab166e
Compare
afdc1a8
to
54e82c5
Compare
026201e
to
8d1a656
Compare
@sourcery-ai review |
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.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- This is a great refactor - the tests are much easier to read now.
- Consider adding a helper function to reduce the boilerplate when defining the test ID.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
This PR converts all remaining pytest parametrized tests to use NamedTuple fixtures instead of raw tuples. This improves test readability, maintainability, and type safety.
Changes
Converted all parametrized tests to use NamedTuple fixtures:
test_cli.py
HelpTestFixture
for help command teststest_help
to use named parameterstest_convert.py
ConvertTestFixture
for YAML conversion teststest_convert
to use named parametersConvertJsonTestFixture
for JSON conversion teststest_convert_json
to use named parameterstest_freeze.py
FreezeTestFixture
for freeze command teststest_freeze
to use named parametersFreezeOverwriteTestFixture
for force overwrite teststest_freeze_overwrite
to use named parameterstest_import.py
ImportTestFixture
for basic import teststest_import
to use named parametersImportTeamocilTestFixture
for teamocil import teststest_import_teamocil
to use named parametersImportTmuxinatorTestFixture
for tmuxinator import teststest_import_tmuxinator
to use named parameterstest_load.py
ZshAutotitleTestFixture
for ZSH auto title warning teststest_load_zsh_autotitle_warning
to use named parametersLogFileTestFixture
for log file teststest_load_log_file
to use named parametersPluginVersionTestFixture
for plugin version teststest_load_plugins_version_fail_skip
to use named parameterstest_load_plugins_version_fail_no_skip
to use named parametersPluginMissingTestFixture
for missing plugin teststest_load_plugins_plugin_missing
to use named parametersEach conversion includes:
Benefits
Example
Before:
After:
Testing
Related
Continues the work of standardizing test fixtures across the codebase, following the pattern established in files like
test_builder.py
andtest_shell.py
.Summary by Sourcery
Refactor existing parametrized tests to use NamedTuple test fixtures, improving readability and maintainability. Add tests for missing scenarios like nonexistent targets and interactive shell in shell command tests. Parametrize workspace finder tests to cover various path scenarios and workspace load tests to cover different enter and sleep behaviors.
Tests: