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

Add example hypothesis roundtrip test #1746

Draft
wants to merge 16 commits into
base: v3
Choose a base branch
from

Conversation

dcherian
Copy link

@dcherian dcherian commented Apr 5, 2024

After mamba install hypothesis

Use

python -m pytest --capture=no --hypothesis-verbosity=verbose test_properties.py

to see all the things it tries.

This was a quick attempt at a property test.

The other thing you can do is a "Stateful" test (e.g. https://github.com/pydata/xarray/blob/main/properties/test_index_manipulation.py)

which runs an arbitrary sequence of manipulations (e.g. add array, rename, move, delete, modify array, copy), and checks for consistency at each point.

cc @Zac-HD

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/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@pep8speaks
Copy link

pep8speaks commented Apr 5, 2024

Hello @dcherian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 5:1: E402 module level import not at top of file
Line 6:1: E402 module level import not at top of file
Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 12:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file
Line 15:1: E402 module level import not at top of file
Line 17:1: E266 too many leading '#' for block comment

Comment last updated at 2024-04-05 17:47:12 UTC

Comment on lines 77 to 92
elif path == "/":
assert name is not None
array_path = name
array_name = "/" + name
else:
assert name is not None
array_path = f"{path}/{name}"
array_name = "/" + array_path
Copy link
Author

Choose a reason for hiding this comment

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

totally not obvious!

tests/test_properties.py Outdated Show resolved Hide resolved
1. Roundtrip a numpy array
2. Basic Indexing
@dcherian dcherian mentioned this pull request Jun 4, 2024
5 tasks
@dcherian
Copy link
Author

dcherian commented Jun 5, 2024

Some input needed:

  1. Should this be a separate action or folded in to the existing actions? I lean former. It's fast now (47s) but it could get slower in the future.
  2. There's more to make strategies.py actually useful as public API but I'd like to punt that to the future.
  3. I'll make comments where I'd like a careful review.

src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
Comment on lines +74 to +75
# TODO: clean this up
if path is None and name is None:
Copy link
Author

Choose a reason for hiding this comment

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

Can this mess be cleaned up?

Comment on lines +107 to +114
assert isinstance(a, Array)
assert nparray.shape == a.shape
# assert chunks == a.chunks # TODO: adapt for v2, v3
assert array_path == a.path
assert array_name == a.name
# assert a.basename is None # TODO
# assert a.store == normalize_store_arg(store)
assert dict(a.attrs) == expected_attrs
Copy link
Author

Choose a reason for hiding this comment

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

Would be good to decide what invariants to check here. Potentially could move out to its own test.

Comment on lines +103 to +104
# TODO: FIXME seems to break with booleans and timedelta
# fill_value=nparray.dtype.type(0),
Copy link
Author

Choose a reason for hiding this comment

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

This seems to be a bug.

Copy link
Member

Choose a reason for hiding this comment

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

the boolean issue is probably a bug, would be good to see if we can extract a standalone reproducer.

@dcherian
Copy link
Author

Some input here would be very helpful

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Long overdue. Thanks @dcherian for working on this. Once this is in, I'd love some guidance on writing a similar thing for the store tests.

src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Show resolved Hide resolved
Comment on lines +103 to +104
# TODO: FIXME seems to break with booleans and timedelta
# fill_value=nparray.dtype.type(0),
Copy link
Member

Choose a reason for hiding this comment

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

the boolean issue is probably a bug, would be good to see if we can extract a standalone reproducer.

* v3: (22 commits)
  [v3] `Buffer` ensure correct subclass based on the `BufferPrototype` argument (zarr-developers#1974)
  Fix doc build (zarr-developers#1987)
  Fix doc build warnings (zarr-developers#1985)
  Automatically generate API reference docs (zarr-developers#1918)
  Update `RemoteStore.__str__` and add UPath tests (zarr-developers#1964)
  [v3] Elevate codec pipeline (zarr-developers#1932)
  0 dim arrays: indexing (zarr-developers#1980)
  `parse_shapelike` allows 0 (zarr-developers#1979)
  Clean up typing and docs for indexing (zarr-developers#1961)
  add json indentation to config (zarr-developers#1952)
  chore: update pre-commit hooks (zarr-developers#1973)
  Bump pypa/gh-action-pypi-publish in the actions group (zarr-developers#1969)
  chore: update pre-commit hooks (zarr-developers#1957)
  Update release.rst (zarr-developers#1960)
  doc: update release notes for 3.0.0.alpha (zarr-developers#1959)
  Basic working FsspecStore (zarr-developers#1785)
  Feature: Top level V3 API (zarr-developers#1884)
  Buffer Prototype Argument (zarr-developers#1910)
  Create issue-metrics.yml
  fixes bug in transpose (zarr-developers#1949)
  ...
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.

None yet

4 participants