test: fix broken test_docstring_consistent_parameters and mark xfail

[Aniketsy]

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

zarr-python/tests/test_api.py

Line 1437 in a994a4f

def test_consistent_signatures(

zarr-python/tests/test_group.py

Line 1792 in a994a4f

)

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)

[d-v-b]

🤖 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):

zarr-python/tests/test_api/test_synchronous.py

Lines 112 to 128 in a994a4f

	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:

  zarr-python/tests/test_api/test_synchronous.py

  Lines 21 to 27 in a994a4f

  	)
  	"""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.

zarr-python/tests/test_api/test_synchronous.py

Lines 45 to 52 in a994a4f

	("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]

@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.

[d-v-b]

instead of xfailing, can we make the test work properly, e.g. by fixing it, and fixing any docstring failures the fixed test reveals?

[Aniketsy]

instead of xfailing, can we make the test work properly, e.g. by fixing it, and fixing any docstring failures the fixed test reveals?

make sense, i'll be back with this shortly, maybe 1-2 days and fix this.

[d-v-b]

iterating this way aggregates all parameters in the same dict, and then checks if they have the same description / type, which won't be true. we need to break down the aggregation per-parameter.

[Aniketsy]

thanks for the pointer