fix incorrect path concatenation in make_store_path, and refactor store_path tests

Author: d-v-b

src/zarr/storage/common.py

@@ -219,7 +219,7 @@ async def make_store_path(
         if mode is not None and mode != store_like.store.mode.str:
             _store = store_like.store.with_mode(mode)
             await _store._ensure_open()
-            store_like = StorePath(_store, path=store_like.path) / path_normalized
+            store_like = StorePath(_store, path=store_like.path)
         result = store_like / path_normalized
     elif isinstance(store_like, Store):
         if mode is not None and mode != store_like.mode.str:

tests/v3/test_group.py

@@ -3,7 +3,7 @@
 import contextlib
 import pickle
 import warnings
-from typing import TYPE_CHECKING, Any, Literal, cast
+from typing import TYPE_CHECKING, Any, Literal
 
 import numpy as np
 import pytest
@@ -14,7 +14,6 @@
 from zarr import Array, AsyncArray, AsyncGroup, Group
 from zarr.abc.store import Store
 from zarr.core.buffer import default_buffer_prototype
-from zarr.core.common import JSON, ZarrFormat
 from zarr.core.group import ConsolidatedMetadata, GroupMetadata
 from zarr.core.sync import sync
 from zarr.errors import ContainsArrayError, ContainsGroupError
@@ -26,6 +25,8 @@
 if TYPE_CHECKING:
     from _pytest.compat import LEGACY_PATH
 
+    from zarr.core.common import JSON, ZarrFormat
+
 
 @pytest.fixture(params=["local", "memory", "zip"])
 async def store(request: pytest.FixtureRequest, tmpdir: LEGACY_PATH) -> Store:

tests/v3/test_store/test_core.py

@@ -1,7 +1,9 @@
 import tempfile
 from pathlib import Path
+from typing import Literal
 
 import pytest
+from _pytest.compat import LEGACY_PATH
 from upath import UPath
 
 from zarr.storage._utils import normalize_path
@@ -11,31 +13,59 @@
 from zarr.storage.remote import RemoteStore
 
 
-async def test_make_store_path(tmpdir: str) -> None:
-    # None
-    store_path = await make_store_path(None)
+@pytest.mark.parametrize("path", [None, "", "bar"])
+async def test_make_store_path_none(path: str) -> None:
+    """
+    Test that creating a store_path with None creates a memorystore
+    """
+    store_path = await make_store_path(None, path=path)
     assert isinstance(store_path.store, MemoryStore)
-
-    # str
-    store_path = await make_store_path(str(tmpdir))
+    assert store_path.path == normalize_path(path)
+
+
+@pytest.mark.parametrize("path", [None, "", "bar"])
+@pytest.mark.parametrize("store_type", [str, Path, LocalStore])
+@pytest.mark.parametrize("mode", ["r", "w", "a"])
+async def test_make_store_path_local(
+    tmpdir: LEGACY_PATH,
+    store_type: type[str] | type[Path] | type[LocalStore],
+    path: str,
+    mode: Literal["r", "w", "a"],
+) -> None:
+    """
+    Test the various ways of invoking make_store_path that create a LocalStore
+    """
+    store_like = store_type(str(tmpdir))
+    store_path = await make_store_path(store_like, path=path, mode=mode)
     assert isinstance(store_path.store, LocalStore)
     assert Path(store_path.store.root) == Path(tmpdir)
-
-    # Path
-    store_path = await make_store_path(Path(tmpdir))
+    assert store_path.path == normalize_path(path)
+    assert store_path.store.mode.str == mode
+
+
+@pytest.mark.parametrize("path", [None, "", "bar"])
+@pytest.mark.parametrize("mode", ["r", "w", "a"])
+async def test_make_store_path_store_path(
+    tmpdir: LEGACY_PATH, path: str, mode: Literal["r", "w", "a"]
+) -> None:
+    """
+    Test invoking make_store_path when the input is another store_path. In particular we want to ensure
+    that a new path is handled correctly.
+    """
+    store_like = StorePath(LocalStore(str(tmpdir)), path="root")
+    store_path = await make_store_path(store_like, path=path, mode=mode)
     assert isinstance(store_path.store, LocalStore)
     assert Path(store_path.store.root) == Path(tmpdir)
+    path_normalized = normalize_path(path)
+    assert store_path.path == (store_like / path_normalized).path
 
-    # Store
-    store_path = await make_store_path(store_path.store)
-    assert isinstance(store_path.store, LocalStore)
-    assert Path(store_path.store.root) == Path(tmpdir)
+    assert store_path.store.mode.str == mode
 
-    # StorePath
-    store_path = await make_store_path(store_path)
-    assert isinstance(store_path.store, LocalStore)
-    assert Path(store_path.store.root) == Path(tmpdir)
 
+async def test_make_store_path_invalid() -> None:
+    """
+    Test that invalid types raise TypeError
+    """
     with pytest.raises(TypeError):
         await make_store_path(1)  # type: ignore[arg-type]