Add tests for package_search (#2092)#2149
Conversation
Signed-off-by: Ahlbertng <70105688+ahlbertng@users.noreply.github.com>
Signed-off-by: Ahlbertng <70105688+ahlbertng@users.noreply.github.com>
c11bd7b to
26ef581
Compare
Signed-off-by: Ahlbertng <70105688+ahlbertng@users.noreply.github.com>
Signed-off-by: Ahlbertng <70105688+ahlbertng@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2149 +/- ##
==========================================
+ Coverage 61.31% 61.72% +0.41%
==========================================
Files 164 164
Lines 20568 20568
Branches 3575 3575
==========================================
+ Hits 12611 12696 +85
+ Misses 7085 6983 -102
- Partials 872 889 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thank you for the submission of this new set of tests. We really appreciate any added coverage we can squeeze into the repo at this point, especially for such un-tested areas.
I've issued some specific comments attached to areas near the code that they pertain to. These are the main comments I'm making that would flip my recommendation from "don't merge" to "do merge". I made some specific line comments, but the real issue is that the current flake8 linter job fails, so just address those items.
I would express that the largest useful gaps to continue covering, whether they are a part of this PR, or you or anyone else tackles with additional work, would be these:
(Please comment, if you intend to go further with the coverage and aim to address these as well, or if you want to keep this PR self-contained just to the current change-set)
test the validate=True path. The ResourceSearcher.search() method has a substantial validate code path with error handling for ResourceContentError. No tests exercise this yet, and is arguably the most complex portion of the module.
test for glob patterns. ResourceSearcher.search() uses fnmatch.fnmatch on the name pattern. A test like searcher.search("py*") would verify the glob codepaths, which is a core feature of the searcher.
test for family resource type. When no version range is specified and multiple families match, the searcher returns resource_type="family". The current tests only exercise the package resource type.
| self.assertEqual(resource_type, "package") | ||
| self.assertEqual(len(results), 1) | ||
| self.assertEqual(results[0].resource.version, Version("3")) | ||
| def test_formatter_default_output(self) -> None: |
There was a problem hiding this comment.
Could use a newline between this test-def and the prior line, this will likely fail future-enforced linter rules (E305)
| self.assertEqual(len(formatted_lines), 1) | ||
| text, color = formatted_lines[0] | ||
| self.assertEqual(text, "pydad-3") | ||
| def test_resource_search_result(self) -> None: |
There was a problem hiding this comment.
(same, could use a newline between this and the line above)
| def tearDownClass(cls) -> None: | ||
| TempdirMixin.tearDownClass() | ||
|
|
||
| def test_reverse_dependency_tree(self) -> None: |
There was a problem hiding this comment.
get_reverse_dependency_tree tests don't verify the graph (g). The function returns a 2-tuple (pkgs_list, g) where g is a digraph. The tests unpack both but only assert on pkgs_list. The graph is arguably the more valuable output - it encodes the edge relationships. A test verifying g.nodes() and g.edges() would be more meaningful than just checking the list-of-lists.
| pkgs_list, g = get_reverse_dependency_tree("pymum") | ||
|
|
||
| self.assertEqual(pkgs_list[0], ["pymum"]) | ||
| self.assertEqual(pkgs_list[1], ["pydad", "pyson"]) |
There was a problem hiding this comment.
The assertion pkgs_list[1] == ["pydad", "pyson"] depends on the sorted order of the working set in package_search.py (sorted(list(working_set_))). That's correct today, but the test doesn't communicate why those two packages appear - it's because pymum-3 is required by pydad-3 and pyson-2, and the function only looks at the latest version of each family. If someone adds a new package that requires pymum to the fixture, or changes which version is latest, the assertion breaks with no obvious connection to the underlying logic. A set-based assertion (self.assertEqual(set(pkgs_list[1]), {"pydad", "pyson"})) would be both more robust and more honest about what the test is actually verifying - that the direct dependents are found - without coupling to sort order.
2 participants
Description
This PR adds unit tests for
src/rez/package_search.py, which currently has no test coverage (as noted in #2092).Addresses part of #2092.
What's tested
get_reverse_dependency_tree()Correctly builds a reverse dependency tree for a package with known direct dependents
Respects the
depthargument (e.g.depth=0returns only the root package)ResourceSearcher.search()Finds all versions of a package family
Returns only the latest version when
latest=TrueResourceSearchResultFormatterDefault (no
output_format) output correctly shows a package's qualified nameResourceSearchResultCorrectly stores
resource,resource_type, andvalidation_error(including defaultNone)Notes
Tests reuse the existing
solver/packagestest data already used bytest_packages.py, so no new test fixtures were added.get_plugins()is not yet covered, no existing test data currently includes packages withplugin_forset. Open to guidance on whether to add new fixture packages for this, or if there's a preferred approach.