Skip to content
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

Merged
merged 51 commits into from
Sep 25, 2023
Merged

Conversation

devdanzin
Copy link
Collaborator

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.

@devdanzin devdanzin changed the title Add unit tests for some commands WIP: Add unit tests for some commands Jul 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d0ad384) 95.76% compared to head (1a55a05) 95.76%.

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           
Files Changed Coverage Δ
src/wily/commands/rank.py 96.66% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devdanzin devdanzin changed the title WIP: Add unit tests for some commands Add unit tests for some commands Sep 5, 2023
):
rank(
config=mock_config,
path=None,
Copy link
Owner

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 :-)

Copy link
Collaborator Author

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.

test/unit/util.py Outdated Show resolved Hide resolved
@tonybaloney
Copy link
Owner

Please rename the files, they don't need to be called x_unit.py they're already in a unit folder

@devdanzin
Copy link
Collaborator Author

Thanks for reviewing this!

Unfortunately, when I renamed the files to avoid using "_unit", pytest threw errors like:

______________________________________ ERROR collecting test/unit/test_graph.py _______________________________________
import file mismatch:
imported module 'test_graph' has this __file__ attribute:
  wily\test\integration\test_graph.py
which is not the same as the test file we want to collect:
  wily\test\unit\test_graph.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

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.

@tonybaloney
Copy link
Owner

Thanks for reviewing this!

Unfortunately, when I renamed the files to avoid using "_unit", pytest threw errors like:

______________________________________ ERROR collecting test/unit/test_graph.py _______________________________________
import file mismatch:
imported module 'test_graph' has this __file__ attribute:
  wily\test\integration\test_graph.py
which is not the same as the test file we want to collect:
  wily\test\unit\test_graph.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

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

@tonybaloney tonybaloney merged commit dbe9172 into tonybaloney:master Sep 25, 2023
18 checks passed
@devdanzin devdanzin deleted the unittests branch September 25, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants