Open maxnus opened 1 year ago
All modified lines are covered by tests :white_check_mark:
Comparison is base (
511ef48
) 71.53% compared to head (1c01eae
) 71.53%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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?
LGTM otherwise :smile:
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
This PR moves the
tests
directory outside the package.In this PR:
--user
install in CI workflow (not sure if there was a good reason for it?).github/workflows/run_tests.py
- I think we don't need this file and we can put any pytest config items intoci.yaml
orpyproject.toml
.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.htmltestssytems
->systems
andTestMolecule
->MoleculeTestSystem
, etc. This is to prevent accidental collection (I believe the exact rules should betest_*.py
filename andTest*
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.__init__.py
files, other thantests/__init__.py
. This one is still needed, since we import fromtests/systems.py
andtests/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.if __name__ == '__main__'
clauses andunittest
imports. We have commited topytest
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.