Refactor check module and associated tests
#202
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6ec216f to
7b7b861
Compare
mknorps
reviewed
Mar 8, 2023
There was a problem hiding this comment.
Great that you took out package-related functions to a separate module!
The rest of the refactor following that change is consistent and a good improvement.
What I worry here about is that Analysis object starts becoming too complicated.
There are a lot of conditional statements, asserts and following what is happening in the case of the default `--check option in the tool is not straightforward.
I still have the tests section to check so for now I will leave this comment and the one in the code :)
These tests will be moved to a different module once we refactor code out of the check module. We also need to find some way to mock the LocalPackageLookup, so that we can isolate these tests from the current Python environment.
The new functions calculate_undeclared() and calculate_unused() perform the first and second half of what compare_imports_to_dependencies() used to do. For now they are simply called from compare_imports_to_dependencies(), but this they will soon replace it altogether.
After having reduced compare_imports_to_dependencies() to a thin wrapper for the tree underlying functions resolve_dependencies(), calculate_undeclared(), and calculate_unused(), we can now easily make the former obsolete by moving the three function calls into Analysis.create(). Removing compare_imports_to_dependencies() will be done in a later commit.
We will soon remove compare_imports_to_dependencies(), but we have a lot of parametrized tests for this function that we want to retain. Turn each test vector into a FDTestVector instance to make the definition of each vector more readable, and then use the same test data to test resolve_dependencies(), calculate_undeclared(), and calculate_unused() in addition to compare_imports_to_dependencies(). Based on Maria's great idea in [1]. [1]: #193 (comment)
No longer in use.
This moves: - DependenciesMapping and Package from fawltydeps.types - LocalPackageLookup and resolve_dependencies from fawltydeps.check into the new fawltydeps.packages module. On the tests/ side, we collect: - Package tests from test_types - LocalPackageLookup and resolve_dependencies() tests from test_map_dep_name_to_import_names - resolve_dependencies() tests from test_compare_imports_to_dependencies into the corresponding new test_packages modules. Since we reuse the FDTestVector test vectors from the test_compare_imports_to_dependencies in test_packages, these now move into the utils module, to be easily shared between test modules. Finally, when combining resolve_dependencies() tests from test_map_dep_name_to_import_names + test_compare_imports_to_dependencies some overlap in test cases was identified and eliminated. The test parameters to test_resolve_dependencies__focus_on_mappings() retain the deduplicated test parameters.
7b7b861 to
3914aea
Compare
mknorps
approved these changes
Mar 9, 2023
Co-authored-by: Maria Knorps <maria.knorps@tweag.io>
This makes failing tests document their inputs, which is helpful when debugging.
25767ae to
c9abe0a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly just moving code around to make our module structure reflect recent changes in our core logic. Also, more parametrized tests!
settings: Remove redundant/nested mutually exclusive argument grouptest_compare_imports_to_dependencies: Tests forresolve_dependencies()check: Refactorcompare_imports_to_dependencies()into two functionsmain: Call new functions instead ofcompare_imports_to_dependencies()test_compare_imports_to_dependencies: Refactor and add testscheck: Removecompare_imports_to_dependencies()fawltydeps.packages module