Skip to content

test: fix broken test_docstring_consistent_parameters and mark xfail - #4224

Open
Aniketsy wants to merge 2 commits into
zarr-developers:mainfrom
Aniketsy:fix-3989-1
Open

test: fix broken test_docstring_consistent_parameters and mark xfail#4224
Aniketsy wants to merge 2 commits into
zarr-developers:mainfrom
Aniketsy:fix-3989-1

Conversation

@Aniketsy

@Aniketsy Aniketsy commented Aug 1, 2026

Copy link
Copy Markdown

Summary

Towards #3989

  • Removed test_docstring_consistent_parameters from tests/test_api/test_synchronous.py. as i find its already covered here
  • also test_docstrings_match checks almost the exact same thing (name/type/description)- for docstring coverage

def test_consistent_signatures(

please feel free to point out if i'm missing something

[Describe what this PR changes and why, in your own words.]

For reviewers

[What would you most value a second look at? What are you already confident in? For a refactor, say whether behavior is meant to be unchanged.]

Author attestation

  • I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

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/*.md
  • 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 Aug 1, 2026
@d-v-b

d-v-b commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Code review

Found 1 issue:

  1. The redundancy claim doesn't hold: the deleted test checks something none of the cited tests check. test_docstring_consistent_parameters compared the type and description of a shared parameter across different routines (create vs create_array vs create_group vs Group.create_array):

types: dict[str, tuple[str, ...]] = {}
for routine in array_creation_routines:
key = f"{routine.__module__}.{routine.__qualname__}"
docstring = NumpyDocString(routine.__doc__)
param_dict = {d.name: d for d in docstring["Parameters"]}
if parameter_name in param_dict:
val = param_dict[parameter_name]
if tuple(val.desc) in descs:
descs[tuple(val.desc)] = descs[tuple(val.desc)] + (key,)
else:
descs[tuple(val.desc)] = (key,)
if val.type in types:
types[val.type] = types[val.type] + (key,)
else:
types[val.type] = (key,)
assert len(descs) <= 1
assert len(types) <= 1

The three tests cited as covering it all compare a sync/async pair of the same callable instead:

  • test_docstrings_match — sync vs async docstring of one callable:
    )
    """A sorted tuple of callable names that are exported by both the sync and async APIs."""
    @pytest.mark.parametrize("callable_name", MATCHED_CALLABLE_NAMES)
    def test_docstrings_match(callable_name: str) -> None:
    """
  • tests/test_api.py:1437 and tests/test_group.py:1793 are test_consistent_signatures, which compare inspect.signature(), not docstring prose:

    zarr-python/tests/test_api.py

    Lines 1436 to 1444 in a994a4f

    )
    def test_consistent_signatures(
    a_func: Callable[[object], object], b_func: Callable[[object], object]
    ) -> None:
    """
    Ensure that pairs of functions have the same signature
    """
    base_sig = inspect.signature(a_func)
    test_sig = inspect.signature(b_func)

grep -n docstring tests/test_api.py tests/test_group.py returns nothing — there are no docstring tests in either file, and NumpyDocString is used nowhere else in the suite. So the cross-routine coverage is not duplicated; it disappears with this PR.

That said, the test as written was vacuously passing: every parameter_name parametrize value is a tuple (e.g. ("store", "path")), while param_dict is keyed by individual parameter-name strings, so if parameter_name in param_dict is always False and both asserts reduce to 0 <= 1.

("parameter_name", "array_creation_routines"),
[
pytest.param(
("store", "path"),
(
asynchronous.create_array,
synchronous.create_array,
asynchronous.create_group,

So nothing is lost in terms of tests that were actually running, but the intended coverage — added deliberately in #3390 and maintained as recently as #4093 — is real and unimplemented. Repairing the test (iterate over the tuple, one parameter at a time) looks like the better fix than deleting it. If the intent is specifically to speed up the suite per #3989, this test contributes essentially nothing to runtime either way.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Aniketsy Aniketsy changed the title test: remove redundant/ overlapping test_docstring_consistent_parameters test: fix broken test_docstring_consistent_parameters and mark xfail Aug 1, 2026
@Aniketsy Aniketsy changed the title test: fix broken test_docstring_consistent_parameters and mark xfail test: fix broken test_docstring_consistent_parameters and mark xfail Aug 1, 2026
@Aniketsy

Aniketsy commented Aug 1, 2026

Copy link
Copy Markdown
Author

@d-v-b thanks for the review, I've fixed the broken test but that leads to failures, so i've marked test as xfail for now and opened issue for the failure. please let me know if you have any thoughts related to this.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants