Skip to content

Conversation

maxrjones
Copy link
Member

This PR creates a pytest fixture so that the temporary directory used in the store tests get cleaned up after use. The motivation is that the current usage of temporary directories was causing the failures seen in https://github.com/zarr-developers/zarr-python/actions/runs/15405315759/job/43346759238?pr=2774.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jun 3, 2025
@maxrjones
Copy link
Member Author

Darn, this helped locally but apparently there are other problems

@d-v-b
Copy link
Contributor

d-v-b commented Jun 3, 2025

I'm surprised pytest's built-in temporary directory management is insufficient for our needs here. can we not make the store_like fixture take a tmp_dir fixture as input (even if some of the stores don't use it)?

@dstansby
Copy link
Contributor

dstansby commented Jun 3, 2025

Perhaps we're running into pytest-dev/pytest-cov#693? Pinning pytest<8.4 would be the quickest way of checking.

@dstansby
Copy link
Contributor

dstansby commented Jun 3, 2025

I'm giving the pytest pin a go over at #3113 - this PR is definitely a good idea regardless 👍

Comment on lines +21 to +40
@pytest.fixture(
params=["none", "temp_dir_str", "temp_dir_path", "store_path", "memory_store", "dict"]
)
def store_like(request):
if request.param == "none":
yield None
elif request.param == "temp_dir_str":
with tempfile.TemporaryDirectory() as temp_dir:
yield temp_dir
elif request.param == "temp_dir_path":
with tempfile.TemporaryDirectory() as temp_dir:
yield Path(temp_dir)
elif request.param == "store_path":
yield StorePath(store=MemoryStore(store_dict={}), path="/")
elif request.param == "memory_store":
yield MemoryStore(store_dict={})
elif request.param == "dict":
yield {}


Choose a reason for hiding this comment

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

I think you can simplify like so:

Suggested change
@pytest.fixture(
params=["none", "temp_dir_str", "temp_dir_path", "store_path", "memory_store", "dict"]
)
def store_like(request):
if request.param == "none":
yield None
elif request.param == "temp_dir_str":
with tempfile.TemporaryDirectory() as temp_dir:
yield temp_dir
elif request.param == "temp_dir_path":
with tempfile.TemporaryDirectory() as temp_dir:
yield Path(temp_dir)
elif request.param == "store_path":
yield StorePath(store=MemoryStore(store_dict={}), path="/")
elif request.param == "memory_store":
yield MemoryStore(store_dict={})
elif request.param == "dict":
yield {}
@pytest.fixture(
params=[
None,
str,
Path,
StorePath(store=MemoryStore(store_dict={}), path="/"),
MemoryStore(store_dict={}),
{},
],
)
def store_like(request, tmp_path: Path):
return param if not callable(param := request.param) else param(tmp_path)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, this is more concise at the expense of being less obviously interpretable. I'm fine with either version.

Copy link

@chuckwondo chuckwondo Jun 5, 2025

Choose a reason for hiding this comment

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

If you want something closer to your original, but slightly simpler:

@pytest.fixture(
    params=[None, "temp_dir_str", "temp_dir_path", "store_path", "memory_store", "dict"]
)
def store_like(request, tmp_path: Path):
    if request.param is None:
        return None
    if request.param == "temp_dir_str":
        return str(tmp_path)
    if request.param == "temp_dir_path":
        return tmp_path
    if request.param == "store_path":
        return StorePath(store=MemoryStore(store_dict={}), path="/")
    if request.param == "memory_store":
        return MemoryStore(store_dict={})
    if request.param == "dict":
        return {}

@dstansby dstansby enabled auto-merge (squash) June 6, 2025 19:15
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Jun 6, 2025
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jun 6, 2025
@dstansby
Copy link
Contributor

dstansby commented Jun 6, 2025

Thanks - when we do switch back to pytest 8.4, hopefully this will be a few less errors to fix!

@dstansby dstansby merged commit a8d4f42 into zarr-developers:main Jun 6, 2025
29 of 30 checks passed
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.

4 participants