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

Split up tests? #5409

Open
max-sixty opened this issue May 31, 2021 · 4 comments
Open

Split up tests? #5409

max-sixty opened this issue May 31, 2021 · 4 comments

Comments

@max-sixty
Copy link
Collaborator

max-sixty commented May 31, 2021

Currently a large share of our tests are in test_dataset.py and test_dataarray.py — each of which are around 7k lines.

There's a case for splitting these up:

  • Many of the tests are somewhat duplicated between the files (and test_variable.py in some cases) — i.e. we're running the same test over a Dataset & DataArray, but putting them far away from each other in separate files. Should we instead have them split by "function"; e.g. test_rolling.py for all rolling tests?
  • My editor takes 5-20 seconds to run the linter and save the file. This is a very narrow complaint.
  • Now that we're all onto pytest, there's no need to have them in the same class.

If we do this, we could start on the margin — new tests around some specific functionality — e.g. join / rolling / reindex / stack (just a few from browsing through) — could go into a new respective test_{}.py file. Rather than some big copy and paste commit.

@keewis
Copy link
Collaborator

keewis commented May 31, 2021

that sounds like a good idea, and we'll definitely have to do this gradually because PRs with several thousand lines of changes are really difficult to review.

Probably unrelated, but I'd also like to refactor / reorder the tests in test_combine.py.

@shoyer
Copy link
Member

shoyer commented Jun 2, 2021

Yes, splitting up these giant test files would definitely be a good idea. test_backends.py might be another good candidate.

I don't think there was ever a good reason why they needed be in a single class. unittest works just fine with multiple test classes, too.

@max-sixty
Copy link
Collaborator Author

Great, it seems there's some backing for this. What's the best way to start?

I'm happy to do a PR to pull all the rolling tests into test_rolling.py as an example to start. I'm also happy to say any new rolling tests need to go into test_rolling.py, to minimize movement, as suggested above. But maybe that's hard to manage, and it's not obvious that's the policy for new contributors.

@TomNicholas
Copy link
Member

TomNicholas commented Jun 4, 2021

This is kind of a general question whenever we add new features: if I add to the API such that I can access my new computation newfunc via any of da.newfunc(), ds.newfunc() or xr.newfunc(), then should the tests live in test_dataarray.py, test_dataset.py, test_computation.py, all three of them, or make a new file test_newfunc.py? I agree we should move towards the latter, and try to just make test_dataarray.py & test_dataset.py test the core functionality of those objects, and not their extensive method APIs.

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

5 participants