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

Fix our test layout #274

Open
njsmith opened this issue Aug 9, 2017 · 6 comments
Open

Fix our test layout #274

njsmith opened this issue Aug 9, 2017 · 6 comments

Comments

@njsmith
Copy link
Member

njsmith commented Aug 9, 2017

Right now our test layout is a mess. Tests are included in the package so that users can run pytest --pyargs trio to make sure it works on their system, but this prevents the use of pytest.ini or configuring extra arguments like --run-slow from conftest.py. We have tests split across trio/tests and trio/_core/tests, and since they're not subdirectories of each other then there's configury that has to be copy-pasted into their appropriate conftest.py files. (And we're not even consistent about this layout, b/c the tests for trio/testing/ are in trio/tests/test_testing.py.) Also these look like public modules, but they aren't really.

The major decision to make is, should we continue distributing tests inside trio? The advantage is that users can (perhaps) test the actual real installed version of trio, and they don't even have to set up some weird dev environment or anything. (Though they do need to install the test requirements, which aren't documented anywhere outside a source tree.) The main disadvantages are (a) we're swimming upstream a bit here, as noted by the issues with pytest.ini and friends, (b) it ~doubles the on-disk size of our installed package for a feature that only a tiny tiny fraction of deployments will use.

(Another commonly cited argument for separating tests out of your package is so that you can run the tests from version A against package version B. This doesn't make any sense to me, since our tests are unit tests that are tightly tied to a specific version of trio. It's not like we're implementing some generic 3rd party spec, and if we were then the compliance tests should be a standalone package anyway...)

How much extra disk space are we talking about? It's small, but less trivial than I was expecting. (I guess that 4 KiB minimum block size adds up.) On my laptop on 2017-08-09, building a wheel from trio master and then installing it in a venv, du reports that the .py and .pyc files take up 1.8 MB total, of which 779 KB is tests. This is not huge in the grand scheme of things, but it's not nothing, either. And in particular in these days of docker deployments, it seems likely that some of our users will be obsessed with making their docker containers smaller and get frustrated and angry about a megabyte of "waste". Maybe this makes sense, maybe not, but it doesn't really matter: making users frustrated and angry is a good thing to avoid no matter why it happens :-).

OTOH twisted does ship their tests inside their source tree, and twisted is massive: 27 MB installed, of which 15 MB is tests. And tornado and asyncio seem to as well. (Though distributors do often take special measures to delete CPython's test suite, which would include asyncio's test suite.)

If we do keep the tests inside trio/, then they should probably all move and be consolidated under trio/_tests/.

If we move them outside the source tree, then we probably have to switch to a layout like tests/ for tests, and src/trio for the actual package. (Explanation.) Which annoys me because it's an extra bit of friction every time you want to open a file, and I find it very handy to run tests or random little scripts directly against the source checkout. Like, tox is cool and all, but it slows things down, and when running locally I want to turn out tests ASAP, in-depth testing is what CI is for.

@njsmith
Copy link
Member Author

njsmith commented Aug 27, 2017

Looking at that pytest page again, I guess there's also the option of putting the tests in a top-level tests/, but not creating a tests/__init__.py. The limitation then is the "test files must have unique names", but... they're all in the same directory so that's not much of a limitation (subdirectories can have __init__.py files, so e.g. if we have tests/test_core/__init__.py and tests/test_core/test_run.py, then the latter will be imported as test_core.test_run, so it doesn't collide with any top-level test_run.py we might have). Also they can theoretically collide with other packages (b/c tests/ itself gets stuck in the front of sys.path), but it would be very weird to have a top-level Python package called test_.... (Aside from the stdlib test, of course, but we wouldn't collide with that and anyway I think pytest puts our tests in front of the stdlib in sys.path.) I guess we would need to be careful about names like tutil.py.

Meh. This is making me nervous, like we're swimming upstream here. And as noted above, literally all python eventloops ship their tests, as do other minor packages like setuptools. (pip doesn't, but it also doesn't use a src/ dir and it does have a tests/__init__.py so you can't test an installed pip at all...?) So I think I want to make sure that pytest --pyargs trio works reliably (like probably switch to using that in CI), and stick with that for now. And if this causes a problem for someone hopefully they'll let us know and we can reconsider.

@njsmith
Copy link
Member Author

njsmith commented Aug 27, 2017

Apparently pytest --pyargs trio --cov=trio actually works. Weird, but nice.

@sscherfke
Copy link

Have you read https://hynek.me/articles/testing-packaging/? Hynek has some good arguments for a layout with a docs, src and tests directory. It also has some beautiful symetry. ;-)

@njsmith
Copy link
Member Author

njsmith commented Jul 26, 2018

@sscherfke I am familiar with it, but I find that src/ adds some small but real overhead to operations I do dozens of times a day, like "look at a file" or "run the tests against my working directory" or "open a REPL with my working directory on PYTHONPATH". And all the issues he brings up there have other solutions – our CI already builds, installs, and runs the tests against the installed version on every PR; we already get combined coverage on CI runs through codecov, it's really only the CI runs that can give useful coverage because we need to combine across multiple operating systems. Well, I guess that doesn't address the aesthetics argument :-). But practicality beats purity...

@A5rocks
Copy link
Contributor

A5rocks commented May 24, 2023

With #2628 merged, we've moved trio/tests to trio/_tests. The only question remaining is: is this a good enough layout or do we want to move tests out of the package entirely?

@webknjaz
Copy link
Member

I suggest moving Codecov out of ci.sh: #2650.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants