-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add unit tests for some commands #199
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #199 +/- ##
=======================================
Coverage 95.76% 95.76%
=======================================
Files 25 25
Lines 1369 1370 +1
Branches 293 293
=======================================
+ Hits 1311 1312 +1
Misses 33 33
Partials 25 25
☔ View full report in Codecov by Sentry. |
): | ||
rank( | ||
config=mock_config, | ||
path=None, |
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.
path, limit and threshold are typed as being required arguments, so this test shouldn't even pass :-)
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.
Done. path
can indeed be None
, so I've updated the typing annotation to Optional[str]
. That happens because in __main__.py
we have:
@click.argument("path", type=click.Path(resolve_path=False), required=False)
The wrong typing on the test only "worked" because the actual call doesn't check for typing conformance and we don't run pyright on test files. I'll submit another PR that fixes the few other mistyped tests and enables pyright on the test directory.
Please rename the files, they don't need to be called |
…ing builtin name 'format'.
…o the same basenames as integration tests.
Thanks for reviewing this! Unfortunately, when I renamed the files to avoid using "_unit", pytest threw errors like:
Not sure whether it comes from pytest or the coverage plugin. I guess that's why we already had a build_unit.py file under test/unit, to avoid this kind of error. So I'm reverting the rename unless you'd prefer that I try to figure out how to support files with the same basenames. I've addressed your comments, let me know if any of the solutions needs improvement. |
ah yes, I forgot about that. Pytest requires that all files have unique names |
This PR adds unit tests for index, report, rank, graph and list-metrics. It should help land the JSON output PR (and any others touching output) without any regressions.
It's a bit rough in the edges, but I believe these are good to have. Instead of testing the output it would be possible to mock tabulate, but I feel checking the output works best.