Separate static analysis, move tests out of the package

[flying-sheep]

This also fixes a few problems namely that tests are no modules, so you should use fixtures instead of importing.

If we want test tools that dependent packages can use we should create a submodule like scanpy.test_utils or so.

I forgot to add the new locations to tool.black.exclude, so the files got reformatted. I hope that’s OK?

[ivirshup]

I'm not sure this should be a separate job. I would prefer we use the jobs we have for other operating systems. Azure allows us to have multiple checks within a job, so I'm not sure it's that useful to put static analysis in it's own build.

[flying-sheep]

that way it runs much faster, so people see earlier if they didn’t format things properly

[ivirshup]

Why move the tests out of the package? It's what I've done in the past, but it certainly doesn't seem like the norm (pandas/tests, altair/tests, seaborn/tests, numba/tests, sklearn/tests)

I personally think fixtures in conftest.py is poor style (why would these things all go in one file? Why is this the one place which gets implicitly imported in all the tests?).

[flying-sheep]

Pytest architecture

tests directories and test_*.py collections aren’t intended to be packages, so you shouldn’t import from there. Just think about a module-level pytest.importorskip(...) or so. Also if there’s a __init__.py somewhere in your tests directory when using pytest, you’re doing something wrong. Yes, that means that all packages except for numba are doing it wrong, because numba also has importable test utils in there, and the others just misunderstand how pytest works. I blame setuptools, because find_packages finds directories that have __init__.py, so people just cargo-cultily started adding it without knowing what they’re doing.

Fixture visibility is hierarchical, so a conftest.py in a subfolder is able to override fixtures from higher-up.
So e.g. for testing a package/app that uses celery, you just define your own celery_config fixture, then start using the celery_app fixture, which will use your config automatically.

tests in the package

I think it’s a good idea to have it in there if you are a huge project and like to physically split up tests into multiple directiories that are close to the source code they test. (like numpy does it)

But as long as there’s only one tests directory with a structure that doesn’t neatly map to your module hierarchy, having it outside is cleaner because people can’t accidentally import from there. And as you can see from your links and our contributors importing stuff from test_* collections, a lot of projects don’t know how to use pytest, so making misuse harder is beneficial.

Also we have test data, which wastes space on every user’s machine and bandwidth for people installing scanpy.

tests and testing/test_utils

Given the above points, I think we should move things out to keep everything clean, and the package small.

I also like the separation of concerns: test_utils or testing (like numpy does) for reusable stuff that other projects depending on you might use and tests for actual tests. We can import the reusable fixtures in conftest.py

[flying-sheep]

I also just found this: https://docs.pytest.org/en/stable/pythonpath.html#import-modes

importlib: new in pytest-6.0, this mode uses importlib to import test modules.
[…]
makes test modules non-importable by each other.
[…]

We intend to make importlib the default in future releases.

[ivirshup]

I've thought about it a bit more, and now think I agree with having the static tests in a separate job. I would like if this could also add flake8 tests, and was setup so they would all run, regardless if any failed (continueOnError: 'true')

---

I don't think I agree with the rest, but am only going to give a partial response for now.

I'm not convinced we should move the tests out of the package. Broadly, I don't think pytest is a particularly opinionated testing tool, so I'm not sure one can use it wrong unless the tests aren't actually running. I do think their docs are not always clear/ correct. For example, we currently import from test modules https://github.com/theislab/scanpy/blob/8d9eec4c4763edb4a522dbec3fa5ea48832ff0f8/scanpy/tests/test_embedding_plots.py#L12

But:

isaac@Mimir:~/github/scanpy ‹master›
$ pytest --version
pytest 6.1.2
isaac@Mimir:~/github/scanpy ‹master›
$ pytest -n 6 --import-mode=importlib
...
================================ 587 passed, 17 skipped, 1 xfailed, 172 warnings in 84.39s (0:01:24) ================================

For good measure I also chucked a import scanpy.tests.test_embedding_plots into one of the test files and the tests still ran with --import-mode=importlib

[flying-sheep]

We should also move them out because of file size, I don’t think everyone should be forced to download all our test data when installing scanpy.

We should separate importable test tools (that e.g. other packages can import too) and our internal test tools.

We can then document the test tools

we currently import from test modules

yes, my PR fixes that

---

But even if you don’t fully agree with all of my arguments, there’s still arguments, and zero for not doing it.

Since there’s no obvious reason to not do it, why struggle to find any? We can just take the obvious advantages (however slight or non-slight they may be) and do it.

So is it OK if I go ahead and merge this before more PRs come in with conflicts? It’s getting a bit tiring to resolve those.

[ivirshup]

So is it OK if I go ahead and merge this before more PRs come in with conflicts?

No. There are already open PRs which I'm working on merging, and this will cause conflicts in those.

But even if you don’t fully agree with all of my arguments, there’s still arguments, and zero for not doing it.

I've only partially responded because I'm low on time. At first glance, there are a number of things I'm against here. But I'll be able to consider them more thoroughly, and tell you my arguments, once I've got more time – sometime after the 1.7.0 release.

[Zethson]

@ivirshup I'd actually be interest in hearing those. My packages also have the test folder outside the package, but I am happy to learn why many major packages have theirs in the actual package and why that might be a good idea.

[flying-sheep]

Me too, but we can sidestep the issue now with my plan in #2225