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

convert schemaview unittests to pytest #343

Merged
merged 9 commits into from
Oct 14, 2024
Merged

Conversation

sierra-moxon
Copy link
Member

No description provided.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.43%. Comparing base (cf9ada1) to head (1b5e9f7).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
- Coverage   63.62%   63.43%   -0.20%     
==========================================
  Files          63       63              
  Lines        8966     8982      +16     
  Branches     2572     2574       +2     
==========================================
- Hits         5705     5698       -7     
- Misses       2643     2665      +22     
- Partials      618      619       +1     

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

Copy link
Contributor

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great!

I'd just double check that make test isn't running the other unconverted tests twice.

Makefile Outdated
Comment on lines 13 to 14
poetry run python -m unittest discover
poetry run pytest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to remove poetry run python -m unittest discover. Pytest knows how to run unittest-based tests, so I believe what's here would end up running unittest-based tests twice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


def test_children_method(self):
def test_children_method():
view = SchemaView(SCHEMA_NO_IMPORTS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to redefine view in all these test functions since you made a fixture for it. Just include view or schema_view_no_imports as an argument to the function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, makes sense. Some of the tests redefine the import to test components of importing but not this. Fixed (I replaced the new SchemaView initialization with the fixture but left the view variable assignment in place because not doing so would mean many more test changes.)

@sierra-moxon sierra-moxon merged commit ad74966 into main Oct 14, 2024
18 checks passed
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.

2 participants