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

Moving tests outside package #143

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

maxnus
Copy link
Contributor

@maxnus maxnus commented Sep 23, 2023

This PR moves the tests directory outside the package.

In this PR:

  • Remove --user install in CI workflow (not sure if there was a good reason for it?)
  • Removes .github/workflows/run_tests.py - I think we don't need this file and we can put any pytest config items into ci.yaml or pyproject.toml.
  • Moves tests directory outside package. This has some benefits, most importantly it's there is less of a danger of accidentally run the tests against the source files rather than the installed version. See also https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html
  • Renames testssytems -> systems and TestMolecule -> MoleculeTestSystem, etc. This is to prevent accidental collection (I believe the exact rules should be test_*.py filename and Test* class name and no __init__ method, however I saw some issues around the collection and it's best not to cut too close to the rules and make it confusing.
  • Deletes __init__.py files, other than tests/__init__.py. This one is still needed, since we import from tests/systems.py and tests/common.py and have used "--import-mode=importlib" (which is recommended, but makes imports across tests difficult). Ideally, I would like to move away from heavy cross-test importing and use a modern, fixture-based design. This will then reduce the issues around importing.
  • Removes if __name__ == '__main__' clauses and unittest imports. We have commited to pytest now, which is the de facto standard, let's not make it confusing by seemingly (but not really) support other modes of running the tests.

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (511ef48) 71.53% compared to head (1c01eae) 71.53%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   71.53%   71.53%           
=======================================
  Files         152      152           
  Lines       20159    20159           
  Branches     3343     3343           
=======================================
  Hits        14420    14420           
  Misses       4906     4906           
  Partials      833      833           

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

@obackhouse
Copy link
Contributor

Removes if name == 'main' clauses and unittest imports. We have commited to pytest now, which is the de facto standard, let's not make it confusing by seemingly (but not really) support other modes of running the tests.

I disagree with this - if we remove these clauses then we have to run python -m pytest test_x.py to run a single test file rather than python test_x.py? We also still use the unittest framework via unittest.TestCase which we subclass - mixing this with pytest is very common.

@obackhouse
Copy link
Contributor

Will there be any path issues with the tests now effectively belonging to a package named tests? If every package did this then it would be chaos, no?

@obackhouse
Copy link
Contributor

LGTM otherwise 😄

@maxnus
Copy link
Contributor Author

maxnus commented Sep 24, 2023

I disagree with this - if we remove these clauses then we have to run python -m pytest test_x.py to run a single test file rather than python test_x.py?

You can also type pytest test_x.py. But note that these do different things, python test_x.py does not run via pytest, which was exactly my point.

We also still use the unittest framework via unittest.TestCase which we subclass

Yes, we will need to get rid of that too, at least over time, since those subclasses cannot use fixtures and parametrization decorators, see: https://docs.pytest.org/en/latest/how-to/unittest.html
At the least, new tests should best not be written in unittest style anymore.

Will there be any path issues with the tests now effectively belonging to a package named tests? If every package did this then it would be chaos, no?

I don't see any issues. A lot of packages do this: https://github.com/pydantic/pydantic/tree/main/tests, https://github.com/tiangolo/fastapi/tree/master/tests

@maxnus maxnus requested a review from ghb24 October 4, 2023 18:29
@obackhouse obackhouse removed their request for review October 2, 2024 07:18
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