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

Speed up hypothesis tests #2650

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Jan 5, 2025

  • tests/test_store/test_stateful.py::test_zarr_hierarchy[memory]
  • tests/test_store/test_stateful.py::test_zarr_store[memory]

currently take up around 70% of our total test time (e.g., see this recent test run).

Since these tests are not comparing two different store implementations, but the same one (comparing MemoryStore to MemoryStore), I think it makes sense to cut the number of examples right down here to save a lot of testing time.

@dstansby dstansby changed the title Reduce number of examples in hypothesis test Speed up hypothesis tests Jan 5, 2025
@dstansby dstansby marked this pull request as ready for review January 5, 2025 12:03
@dstansby dstansby requested a review from dcherian January 5, 2025 12:03
@dstansby dstansby added the tests label Jan 5, 2025
@dcherian
Copy link
Contributor

dcherian commented Jan 6, 2025

5 examples is too small to be useful, and this time will just keep going up as we start testing the other stores.

What do you think of making a separate nightly action that runs the stateful tests, and auto-opens an issue on failure similar to this action I wrote for Xarray

@dstansby
Copy link
Contributor Author

dstansby commented Jan 6, 2025

What's the minimum number of examples to be useful?

My thinking was checking MemoryStore against MemoryStore was useful to make sure the test itself works, but it's just comparing instances of identical classes, so there's no benefit to running lots of tests, especially as it's taking up 70% of the total test time. Maybe I'm misunderstanding the tests though?

I'd be 👍 to doing a nightly action if there is good reason to run lots of examples for these tests.

@dcherian
Copy link
Contributor

dcherian commented Jan 6, 2025

I don't think 5 examples is useful for testing all the actions because it generates random sequences of actions and longer examples tend to come later in the run. Run pytest -s --hypothesis-verbosity=verbose to see how it tries things.

I agree that testing MemoryStore against MemoryStore seems silly, but this is on par for the property and stateful testing literature. Very simple and seemingly-silly tests (e.g. "get the same key twice", "set the same key twice, and get it") tend to be very effective particularly when you have some source of complexity (like async in Zarr's case).

My goal was to remove MemoryStore in favour of testing LocalStore against MemoryStore but LocalStore had more inconsistencies than I cared to fix at that point. Are you interested in trying this?

@dstansby
Copy link
Contributor Author

dstansby commented Jan 6, 2025

How about cuting down to 50 examples then? Even that would give us a big and quick win in terms of test times. Then in the medium term we could look at 1) a nighly run of the stateful tests, and 2) testing LocalStore against MemoryStore.

@dcherian
Copy link
Contributor

dcherian commented Jan 6, 2025

OK sounds good.

@dcherian dcherian enabled auto-merge (squash) January 6, 2025 18:57
@dcherian dcherian disabled auto-merge January 6, 2025 19:01
@dcherian
Copy link
Contributor

dcherian commented Jan 6, 2025

Sorry I had this all backwards. I have already done the nightly action: https://github.com/zarr-developers/zarr-python/actions/workflows/hypothesis.yaml

I think the error here is in running the hypothesis tests in all test commands instead of just that action. Are you able to take a look at that.

For example on this PR we run the tests separately too.

@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants