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

use pytest #18

Merged
merged 20 commits into from
Mar 15, 2023
Merged

use pytest #18

merged 20 commits into from
Mar 15, 2023

Conversation

carueda
Copy link
Collaborator

@carueda carueda commented Jan 31, 2023

Locally, once pytest is installed:

pip3 install pytest

(already part of requirements.txt), one can then run pytest tests or python -m pytest tests, the latter being the form used in CI. Since pytest supports unittest, the initial commit here is just to run the tests via pytest (without changes in the test code).

in this initial commit.
@carueda carueda marked this pull request as draft January 31, 2023 00:20
note, changes to minimize shame history, but this could be further simplified by removing the class and @staticmethod stuff
@carueda
Copy link
Collaborator Author

carueda commented Jan 31, 2023

A 2nd commit with just minimal adjustments to test_utils, which is a good case due to not using setUp, just direct test_ methods.

Also, I marked this PR as draft because it's initially just intended to demonstrate what the transition to pytest would entail, in particular, I'd like to add some test exercising snaphots, so we can then discuss whether to eventually incorporate or not.

@carueda
Copy link
Collaborator Author

carueda commented Jan 31, 2023

A 3rd commit with a new test exercising snapshot based comparison.

@carueda carueda mentioned this pull request Jan 31, 2023
@carueda
Copy link
Collaborator Author

carueda commented Jan 31, 2023

ok, tests are now passing after doing some adjustments to the snapshot based comparison, in particular, with the use of DataFrame.round() to make the comparison consistent across the various environments (it all started passing for me locally, but not at CI).

Note: had to skip the test_utils tests due to unexpected failed asserts. Afaict, except for the reorganized structure, these tests are exactly as in master, so not sure what may be going on. We will need to revisit these.

@cparcerisas
Copy link
Collaborator

Yes in test_utils the assert fails because I haven't changed the ground truth results to the correct ones. It will be included under #13

@carueda
Copy link
Collaborator Author

carueda commented Feb 8, 2023

I'm manually adjusting things in this PR's branch according to adjustments seen in master ... but will likely re-create the PR given the various (expected) conflicts. If/when deciding that migration to pytest+snapshots is worth pursuing, then we would just need some little coordination to proceed with a merge while avoiding future conflicts.

# Conflicts:
#	tests/test_utils.py
@carueda
Copy link
Collaborator Author

carueda commented Feb 8, 2023

Actually the conflicts were trivial to resolve. This (draft) PR is now up-to-date.

@carueda carueda marked this pull request as ready for review February 10, 2023 01:16
@carueda
Copy link
Collaborator Author

carueda commented Feb 10, 2023

Thanks again @cparcerisas for one more great discussion earlier today (PST). Per that conversation, I shall be doing some further adjustments here.

Copy link
Collaborator

@danellecline danellecline left a comment

Choose a reason for hiding this comment

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

These look fine to me, but I'm unfamiliar with PyPam. Maybe better to have @cparcerisas review too. Suggest checking not just the formatting of the output, but also the content of the dataframe/csv, e.g. first rows.

@carueda
Copy link
Collaborator Author

carueda commented Mar 14, 2023

Thanks @danellecline!

Comparison against content is actually the core objective with snapshot based testing (formatting hasn't been a concern here).

Clea and I already went through this, but I want to have one more look myself at some point (hopefully soon) to then do one more quick coordination and merge.

@cparcerisas cparcerisas merged commit a2dcf09 into lifewatch:master Mar 15, 2023
@carueda carueda deleted the use_pytest branch March 15, 2023 16:15
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.

3 participants