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

Enable stateful tests for LocalStore #2804

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

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Feb 6, 2025

Also restrict stateful tests to slow hypothesis CI workflow to speed up the CI a bit (cc @dstansby)

Seems to work:

  • SKIPPED [6] tests/test_store/test_stateful.py: need --run-slow-hypothesis option to run

Also fixes the LocalStore stateful test

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Feb 6, 2025
@dcherian dcherian force-pushed the restrict-slow-hypothesis branch from 38b17c2 to 2ef7a4d Compare February 6, 2025 04:40
@dcherian dcherian changed the title Restrict stateful tests to slow hypothesis CI workflow. Enable stateful tests for LocalStore Feb 6, 2025
@dcherian dcherian force-pushed the restrict-slow-hypothesis branch from 2ef7a4d to 4571884 Compare February 6, 2025 04:45
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Feb 6, 2025
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 great that the slow tests aren't running on all the test runs now! I left some comments/requests for updates/questions inline.

changes/2804.feature.rst Outdated Show resolved Hide resolved
src/zarr/storage/_local.py Show resolved Hide resolved
src/zarr/testing/stateful.py Show resolved Hide resolved
src/zarr/testing/strategies.py Show resolved Hide resolved
@@ -209,7 +210,7 @@ def basic_indices(draw: st.DrawFn, *, shape: tuple[int], **kwargs: Any) -> Any:


def key_ranges(
keys: SearchStrategy = node_names, max_size: int | None = None
keys: SearchStrategy = node_names, max_size: int = sys.maxsize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of our public API, so changing the default here is an API change. What was the motivation for changing it - does hypothesis not work well with max_value=None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleans up the use of min below, that's all.

tests/test_store/test_stateful.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants