From b3a678325d0c0f0ca71ae39330fa3133235629fb Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:47:33 -0400 Subject: [PATCH 01/24] Add code duplication analysis to the dev workflow Wire jscpd (pinned to 5.3.3) in as a `tox -e duplication` env, backed by tools/check-duplication.sh. Config lives in .jscpd.json, scanning dandi/ and tools/ with sensible ignores for vendored/generated files, test fixture data, and build artifacts. Threshold is temporarily set to 5% (above the measured 4.38% baseline) so this lands green; a follow-up commit tightens it after mitigating the duplication found. Co-Authored-By: Claude Sonnet 5 --- .gitignore | 1 + .jscpd.json | 24 ++++++++++++++++++++++++ tools/check-duplication.sh | 21 +++++++++++++++++++++ tox.ini | 8 +++++++- 4 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 .jscpd.json create mode 100755 tools/check-duplication.sh diff --git a/.gitignore b/.gitignore index e2d2dab22..9d282c3ce 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ .mypy_cache/ .pytest_cache/ .ruff_cache/ +.tmp/ .tox/ .venv*/ __pycache__/ diff --git a/.jscpd.json b/.jscpd.json new file mode 100644 index 000000000..6421b2211 --- /dev/null +++ b/.jscpd.json @@ -0,0 +1,24 @@ +{ + "threshold": 5, + "reporters": ["console"], + "ignore": [ + "**/.git/**", + "**/.tox/**", + "**/.venv*/**", + "**/venv*/**", + "**/node_modules/**", + "**/__pycache__/**", + "**/.hypothesis/**", + "**/*.egg-info/**", + "**/.eggs/**", + "**/.npm/**", + "**/.tmp/**", + "**/dist/**", + "**/build/**", + "**/tests/data/**", + "**/versioneer.py", + "**/dandi/_version.py" + ], + "minTokens": 50, + "minLines": 5 +} diff --git a/tools/check-duplication.sh b/tools/check-duplication.sh new file mode 100755 index 000000000..fd88c70cc --- /dev/null +++ b/tools/check-duplication.sh @@ -0,0 +1,21 @@ +#!/bin/bash +# Code duplication detection. +# Run via: tox -e duplication +# Requires: npx (Node.js) + +set -eu + +# Pinned so a new jscpd release can't change behavior or fail CI on an +# otherwise-unchanged commit. Bump deliberately (and re-verify) when needed. +JSCPD_VERSION=5.3.3 + +if ! command -v npx >/dev/null 2>&1; then + echo "ERROR: npx not found. Install Node.js to run duplication checks." + exit 1 +fi + +echo "=== Code duplication check (jscpd ${JSCPD_VERSION}) ===" +echo + +# jscpd reads .jscpd.json for config (threshold, ignores, etc.) +npx --yes "jscpd@${JSCPD_VERSION}" dandi tools diff --git a/tox.ini b/tox.ini index f40c8230a..0fb5949b8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,7 +1,7 @@ [tox] requires = tox-uv >= 1.11 -envlist = lint,typing,py3,py3-lowest +envlist = lint,typing,duplication,py3,py3-lowest [testenv] setenv = @@ -40,6 +40,12 @@ commands = codespell dandi docs tools setup.py flake8 --config=setup.cfg {posargs} dandi setup.py +[testenv:duplication] +skip_install = true +allowlist_externals = bash +description = Detect code duplication +commands = bash {toxinidir}/tools/check-duplication.sh + [testenv:typing] deps = mypy != 1.11.0 From 871af90b0eae6e01adca79d44dd01c972cef6611 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:48:02 -0400 Subject: [PATCH 02/24] Run duplication check in CI Add .github/workflows/duplication.yml, mirroring the existing typing.yml/lint.yml pattern: a dedicated job that installs tox and runs `tox -e duplication`. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/duplication.yml | 32 +++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 .github/workflows/duplication.yml diff --git a/.github/workflows/duplication.yml b/.github/workflows/duplication.yml new file mode 100644 index 000000000..ea79a8e79 --- /dev/null +++ b/.github/workflows/duplication.yml @@ -0,0 +1,32 @@ +name: Duplication check + +on: + - push + - pull_request + +jobs: + duplication: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.11' + + - name: Set up Node.js + uses: actions/setup-node@v5 + with: + node-version: '24' + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + python -m pip install --upgrade tox + + - name: Run duplication check + run: tox -e duplication From 3a4f5802ff4bafd2fe152a43b26d8a579b746843 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:53:24 -0400 Subject: [PATCH 03/24] Deduplicate _bids_validate docstring by referencing bids_validate The private _bids_validate() implementation repeated the full Parameters/Returns docstring of its public wrapper bids_validate(). Since _bids_validate is internal-only, point its docstring at the public function instead of duplicating the parameter docs. Co-Authored-By: Claude Sonnet 5 --- dandi/bids_validator_deno/_validator.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/dandi/bids_validator_deno/_validator.py b/dandi/bids_validator_deno/_validator.py index 8c55cb488..a946e6e9f 100644 --- a/dandi/bids_validator_deno/_validator.py +++ b/dandi/bids_validator_deno/_validator.py @@ -206,22 +206,8 @@ def _bids_validate( recursive: bool = False, ) -> BidsValidationResult: """ - Validate a file directory as a BIDS dataset with the deno-compiled BIDS validator - - Parameters - ---------- - dir_ : DirectoryPath - The path to the directory to validate - config : Optional[dict] - The configuration to use in the validation. This specifies a JSON configuration - file to be provided through the `--config` option when invoking the underlying - deno-compiled BIDS validator. If `None`, the deno-compiled BIDS validator will - be invoked without the `--config` option. - ignore_nifti_headers : bool - If `True`, disregard NIfTI header content during validation - recursive : bool - If `True`, validate datasets found in derivatives directories in addition to - root dataset + Implementation of `bids_validate()`. See that function for a description of the + parameters. Returns ------- From 7b216a8256fe711cbbe9c4b069aeae5e7280577f Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:54:39 -0400 Subject: [PATCH 04/24] Deduplicate flatten_meta_to_pyout_v1 docstring Its docstring repeated flatten_meta_to_pyout()'s prose almost verbatim; point it at the sibling function and describe only how it differs (recursive flattening of nested dicts). Co-Authored-By: Claude Sonnet 5 --- dandi/cli/cmd_ls.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/dandi/cli/cmd_ls.py b/dandi/cli/cmd_ls.py index 1969236ad..8b4b49e1a 100644 --- a/dandi/cli/cmd_ls.py +++ b/dandi/cli/cmd_ls.py @@ -286,17 +286,9 @@ def flatten_v(v): def flatten_meta_to_pyout_v1(meta): - """Given a meta record, possibly flatten record since no nested records - supported yet - - lists become joined using ', ', dicts get individual key: values. - lists of dict - doing nothing magical. - - Empty values are not considered. - - Parameters - ---------- - meta: dict + """Like `flatten_meta_to_pyout`, but nested dicts get flattened + recursively into individual "key: value" entries instead of being + joined into a single string. """ out = {} From bca564d8161a0a4f77a7519e62d29b6e4741841a Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:57:09 -0400 Subject: [PATCH 05/24] Deduplicate RemoteDandiset asset-listing and raw-asset-upload methods get_assets, get_assets_with_path_prefix, and get_assets_by_glob shared the same paginate/except/yield body, differing only in the request params; extract it into _iter_version_assets(). upload_raw_asset and iter_upload_raw_asset shared the same dandi_file()/isinstance check for resolving a LocalAsset; extract it into _local_asset_file(). Co-Authored-By: Claude Sonnet 5 --- dandi/dandiapi.py | 64 +++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/dandi/dandiapi.py b/dandi/dandiapi.py index ccf17239b..64292256a 100644 --- a/dandi/dandiapi.py +++ b/dandi/dandiapi.py @@ -68,6 +68,8 @@ if TYPE_CHECKING: from typing_extensions import Self + from .files import LocalAsset + lgr = get_logger() @@ -1407,6 +1409,17 @@ def publish(self, max_time: float = 120) -> RemoteDandiset: f"No published versions found for Dandiset {self.identifier}" ) + def _iter_version_assets(self, params: dict) -> Iterator[RemoteAsset]: + try: + for a in self.client.paginate( + f"{self.version_api_path}assets/", params=params + ): + yield RemoteAsset.from_data(self, a) + except HTTP404Error: + raise NotFoundError( + f"No such version: {self.version_id!r} of Dandiset {self.identifier}" + ) + def get_assets(self, order: str | None = None) -> Iterator[RemoteAsset]: """ Returns an iterator of all assets in this version of the Dandiset. @@ -1416,15 +1429,7 @@ def get_assets(self, order: str | None = None) -> Iterator[RemoteAsset]: ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", params={"order": order} - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets({"order": order}) def get_asset(self, asset_id: str) -> RemoteAsset: """ @@ -1453,16 +1458,9 @@ def get_assets_with_path_prefix( ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", - params={"path": self._normalize_path(path), "order": order}, - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets( + {"path": self._normalize_path(path), "order": order} + ) def get_assets_by_glob( self, pattern: str, order: str | None = None @@ -1478,16 +1476,7 @@ def get_assets_by_glob( ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", - params={"glob": pattern, "order": order}, - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets({"glob": pattern, "order": order}) def get_asset_by_path(self, path: str) -> RemoteAsset: """ @@ -1564,15 +1553,19 @@ def upload_raw_asset( :param RemoteAsset replace_asset: If set, replace the given asset, which must have the same path as the new asset """ + df = self._local_asset_file(filepath) + return df.upload( + self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset + ) + + def _local_asset_file(self, filepath: str | Path) -> "LocalAsset": # Avoid circular import by importing within function: from .files import LocalAsset, dandi_file df = dandi_file(filepath) if not isinstance(df, LocalAsset): raise ValueError(f"{filepath}: not an asset file") - return df.upload( - self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset - ) + return df def iter_upload_raw_asset( self, @@ -1606,12 +1599,7 @@ def iter_upload_raw_asset( ``"done"`` and an ``"asset"`` key containing the resulting `RemoteAsset`. """ - # Avoid circular import by importing within function: - from .files import LocalAsset, dandi_file - - df = dandi_file(filepath) - if not isinstance(df, LocalAsset): - raise ValueError(f"{filepath}: not an asset file") + df = self._local_asset_file(filepath) return df.iter_upload( self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset ) From 73e5f068f8772cb608fc591f480750f39f395234 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 14:59:37 -0400 Subject: [PATCH 06/24] Deduplicate asset download streaming and set_raw_metadata BaseRemoteAsset.get_download_file_iter and RemoteZarrEntry.get_download_file_iter shared the same GET-with-Range-header/raise_for_status logic; extract it into module-level _get_download_response(). RemoteBlobAsset.set_raw_metadata and RemoteZarrAsset.set_raw_metadata were identical apart from the blob_id/zarr_id key; extract the shared PUT + in-place field update into RemoteAsset._put_raw_metadata(). Co-Authored-By: Claude Sonnet 5 --- dandi/dandiapi.py | 76 ++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/dandi/dandiapi.py b/dandi/dandiapi.py index 64292256a..e9cdf8f79 100644 --- a/dandi/dandiapi.py +++ b/dandi/dandiapi.py @@ -1605,6 +1605,24 @@ def iter_upload_raw_asset( ) +def _get_download_response( + session: requests.Session, url: str, start_at: int = 0 +) -> requests.Response: + """ + Issue the (optionally range-restricted) GET request for streaming a + download from ``url``, raising for any HTTP error status. + """ + lgr.debug("Starting download from %s", url) + headers = None + if start_at > 0: + headers = {"Range": f"bytes={start_at}-"} + result = session.get(url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT) + # TODO: apparently we might need retries here as well etc + # if result.status_code not in (200, 201): + result.raise_for_status() + return result + + class BaseRemoteAsset(ABC, APIBase): """ Representation of an asset retrieved from the API without associated @@ -1850,16 +1868,7 @@ def get_download_file_iter( url = self.base_download_url def downloader(start_at: int = 0) -> Iterator[bytes]: - lgr.debug("Starting download from %s", url) - headers = None - if start_at > 0: - headers = {"Range": f"bytes={start_at}-"} - result = self.client.session.get( - url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT - ) - # TODO: apparently we might need retries here as well etc - # if result.status_code not in (200, 201): - result.raise_for_status() + result = _get_download_response(self.client.session, url, start_at) nbytes, nchunks = 0, 0 for chunk in result.iter_content(chunk_size=chunk_size): nchunks += 1 @@ -2131,6 +2140,20 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: """ ... + def _put_raw_metadata( + self, metadata: dict[str, Any], id_field: str, id_value: str + ) -> None: + set_asset_schema_key(metadata) + data = self.client.put( + self.api_path, json={"metadata": metadata, id_field: id_value} + ) + self.identifier = data["asset_id"] + self.path = data["path"] + self.size = int(data["size"]) + self.created = ensure_datetime(data["created"]) + self.modified = ensure_datetime(data["modified"]) + self._metadata = data["metadata"] + def rename(self, dest: str) -> None: """ .. versionadded:: 0.41.0 @@ -2160,16 +2183,7 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: Set the metadata for the asset on the server to the given value and update the `RemoteBlobAsset` in place. """ - set_asset_schema_key(metadata) - data = self.client.put( - self.api_path, json={"metadata": metadata, "blob_id": self.blob} - ) - self.identifier = data["asset_id"] - self.path = data["path"] - self.size = int(data["size"]) - self.created = ensure_datetime(data["created"]) - self.modified = ensure_datetime(data["modified"]) - self._metadata = data["metadata"] + self._put_raw_metadata(metadata, "blob_id", self.blob) class RemoteZarrAsset(RemoteAsset, BaseRemoteZarrAsset): @@ -2184,16 +2198,7 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: Set the metadata for the asset on the server to the given value and update the `RemoteZarrAsset` in place. """ - set_asset_schema_key(metadata) - data = self.client.put( - self.api_path, json={"metadata": metadata, "zarr_id": self.zarr} - ) - self.identifier = data["asset_id"] - self.path = data["path"] - self.size = int(data["size"]) - self.created = ensure_datetime(data["created"]) - self.modified = ensure_datetime(data["modified"]) - self._metadata = data["metadata"] + self._put_raw_metadata(metadata, "zarr_id", self.zarr) @dataclass @@ -2308,16 +2313,7 @@ def get_download_file_iter( url = self.download_url def downloader(start_at: int = 0) -> Iterator[bytes]: - lgr.debug("Starting download from %s", url) - headers = None - if start_at > 0: - headers = {"Range": f"bytes={start_at}-"} - result = self.client.session.get( - url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT - ) - # TODO: apparently we might need retries here as well etc - # if result.status_code not in (200, 201): - result.raise_for_status() + result = _get_download_response(self.client.session, url, start_at) for chunk in result.iter_content(chunk_size=chunk_size): if chunk: # could be some "keep alive"? yield chunk From 68fd59223e6a8c7aa2f12b78ec8fc786d48b2f0e Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:03:23 -0400 Subject: [PATCH 07/24] Deduplicate LocalAsset.upload/iter_upload docstrings and BIDS asset methods LocalAsset.upload, the abstract LocalAsset.iter_upload, LocalFileAsset's concrete override, ZarrAsset's override, and the deprecated DandiAPIClient.iter_upload_raw_asset all repeated the same Parameters/Returns prose. Keep the full docs on upload() (the one method every subclass inherits unchanged) and have the others reference it, keeping only what's actually specific to each override. NWBBIDSAsset.get_validation_errors and ZarrBIDSAsset.get_validation_errors had identical bodies apart from which sibling class's method they combined with BIDSAsset's; extract _bids_combined_validation_errors(). ZarrBIDSAsset.get_metadata duplicated BIDSAsset.get_metadata's body plus one extra line; have it delegate instead. Co-Authored-By: Claude Sonnet 5 --- dandi/dandiapi.py | 6 ++---- dandi/files/bases.py | 36 ++++-------------------------------- dandi/files/bids.py | 40 +++++++++++++++++++++++----------------- dandi/files/zarr.py | 26 ++++++++------------------ 4 files changed, 37 insertions(+), 71 deletions(-) diff --git a/dandi/dandiapi.py b/dandi/dandiapi.py index e9cdf8f79..d2ce00a5b 100644 --- a/dandi/dandiapi.py +++ b/dandi/dandiapi.py @@ -1594,10 +1594,8 @@ def iter_upload_raw_asset( :param RemoteAsset replace_asset: If set, replace the given asset, which must have the same path as the new asset :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. + A generator of `dict`\\s; see `~dandi.files.LocalAsset.iter_upload` + for the shape of the status dicts """ df = self._local_asset_file(filepath) return df.iter_upload( diff --git a/dandi/files/bases.py b/dandi/files/bases.py index db502dcc3..97a8a3c38 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -292,19 +292,10 @@ def iter_upload( replacing: RemoteAsset | None = None, ) -> Iterator[dict]: """ - Upload the asset with the given metadata to the given Dandiset, - returning a generator of status `dict`\\s. + Like `upload()`, but returns a generator of status `dict`\\s instead + of blocking until the upload is complete. See `upload()` for a + description of the parameters. - :param RemoteDandiset dandiset: - the Dandiset to which the asset will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset :returns: A generator of `dict`\\s containing at least a ``"status"`` key. Upon successful upload, the last `dict` will have a status of @@ -344,26 +335,7 @@ def iter_upload( jobs: int | None = None, replacing: RemoteAsset | None = None, ) -> Iterator[dict]: - """ - Upload the file as an asset with the given metadata to the given - Dandiset, returning a generator of status `dict`\\s. - - :param RemoteDandiset dandiset: - the Dandiset to which the file will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset - :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. - """ + """See `LocalAsset.iter_upload`.""" # Avoid heavy import by importing within function: from dandi.support.digests import get_dandietag diff --git a/dandi/files/bids.py b/dandi/files/bids.py index 008b085dd..d44dc7b32 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -11,7 +11,7 @@ from dandi.bids_validator_deno import bids_validate -from .bases import GenericAsset, LocalFileAsset, NWBAsset +from .bases import DandiFile, GenericAsset, LocalFileAsset, NWBAsset from .zarr import ZarrAsset from ..consts import ZARR_MIME_TYPE, dandiset_metadata_file from ..metadata.core import add_common_metadata, prepare_metadata @@ -197,6 +197,21 @@ def get_validation_errors( # get_metadata(): inherit use of default metadata from LocalFileAsset +def _bids_combined_validation_errors( + self: "BIDSAsset", + format_cls: type[DandiFile], + schema_version: str | None, + devel_debug: bool, + missing_file_content: MissingFileContent | None, +) -> list[ValidationResult]: + return format_cls.get_validation_errors( + self, + schema_version, + devel_debug, + missing_file_content=missing_file_content, + ) + BIDSAsset.get_validation_errors(self) + + @dataclass class BIDSAsset(LocalFileAsset): """ @@ -271,12 +286,9 @@ def get_validation_errors( devel_debug: bool = False, missing_file_content: MissingFileContent | None = None, ) -> list[ValidationResult]: - return NWBAsset.get_validation_errors( - self, - schema_version, - devel_debug, - missing_file_content=missing_file_content, - ) + BIDSAsset.get_validation_errors(self) + return _bids_combined_validation_errors( + self, NWBAsset, schema_version, devel_debug, missing_file_content + ) def get_metadata( self, @@ -306,22 +318,16 @@ def get_validation_errors( devel_debug: bool = False, missing_file_content: MissingFileContent | None = None, ) -> list[ValidationResult]: - return ZarrAsset.get_validation_errors( - self, - schema_version, - devel_debug, - missing_file_content=missing_file_content, - ) + BIDSAsset.get_validation_errors(self) + return _bids_combined_validation_errors( + self, ZarrAsset, schema_version, devel_debug, missing_file_content + ) def get_metadata( self, digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: - metadata = self.bids_dataset_description.get_asset_metadata(self) - start_time = end_time = datetime.now().astimezone() - add_common_metadata(metadata, self.filepath, start_time, end_time, digest) - metadata.path = self.path + metadata = BIDSAsset.get_metadata(self, digest, ignore_errors) metadata.encodingFormat = ZARR_MIME_TYPE return metadata diff --git a/dandi/files/zarr.py b/dandi/files/zarr.py index 25394769e..e3454b7a6 100644 --- a/dandi/files/zarr.py +++ b/dandi/files/zarr.py @@ -565,28 +565,18 @@ def iter_upload( zarr_mode: ZarrMode = "full", # type: ignore[assignment] ) -> Iterator[dict]: """ - Upload the Zarr directory as an asset with the given metadata to the - given Dandiset, returning a generator of status `dict`\\s. + Like `LocalAsset.iter_upload`, with two differences: ``replacing``, + if set and the old asset is a Zarr, has that Zarr updated & reused + for the new asset; and the resulting status generator can terminate + as ``"skipped"`` (see below). :param RemoteDandiset dandiset: the Dandiset to which the Zarr will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset; if the old asset is a Zarr, the Zarr will be updated - & reused for the new asset :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. If the local Zarr is bit-identical to the remote - (nothing to upload or delete), the terminal status is instead - ``"skipped"`` with a ``"message"`` of ``"identical"`` and the same - ``"asset"`` key. + See `LocalAsset.iter_upload`. Additionally, if the local Zarr is + bit-identical to the remote (nothing to upload or delete), the + terminal status is instead ``"skipped"`` with a ``"message"`` of + ``"identical"`` and the same ``"asset"`` key. """ asset_path = metadata.setdefault("path", self.path) set_asset_schema_key(metadata) From 12f50a5c8eaf2bcd98aa2e9f2aff06a8599ac5d7 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:06:29 -0400 Subject: [PATCH 08/24] Deduplicate path-like helpers between BasePath and RemoteZarrEntry RemoteZarrEntry deliberately stopped inheriting from `misctypes.BasePath` (0.48.0), but its suffix/suffixes/stem/match implementations were copy-pasted from BasePath verbatim. Extract the underlying logic into module-level helpers in misctypes.py (_path_suffix, _path_suffixes, _path_stem, _match_parts, _split_path) that both classes delegate to, without re-establishing the inheritance that was intentionally removed. Co-Authored-By: Claude Sonnet 5 --- dandi/dandiapi.py | 39 +++++++-------------- dandi/misctypes.py | 86 ++++++++++++++++++++++++++++------------------ 2 files changed, 65 insertions(+), 60 deletions(-) diff --git a/dandi/dandiapi.py b/dandi/dandiapi.py index d2ce00a5b..2ffc7cfde 100644 --- a/dandi/dandiapi.py +++ b/dandi/dandiapi.py @@ -18,7 +18,6 @@ from dataclasses import dataclass from datetime import datetime from enum import Enum -from fnmatch import fnmatchcase from functools import cached_property import json import os.path @@ -52,7 +51,14 @@ ) from .exceptions import HTTP404Error, NotFoundError, SchemaVersionError from .keyring_utils import keyring_lookup, keyring_save -from .misctypes import Digest, RemoteReadableAsset +from .misctypes import ( + Digest, + RemoteReadableAsset, + _match_parts, + _path_stem, + _path_suffix, + _path_suffixes, +) from .utils import ( USER_AGENT, check_dandi_version, @@ -2251,42 +2257,21 @@ def name(self) -> str: @property def suffix(self) -> str: """The final file extension of the basename, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[i:] - else: - return "" + return _path_suffix(self.name) @property def suffixes(self) -> list[str]: """A list of the basename's file extensions""" - if self.name.endswith("."): - return [] - name = self.name.lstrip(".") - return ["." + suffix for suffix in name.split(".")[1:]] + return _path_suffixes(self.name) @property def stem(self) -> str: """The basename without its final file extension, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[:i] - else: - return self.name + return _path_stem(self.name) def match(self, pattern: str) -> bool: """Tests whether the path matches the given glob pattern""" - if pattern.startswith("/"): - raise ValueError(f"Absolute paths not allowed: {pattern!r}") - patparts = tuple(q for q in pattern.split("/") if q) - if not patparts: - raise ValueError("Empty pattern") - if len(patparts) > len(self.parts): - return False - for part, pat in zip(reversed(self.parts), reversed(patparts)): - if not fnmatchcase(part, pat): - return False - return True + return _match_parts(self.parts, pattern) @property def download_url(self) -> str: diff --git a/dandi/misctypes.py b/dandi/misctypes.py index b50cffc42..bbe4220b5 100644 --- a/dandi/misctypes.py +++ b/dandi/misctypes.py @@ -60,6 +60,53 @@ def asdict(self) -> dict[DigestType, str]: value=32 * "d" + "-1--1", ) + +def _split_path(path: str) -> tuple[str, ...]: + """Split a path into its path components""" + if path.startswith("/"): + raise ValueError(f"Absolute paths not allowed: {path!r}") + return tuple(q for q in path.split("/") if q) + + +def _match_parts(parts: tuple[str, ...], pattern: str) -> bool: + """Tests whether ``parts`` matches the glob pattern ``pattern``""" + patparts = _split_path(pattern) + if not patparts: + raise ValueError("Empty pattern") + if len(patparts) > len(parts): + return False + for part, pat in zip(reversed(parts), reversed(patparts)): + if not fnmatchcase(part, pat): + return False + return True + + +def _path_suffix(name: str) -> str: + """The final file extension of ``name``, if any""" + i = name.rfind(".") + if 0 < i < len(name) - 1: + return name[i:] + else: + return "" + + +def _path_suffixes(name: str) -> list[str]: + """A list of ``name``'s file extensions""" + if name.endswith("."): + return [] + name = name.lstrip(".") + return ["." + suffix for suffix in name.split(".")[1:]] + + +def _path_stem(name: str) -> str: + """``name`` without its final file extension, if any""" + i = name.rfind(".") + if 0 < i < len(name) - 1: + return name[:i] + else: + return name + + P = TypeVar("P", bound="BasePath") @@ -103,7 +150,7 @@ def _get_subpath(self: P, name: str) -> P: def __truediv__(self: P, path: str) -> P: p = self - for q in self._split_path(path): + for q in _split_path(path): p = p._get_subpath(q) return p @@ -117,13 +164,6 @@ def joinpath(self: P, *paths: str) -> P: p /= q return p - @staticmethod - def _split_path(path: str) -> tuple[str, ...]: - """Split a path into its path components""" - if path.startswith("/"): - raise ValueError(f"Absolute paths not allowed: {path!r}") - return tuple(q for q in path.split("/") if q) - def is_root(self) -> bool: """ Returns true if this path object represents the root of its hierarchy @@ -168,28 +208,17 @@ def with_name(self: P, name: str) -> P: @property def suffix(self) -> str: """The final file extension of the basename, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[i:] - else: - return "" + return _path_suffix(self.name) @property def suffixes(self) -> list[str]: """A list of the basename's file extensions""" - if self.name.endswith("."): - return [] - name = self.name.lstrip(".") - return ["." + suffix for suffix in name.split(".")[1:]] + return _path_suffixes(self.name) @property def stem(self) -> str: """The basename without its final file extension, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[:i] - else: - return self.name + return _path_stem(self.name) def with_stem(self: P, stem: str) -> P: """Returns a new path with the stem changed""" @@ -209,15 +238,7 @@ def with_suffix(self: P, suffix: str) -> P: def match(self, pattern: str) -> bool: """Tests whether the path matches the given glob pattern""" - patparts = self._split_path(pattern) - if not patparts: - raise ValueError("Empty pattern") - if len(patparts) > len(self.parts): - return False - for part, pat in zip(reversed(self.parts), reversed(patparts)): - if not fnmatchcase(part, pat): - return False - return True + return _match_parts(self.parts, pattern) @abstractmethod def exists(self) -> bool: @@ -343,9 +364,8 @@ class RemoteReadableAsset(Readable): def open(self) -> IO[bytes]: # Optional dependency: - import fsspec - from aiohttp import ClientTimeout + import fsspec # We need to call open() on the return value of fsspec.open() because # otherwise the filehandle will only be opened when used to enter a From 7cd2714472965cd871e6736d32802599b9afc167 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:08:34 -0400 Subject: [PATCH 09/24] Deduplicate MultiAssetURL.get_assets overrides AssetPathPrefixURL, AssetFolderURL, and AssetGlobURL's get_assets() shared the same "look up the dandiset, iterate assets, track whether any were found, raise NotFoundError if strict and none were" body, differing only in the query method called and the error message. Extract it into _iter_multi_assets(). Co-Authored-By: Claude Sonnet 5 --- dandi/dandiarchive.py | 68 +++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 262d162d8..37c5b10a8 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -28,7 +28,7 @@ from __future__ import annotations from abc import ABC, abstractmethod -from collections.abc import Iterable, Iterator +from collections.abc import Callable, Iterable, Iterator from contextlib import contextmanager from dataclasses import dataclass, field import posixpath @@ -387,15 +387,13 @@ def get_assets( `NotFoundError` will now be raised if `strict` is true and there are no such assets. """ - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_with_path_prefix(self.path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found with path prefix {self.path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_with_path_prefix(self.path, order=order), + f"No assets found with path prefix {self.path!r}", + ) @dataclass @@ -535,15 +533,13 @@ def get_assets( path = self.path if not path.endswith("/"): path += "/" - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_with_path_prefix(path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found under folder {path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_with_path_prefix(path, order=order), + f"No assets found under folder {path!r}", + ) @dataclass @@ -566,15 +562,13 @@ def get_assets( `NotFoundError` will now be raised if `strict` is true and there are no such assets. """ - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_by_glob(self.path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found matching glob {self.path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_by_glob(self.path, order=order), + f"No assets found matching glob {self.path!r}", + ) def get_asset_download_path( self, asset: BaseRemoteAsset, preserve_tree: bool @@ -597,6 +591,24 @@ def _maybe_strict(strict: bool) -> Iterator[None]: raise +def _iter_multi_assets( + url: MultiAssetURL, + client: DandiAPIClient, + strict: bool, + get_assets: Callable[[RemoteDandiset], Iterator[BaseRemoteAsset]], + not_found_msg: str, +) -> Iterator[BaseRemoteAsset]: + any_assets = False + with _maybe_strict(strict): + d = url.get_dandiset(client, lazy=not strict) + assert d is not None + for a in get_assets(d): + any_assets = True + yield a + if strict and not any_assets: + raise NotFoundError(not_found_msg) + + @contextmanager def navigate_url( url: str, *, strict: bool = False, authenticate: bool | None = None From c605946d4d8b6d6a8a101191b364fd23bc59295b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:12:14 -0400 Subject: [PATCH 10/24] Deduplicate Mover/LocalizedMover abstract-method docstrings calculate_moves, calculate_moves_by_regex, get_assets, get_path, and move are declared abstractly on Mover/LocalizedMover with a full docstring, then re-implemented with the same docstring copy-pasted verbatim in LocalMover, RemoteMover, and (for the calculate_moves* pair) LocalRemoteMover. Keep the docs on the abstract declarations and have every concrete override point back at them instead. Co-Authored-By: Claude Sonnet 5 --- dandi/move.py | 66 ++++++++------------------------------------------- 1 file changed, 10 insertions(+), 56 deletions(-) diff --git a/dandi/move.py b/dandi/move.py index 887c7dc1f..95570de16 100644 --- a/dandi/move.py +++ b/dandi/move.py @@ -248,10 +248,7 @@ def resolve(self, path: str) -> tuple[AssetPath, bool]: def calculate_moves( self, *srcs: str, dest: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a sequence of input source paths and a destination path, return a - sorted list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves`.""" destpath, dest_is_dir = self.resolve(dest) destobj: File | Folder | None try: @@ -322,10 +319,7 @@ def calculate_moves( def calculate_moves_by_regex( self, find: str, replace: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a regular expression and a replacement string, return a sorted - list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves_by_regex`.""" rgx = re.compile(find) moves: dict[AssetPath, AssetPath] = {} rev: dict[AssetPath, AssetPath] = {} @@ -447,14 +441,7 @@ def placename(self) -> str: return "local" def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, str]]: - """ - Yield all available assets as ``(asset_path, relpath)`` pairs, where - ``asset_path`` is a ``/``-separated path relative to the root of the - Dandiset and ``relpath`` is a ``/``-separated path to that asset, - relative to `subpath` (For assets outside of `subpath`, ``relpath`` - starts with ``"../"``). If ``subpath_only`` is true, only assets - underneath `subpath` are returned. - """ + """See `LocalizedMover.get_assets`.""" root = self.dandiset_path if subpath_only: root /= self.subpath @@ -470,14 +457,7 @@ def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, st yield (AssetPath(df.path), relpath) def get_path(self, path: str, is_src: bool = True) -> File | Folder: - """ - Return the asset or folder of assets at ``path`` (relative to - `subpath`) as a `File` or `Folder` instance. If there is nothing at - the given path, raises `NotFoundError`. - - If the path points to a folder, its `~Folder.relcontents` attribute - will be populated iff ``is_src`` is given. - """ + """See `LocalizedMover.get_path`.""" rpath, needs_dir = self.resolve(path) p = self.dandiset_path / rpath if not os.path.lexists(p): @@ -525,10 +505,7 @@ def is_file(self, path: AssetPath) -> bool: ) def move(self, src: AssetPath, dest: AssetPath) -> None: - """ - Move the asset at path ``src`` to path ``dest`` (which can be assumed - to not exist) - """ + """See `LocalizedMover.move`.""" lgr.debug("Moving local file %r to %r", src, dest) target = self.dandiset_path / dest try: @@ -602,14 +579,7 @@ def placename(self) -> str: return "remote" def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, str]]: - """ - Yield all available assets as ``(asset_path, relpath)`` pairs, where - ``asset_path`` is a ``/``-separated path relative to the root of the - Dandiset and ``relpath`` is a ``/``-separated path to that asset, - relative to `subpath` (For assets outside of `subpath`, ``relpath`` - starts with ``"../"``). If ``subpath_only`` is true, only assets - underneath `subpath` are returned. - """ + """See `LocalizedMover.get_assets`.""" for path in self.assets.keys(): relpath = posixpath.relpath(path, self.subpath.as_posix()) if subpath_only and relpath.startswith("../"): @@ -617,14 +587,7 @@ def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, st yield (path, relpath) def get_path(self, path: str, is_src: bool = True) -> File | Folder: - """ - Return the asset or folder of assets at ``path`` (relative to - `subpath`) as a `File` or `Folder` instance. If there is nothing at - the given path, raises `NotFoundError`. - - If the path points to a folder, its `~Folder.relcontents` attribute - will be populated iff ``is_src`` is given. - """ + """See `LocalizedMover.get_path`.""" rpath, needs_dir = self.resolve(path) relcontents: list[str] = [] file_found = False @@ -680,10 +643,7 @@ def is_file(self, path: AssetPath) -> bool: return path in self.assets def move(self, src: AssetPath, dest: AssetPath) -> None: - """ - Move the asset at path ``src`` to path ``dest`` (which can be assumed - to not exist) - """ + """See `LocalizedMover.move`.""" lgr.debug("Moving remote asset %r to %r", src, dest) assert src in self.assets try: @@ -742,10 +702,7 @@ def status_field(self) -> str: def calculate_moves( self, *srcs: str, dest: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a sequence of input source paths and a destination path, return a - sorted list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves`.""" local_moves = self.local.calculate_moves(*srcs, dest=dest, existing=existing) remote_moves = self.remote.calculate_moves(*srcs, dest=dest, existing=existing) self.compare_moves(local_moves, remote_moves) @@ -754,10 +711,7 @@ def calculate_moves( def calculate_moves_by_regex( self, find: str, replace: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a regular expression and a replacement string, return a sorted - list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves_by_regex`.""" local_moves = self.local.calculate_moves_by_regex(find, replace, existing) remote_moves = self.remote.calculate_moves_by_regex(find, replace, existing) self.compare_moves(local_moves, remote_moves) From bb05ddfdeefbc0957610b9b888ad36606dda31bd Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:14:01 -0400 Subject: [PATCH 11/24] Deduplicate NWB fixture builders in tests/fixtures.py simple4_nwb/simple5_nwb built near-identical NWBFile+TimeSeries+Subject structures differing only in the TimeSeries unit and subject_id; extract _make_ambiguous_orientation_nwb(). organized_nwb_dir/organized_nwb_dir3 both ran `organize -f copy` on a given NWB fixture into a fresh dandiset dir; extract _organize_copy(). Co-Authored-By: Claude Sonnet 5 --- dandi/tests/fixtures.py | 67 +++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..bc56edbf3 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -138,14 +138,13 @@ def simple3_nwb( ) -@pytest.fixture(scope="session") -def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: - """With subject, subject_id, species, but including data orientation ambiguity.""" - +def _make_ambiguous_orientation_nwb( + tmp_path_factory: pytest.TempPathFactory, *, unit: str, subject_id: str +) -> Path: start_time = datetime(2017, 4, 3, 11, tzinfo=timezone.utc) time_series = pynwb.TimeSeries( name="test_time_series", - unit="test_units", + unit=unit, data=np.zeros(shape=(2, 100)), rate=1.0, ) @@ -155,7 +154,7 @@ def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: identifier="NWBE4", session_start_time=start_time, subject=Subject( - subject_id="mouse004", + subject_id=subject_id, age="P1D/", sex="O", species="Mus musculus", @@ -168,6 +167,14 @@ def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: return filename +@pytest.fixture(scope="session") +def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: + """With subject, subject_id, species, but including data orientation ambiguity.""" + return _make_ambiguous_orientation_nwb( + tmp_path_factory, unit="test_units", subject_id="mouse004" + ) + + @pytest.fixture(scope="session") def simple5_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: """ @@ -180,46 +187,28 @@ def simple5_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: https://github.com/NeurodataWithoutBorders/nwbinspector/issues/345#issuecomment-1459232718 """ - - start_time = datetime(2017, 4, 3, 11, tzinfo=timezone.utc) - time_series = pynwb.TimeSeries( - name="test_time_series", - unit="", - data=np.zeros(shape=(2, 100)), - rate=1.0, + return _make_ambiguous_orientation_nwb( + tmp_path_factory, unit="", subject_id="mouse001" ) - nwbfile = NWBFile( - session_description="some session", - identifier="NWBE4", - session_start_time=start_time, - subject=Subject( - subject_id="mouse001", - age="P1D/", - sex="O", - species="Mus musculus", - ), - ) - nwbfile.add_acquisition(time_series) - filename = tmp_path_factory.mktemp("simple4") / "simple4.nwb" - with pynwb.NWBHDF5IO(filename, "w") as io: - io.write(nwbfile, cache_spec=False) - return filename - -@pytest.fixture(scope="session") -def organized_nwb_dir( - simple2_nwb: Path, tmp_path_factory: pytest.TempPathFactory -) -> Path: +def _organize_copy(nwb_path: Path, tmp_path_factory: pytest.TempPathFactory) -> Path: tmp_path = tmp_path_factory.mktemp("organized_nwb_dir") (tmp_path / dandiset_metadata_file).write_text("{}\n") r = CliRunner().invoke( - organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(simple2_nwb)] + organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(nwb_path)] ) assert r.exit_code == 0, r.stdout return tmp_path +@pytest.fixture(scope="session") +def organized_nwb_dir( + simple2_nwb: Path, tmp_path_factory: pytest.TempPathFactory +) -> Path: + return _organize_copy(simple2_nwb, tmp_path_factory) + + @pytest.fixture(scope="session") def organized_nwb_dir2( simple1_nwb_metadata: dict[str, Any], @@ -252,13 +241,7 @@ def organized_nwb_dir2( def organized_nwb_dir3( simple4_nwb: Path, tmp_path_factory: pytest.TempPathFactory ) -> Path: - tmp_path = tmp_path_factory.mktemp("organized_nwb_dir") - (tmp_path / dandiset_metadata_file).write_text("{}\n") - r = CliRunner().invoke( - organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(simple4_nwb)] - ) - assert r.exit_code == 0, r.stdout - return tmp_path + return _organize_copy(simple4_nwb, tmp_path_factory) @pytest.fixture(scope="session") From ab56c4dc4d16c48d35920e209059fc05e144f1dd Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:16:20 -0400 Subject: [PATCH 12/24] Deduplicate test_move.py setup boilerplate Every test function repeated the same three lines (snapshot starting assets, monkeypatch.chdir to some directory, monkeypatch the API key env var). Extract _setup_move() and call it from all 34 test functions; the chdir target still varies per test. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_move.py | 164 ++++++++++++++------------------------- 1 file changed, 60 insertions(+), 104 deletions(-) diff --git a/dandi/tests/test_move.py b/dandi/tests/test_move.py index 7b8c99ede..e7ebd6ff0 100644 --- a/dandi/tests/test_move.py +++ b/dandi/tests/test_move.py @@ -33,6 +33,15 @@ def moving_dandiset(new_dandiset: SampleDandiset) -> SampleDandiset: return new_dandiset +def _setup_move( + monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, chdir_to: Path +) -> list[RemoteAsset]: + starting_assets = list(moving_dandiset.dandiset.get_assets()) + monkeypatch.chdir(chdir_to) + moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + return starting_assets + + def check_assets( sample_dandiset: SampleDandiset, starting_assets: list[RemoteAsset], @@ -170,9 +179,7 @@ def test_move( remapping: dict[str, str | None], work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( *srcs, dest=dest, @@ -192,9 +199,7 @@ def test_move_skip( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", @@ -219,9 +224,7 @@ def test_move_error( work_on: MoveWorkOn, kwargs: dict[str, Any], ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -246,9 +249,7 @@ def test_move_overwrite( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", @@ -273,9 +274,7 @@ def test_move_overwrite( def test_move_no_srcs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( dest="nowhere", @@ -289,9 +288,7 @@ def test_move_no_srcs( def test_move_regex_multisrcs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( r"\.txt", @@ -315,9 +312,7 @@ def test_move_multisrcs_file_dest( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -341,9 +336,7 @@ def test_move_folder_src_file_dest( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "subdir1", @@ -363,9 +356,7 @@ def test_move_nonexistent_src( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(NotFoundError) as excinfo: move( "file.txt", @@ -395,9 +386,7 @@ def test_move_file_slash_src( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -422,9 +411,7 @@ def test_move_file_slash_dest( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -443,9 +430,7 @@ def test_move_file_slash_dest( def test_move_regex_no_match( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "no-match", @@ -464,9 +449,7 @@ def test_move_regex_no_match( def test_move_regex_collision( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( r"^\w+/foo\.json$", @@ -492,9 +475,7 @@ def test_move_regex_some_to_self( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( r"(.+[123])/([^.]+)\.(.+)", dest=r"\1/\2.dat", @@ -534,9 +515,9 @@ def test_move_from_subdir( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "../file.txt", "apple.txt", @@ -564,9 +545,9 @@ def test_move_in_subdir( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "apple.txt", dest="macintosh.txt", @@ -590,9 +571,9 @@ def test_move_from_subdir_abspaths( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) with pytest.raises(NotFoundError) as excinfo: move( "file.txt", @@ -621,9 +602,9 @@ def test_move_from_subdir_as_dot( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) with pytest.raises(ValueError) as excinfo: move( ".", @@ -633,8 +614,7 @@ def test_move_from_subdir_as_dot( devel_debug=True, ) assert ( - str(excinfo.value) - == "Cannot move current working directory. " + str(excinfo.value) == "Cannot move current working directory. " "Change to a different directory before moving this location." ) check_assets(moving_dandiset, starting_assets, work_on, {}) @@ -648,9 +628,9 @@ def test_move_from_subdir_regex( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( r"\.txt", dest=".dat", @@ -676,9 +656,9 @@ def test_move_from_subdir_regex_no_changes( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( r"\.txt", dest=".txt", @@ -700,9 +680,7 @@ def test_move_dandiset_path( tmp_path: Path, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(tmp_path) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, tmp_path) move( "file.txt", "subdir2/banana.txt", @@ -730,9 +708,7 @@ def test_move_dandiset_url( tmp_path: Path, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(tmp_path) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, tmp_path) move( "file.txt", "subdir2/banana.txt", @@ -755,9 +731,7 @@ def test_move_dandiset_url( def test_move_work_on_auto( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, tmp_path: Path ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir2/banana.txt", @@ -796,9 +770,9 @@ def test_move_not_dandiset( def test_move_local_delete_empty_dirs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir4") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir4" + ) move( "../subdir1/apple.txt", "../subdir2/banana.txt", @@ -826,9 +800,7 @@ def test_move_both_src_path_not_in_local( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "subdir2" / "banana.txt").unlink() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "subdir2", @@ -851,9 +823,7 @@ def test_move_both_src_path_not_in_remote( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "subdir2" / "mango.txt").write_text("Mango\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "subdir2", @@ -876,9 +846,7 @@ def test_move_both_dest_path_not_in_remote( existing: MoveExisting, ) -> None: (moving_dandiset.dspath / "subdir2" / "file.txt").write_text("This is a file.\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -903,9 +871,7 @@ def test_move_both_dest_path_not_in_local( existing: MoveExisting, ) -> None: (moving_dandiset.dspath / "subdir2" / "banana.txt").unlink() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -932,9 +898,7 @@ def test_move_both_dest_mismatch( (moving_dandiset.dspath / "subdir1" / "apple.txt").unlink() (moving_dandiset.dspath / "subdir1" / "apple.txt").mkdir() (moving_dandiset.dspath / "subdir1" / "apple.txt" / "seeds").write_text("12345\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -962,9 +926,7 @@ def test_move_pyout( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", @@ -994,9 +956,7 @@ def test_move_pyout_dry_run( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", @@ -1020,9 +980,9 @@ def test_move_path_to_self( work_on: MoveWorkOn, ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "apple.txt", dest="../subdir1", @@ -1048,9 +1008,7 @@ def test_move_remote_dest_is_local_dir_sans_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", dest="newdir", @@ -1067,9 +1025,7 @@ def test_move_both_dest_is_local_dir_sans_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", dest="newdir", From 2441b12c94f11a0ace9cbe388d2b5267ddd033ce Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:19:26 -0400 Subject: [PATCH 13/24] Parametrize near-duplicate test_move.py scenarios - Merge test_move_skip/test_move_overwrite into test_move_existing, parametrized on the existing= value and expected remapping. - Merge test_move_pyout/test_move_pyout_dry_run into one test parametrized on dry_run and the expected remapping. - Merge test_move_file_slash_src/test_move_file_slash_dest into test_move_file_slash, parametrized on which path (src or dest) gets the trailing slash. Remaining near-identical pairs in this file (dandiset-by-path vs dandiset-by-URL addressing, and the AssetMismatchError scenarios under work_on=BOTH) test genuinely different setups/expected messages; merging them would obscure what each case actually asserts, so they're left as intrinsic test duplication. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_move.py | 141 ++++++++++++++------------------------- 1 file changed, 49 insertions(+), 92 deletions(-) diff --git a/dandi/tests/test_move.py b/dandi/tests/test_move.py index e7ebd6ff0..f5868f457 100644 --- a/dandi/tests/test_move.py +++ b/dandi/tests/test_move.py @@ -194,10 +194,27 @@ def test_move( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_skip( +@pytest.mark.parametrize( + "existing,remapping", + [ + (MoveExisting.SKIP, {"file.txt": "subdir5/file.txt"}), + ( + MoveExisting.OVERWRITE, + { + "file.txt": "subdir5/file.txt", + "subdir4/foo.json": "subdir5/foo.json", + "subdir5/foo.json": None, + }, + ), + ], + ids=["skip", "overwrite"], +) +def test_move_existing( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + existing: MoveExisting, + remapping: dict[str, str | None], ) -> None: starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( @@ -205,13 +222,11 @@ def test_move_skip( "subdir4/foo.json", dest="subdir5", work_on=work_on, - existing=MoveExisting.SKIP, + existing=existing, dandi_instance=moving_dandiset.api.instance_id, devel_debug=True, ) - check_assets( - moving_dandiset, starting_assets, work_on, {"file.txt": "subdir5/file.txt"} - ) + check_assets(moving_dandiset, starting_assets, work_on, remapping) @pytest.mark.parametrize( @@ -241,36 +256,6 @@ def test_move_error( check_assets(moving_dandiset, starting_assets, work_on, {}) -@pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] -) -def test_move_overwrite( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) - move( - "file.txt", - "subdir4/foo.json", - dest="subdir5", - work_on=work_on, - existing=MoveExisting.OVERWRITE, - devel_debug=True, - dandi_instance=moving_dandiset.api.instance_id, - ) - check_assets( - moving_dandiset, - starting_assets, - work_on, - { - "file.txt": "subdir5/file.txt", - "subdir4/foo.json": "subdir5/foo.json", - "subdir5/foo.json": None, - }, - ) - - def test_move_no_srcs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: @@ -381,41 +366,26 @@ def test_move_nonexistent_src( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_file_slash_src( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) - with pytest.raises(ValueError) as excinfo: - move( - "file.txt", - "subdir1/apple.txt/", - dest="subdir2/", - work_on=work_on, - dandi_instance=moving_dandiset.api.instance_id, - ) - path_type = "Remote" if work_on == MoveWorkOn.REMOTE else "Local" - assert str(excinfo.value) == ( - f"{path_type} path 'subdir1/apple.txt/' is a file but a directory " - "was expected. Use a path ending with '/' for directories." - ) - check_assets(moving_dandiset, starting_assets, work_on, {}) - - @pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] + "srcs,dest", + [ + (["file.txt", "subdir1/apple.txt/"], "subdir2/"), + (["file.txt"], "subdir1/apple.txt/"), + ], + ids=["src", "dest"], ) -def test_move_file_slash_dest( +def test_move_file_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + srcs: list[str], + dest: str, ) -> None: starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( - "file.txt", - dest="subdir1/apple.txt/", + *srcs, + dest=dest, work_on=work_on, dandi_instance=moving_dandiset.api.instance_id, ) @@ -921,40 +891,27 @@ def test_move_both_dest_mismatch( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_pyout( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) - move( - "file.txt", - "subdir4/foo.json", - dest="subdir5", - work_on=work_on, - existing=MoveExisting.OVERWRITE, - devel_debug=False, - dandi_instance=moving_dandiset.api.instance_id, - ) - check_assets( - moving_dandiset, - starting_assets, - work_on, - { - "file.txt": "subdir5/file.txt", - "subdir4/foo.json": "subdir5/foo.json", - "subdir5/foo.json": None, - }, - ) - - @pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] + "dry_run,remapping", + [ + ( + False, + { + "file.txt": "subdir5/file.txt", + "subdir4/foo.json": "subdir5/foo.json", + "subdir5/foo.json": None, + }, + ), + (True, {}), + ], + ids=["moved", "dry_run"], ) -def test_move_pyout_dry_run( +def test_move_pyout( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + dry_run: bool, + remapping: dict[str, str | None], ) -> None: starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( @@ -964,10 +921,10 @@ def test_move_pyout_dry_run( work_on=work_on, existing=MoveExisting.OVERWRITE, devel_debug=False, - dry_run=True, + dry_run=dry_run, dandi_instance=moving_dandiset.api.instance_id, ) - check_assets(moving_dandiset, starting_assets, work_on, {}) + check_assets(moving_dandiset, starting_assets, work_on, remapping) @pytest.mark.parametrize( From 59d17c68a504f63821080a34bab834ed9b9acbe5 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:23:45 -0400 Subject: [PATCH 14/24] Deduplicate test_download.py setup/assertion boilerplate - test_download_sync/test_download_sync_do: extract the shared "delete file.txt, move dandiset dir into tmp_path, patch abbrev_prompt" setup into _prep_sync_download(). - test_download_sync_folder/test_download_sync_glob: extract the shared "delete two assets, patch abbrev_prompt" setup into _prep_sync_delete_two_assets(). - test_download_glob_option/test_download_glob_url: extract the shared post-download assertions into _assert_glob_download(). - test__check_attempts_and_sleep_retries: two inline scenarios only differed in the Retry-After/now timestamps and whether a sleep was expected; factor into a local check_retry_after() helper. The remaining duplication in this file is a large existing @pytest.mark.parametrize data table for progress-event aggregation (intrinsic to the parametrize pattern itself, not code duplication) plus one small residual signature/call overlap between two tests that exercise genuinely different URL addressing schemes. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_download.py | 123 ++++++++++++++++++----------------- 1 file changed, 64 insertions(+), 59 deletions(-) diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 43bb9579f..65dfa530f 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -443,15 +443,27 @@ def test_download_asset_by_equal_prefix( assert (tmp_path / "apple.txt").read_text() == "Apple\n" -@pytest.mark.parametrize("confirm", [True, False]) -def test_download_sync( - confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path -) -> None: +def _prep_sync_download( + mocker: MockerFixture, + text_dandiset: SampleDandiset, + tmp_path: Path, + return_value: str | None, +) -> tuple[Path, mock.MagicMock]: text_dandiset.dandiset.get_asset_by_path("file.txt").delete() dspath = tmp_path / text_dandiset.dandiset_id os.rename(text_dandiset.dspath, dspath) confirm_mock = mocker.patch( - "dandi.download.abbrev_prompt", return_value="yes" if confirm else "no" + "dandi.download.abbrev_prompt", return_value=return_value + ) + return dspath, confirm_mock + + +@pytest.mark.parametrize("confirm", [True, False]) +def test_download_sync( + confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path +) -> None: + dspath, confirm_mock = _prep_sync_download( + mocker, text_dandiset, tmp_path, "yes" if confirm else "no" ) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}", @@ -470,10 +482,7 @@ def test_download_sync( def test_download_sync_do( mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path ) -> None: - text_dandiset.dandiset.get_asset_by_path("file.txt").delete() - dspath = tmp_path / text_dandiset.dandiset_id - os.rename(text_dandiset.dspath, dspath) - confirm_mock = mocker.patch("dandi.download.abbrev_prompt") + dspath, confirm_mock = _prep_sync_download(mocker, text_dandiset, tmp_path, None) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}", tmp_path, @@ -484,12 +493,18 @@ def test_download_sync_do( assert not (dspath / "file.txt").exists() -def test_download_sync_folder( +def _prep_sync_delete_two_assets( mocker: MockerFixture, text_dandiset: SampleDandiset -) -> None: +) -> mock.MagicMock: text_dandiset.dandiset.get_asset_by_path("file.txt").delete() text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt").delete() - confirm_mock = mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + return mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + + +def test_download_sync_folder( + mocker: MockerFixture, text_dandiset: SampleDandiset +) -> None: + confirm_mock = _prep_sync_delete_two_assets(mocker, text_dandiset) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}/subdir2/", text_dandiset.dspath, @@ -1182,13 +1197,7 @@ def test_download_multiple_urls( ] -def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> None: - dandiset_id = text_dandiset.dandiset_id - download( - f"dandi://{text_dandiset.api.instance_id}/{dandiset_id}/s*.Txt", - tmp_path, - path_type=PathType.GLOB, - ) +def _assert_glob_download(tmp_path: Path) -> None: assert list_paths(tmp_path, dirs=True) == [ tmp_path / "subdir1", tmp_path / "subdir1" / "apple.txt", @@ -1201,26 +1210,25 @@ def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> assert (tmp_path / "subdir2" / "coconut.txt").read_text() == "Coconut\n" +def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> None: + dandiset_id = text_dandiset.dandiset_id + download( + f"dandi://{text_dandiset.api.instance_id}/{dandiset_id}/s*.Txt", + tmp_path, + path_type=PathType.GLOB, + ) + _assert_glob_download(tmp_path) + + def test_download_glob_url(text_dandiset: SampleDandiset, tmp_path: Path) -> None: download(f"{text_dandiset.dandiset.version_api_url}assets/?glob=s*.Txt", tmp_path) - assert list_paths(tmp_path, dirs=True) == [ - tmp_path / "subdir1", - tmp_path / "subdir1" / "apple.txt", - tmp_path / "subdir2", - tmp_path / "subdir2" / "banana.txt", - tmp_path / "subdir2" / "coconut.txt", - ] - assert (tmp_path / "subdir1" / "apple.txt").read_text() == "Apple\n" - assert (tmp_path / "subdir2" / "banana.txt").read_text() == "Banana\n" - assert (tmp_path / "subdir2" / "coconut.txt").read_text() == "Coconut\n" + _assert_glob_download(tmp_path) def test_download_sync_glob( mocker: MockerFixture, text_dandiset: SampleDandiset ) -> None: - text_dandiset.dandiset.get_asset_by_path("file.txt").delete() - text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt").delete() - confirm_mock = mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + confirm_mock = _prep_sync_delete_two_assets(mocker, text_dandiset) download( f"{text_dandiset.dandiset.version_api_url}assets/?glob=s*.Txt", text_dandiset.dspath, @@ -1502,31 +1510,28 @@ def test__check_attempts_and_sleep_retries(status_code: int) -> None: mock_sleep.assert_called_once() assert mock_sleep.call_args.args[0] > 0 - # shifted by 1 year! (too long) - response.headers["Retry-After"] = "Wed, 21 Oct 2016 07:28:00 GMT" - with mock.patch("time.sleep") as mock_sleep, mock.patch( - "dandi.utils.datetime" - ) as mock_datetime: - mock_datetime.datetime.now.return_value = parsedate_to_datetime( - "Wed, 21 Oct 2015 07:28:00 GMT" - ) - assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 - mock_sleep.assert_called_once() - # and we do sleep some time - assert mock_sleep.call_args.args[0] > 0 + def check_retry_after(retry_after: str, now: str, sleeps: bool) -> None: + response.headers["Retry-After"] = retry_after + with mock.patch("time.sleep") as mock_sleep, mock.patch( + "dandi.utils.datetime" + ) as mock_datetime: + mock_datetime.datetime.now.return_value = parsedate_to_datetime(now) + assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 + mock_sleep.assert_called_once() + if sleeps: + assert mock_sleep.call_args.args[0] > 0 + else: + assert not mock_sleep.call_args.args[0] + + # shifted by 1 year! (too long) -- we do sleep some time + check_retry_after( + "Wed, 21 Oct 2016 07:28:00 GMT", "Wed, 21 Oct 2015 07:28:00 GMT", sleeps=True + ) - # in the past second (too quick) - response.headers["Retry-After"] = "Wed, 21 Oct 2015 07:27:59 GMT" - with mock.patch("time.sleep") as mock_sleep, mock.patch( - "dandi.utils.datetime" - ) as mock_datetime: - mock_datetime.datetime.now.return_value = parsedate_to_datetime( - "Wed, 21 Oct 2015 07:28:00 GMT" - ) - assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 - mock_sleep.assert_called_once() - # and we do not sleep really - assert not mock_sleep.call_args.args[0] + # in the past second (too quick) -- we do not sleep really + check_retry_after( + "Wed, 21 Oct 2015 07:27:59 GMT", "Wed, 21 Oct 2015 07:28:00 GMT", sleeps=False + ) # ---------- Partial Zarr download tests ---------- @@ -1553,9 +1558,9 @@ def test_download_zarr_with_glob_filter( all_files = list_paths(zarr_dir) assert len(all_files) > 0 for f in all_files: - assert f.name.startswith(".z") or f.name == "zarr.json", ( - f"Non-metadata file downloaded: {f}" - ) + assert ( + f.name.startswith(".z") or f.name == "zarr.json" + ), f"Non-metadata file downloaded: {f}" @pytest.mark.ai_generated From dd2b48198c60051e7f4892e8cf6d29f0dc2407b1 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:27:44 -0400 Subject: [PATCH 15/24] Deduplicate test_dandiapi.py assertion boilerplate - test_get_content_url/test_get_content_url_regex: extract the shared "stream the content URL to asset.nwb" tail into _fetch_content(). - test_get_dandiset_lazy/non_lazy/no_version_id/published/ published_no_version_id/published_draft/published_other_version: extract the shared post-fetch field assertions (version_id, created, modified, version, most_recent_published_version, draft_version, contact_person -- including dropping an accidentally double-asserted `isinstance(dandiset.created, datetime)` line) into _assert_dandiset_fields(). The remaining duplication is mock-setup scaffolding across the test_authenticate_* variants, each exercising a genuinely different keyring/input code path, plus small residual call-site overlap between tests whose only real content is a differing (version_id, most_recent_published) pair. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_dandiapi.py | 108 ++++++++++++----------------------- 1 file changed, 35 insertions(+), 73 deletions(-) diff --git a/dandi/tests/test_dandiapi.py b/dandi/tests/test_dandiapi.py index 8da28852e..9f0152b14 100644 --- a/dandi/tests/test_dandiapi.py +++ b/dandi/tests/test_dandiapi.py @@ -230,6 +230,13 @@ def test_authenticate_bad_key_keyring_good_key_input( confirm_mock.assert_called_once_with("API key is invalid; enter another?") +def _fetch_content(client: DandiAPIClient, url: str, tmp_path: Path) -> None: + r = client.get(url, stream=True, json_resp=False) + with open(tmp_path / "asset.nwb", "wb") as fp: + for chunk in r.iter_content(chunk_size=8192): + fp.write(chunk) + + @mark.skipif_no_network def test_get_content_url(tmp_path: Path) -> None: with DandiAPIClient.for_dandi_instance("dandi") as client: @@ -243,10 +250,7 @@ def test_get_content_url(tmp_path: Path) -> None: + UUID_PATTERN.rstrip("$") + "/download/?$", url, ) - r = client.get(url, stream=True, json_resp=False) - with open(tmp_path / "asset.nwb", "wb") as fp: - for chunk in r.iter_content(chunk_size=8192): - fp.write(chunk) + _fetch_content(client, url, tmp_path) @mark.skipif_no_network @@ -256,10 +260,7 @@ def test_get_content_url_regex(tmp_path: Path) -> None: "sub-RAT123/sub-RAT123.nwb" ) url = asset.get_content_url(r"amazonaws.com/.*blobs/") - r = client.get(url, stream=True, json_resp=False) - with open(tmp_path / "asset.nwb", "wb") as fp: - for chunk in r.iter_content(chunk_size=8192): - fp.write(chunk) + _fetch_content(client, url, tmp_path) @mark.skipif_no_network @@ -645,6 +646,25 @@ def test_search_get_dandisets( assert ds.dandiset_id not in [d.identifier for d in dandisets] +def _assert_dandiset_fields( + dandiset: RemoteDandiset, version_id: str, most_recent_published: str | None +) -> None: + assert dandiset.version_id == version_id + assert isinstance(dandiset.created, datetime) + assert isinstance(dandiset.modified, datetime) + assert isinstance(dandiset.version, Version) + assert dandiset.version.identifier == version_id + if most_recent_published is None: + assert dandiset.most_recent_published_version is None + else: + assert isinstance(dandiset.most_recent_published_version, Version) + assert ( + dandiset.most_recent_published_version.identifier == most_recent_published + ) + assert isinstance(dandiset.draft_version, Version) + assert isinstance(dandiset.contact_person, str) + + def test_get_dandiset_lazy( mocker: MockerFixture, text_dandiset: SampleDandiset ) -> None: @@ -657,13 +677,7 @@ def test_get_dandiset_lazy( assert isinstance(dandiset.created, datetime) get_spy.assert_called_once() get_spy.reset_mock() - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) get_spy.assert_not_called() @@ -677,30 +691,14 @@ def test_get_dandiset_non_lazy( get_spy.reset_mock() assert dandiset.version_id == DRAFT get_spy.assert_not_called() - assert isinstance(dandiset.created, datetime) - get_spy.assert_not_called() - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) get_spy.assert_not_called() @pytest.mark.parametrize("lazy", [True, False]) def test_get_dandiset_no_version_id(lazy: bool, text_dandiset: SampleDandiset) -> None: dandiset = text_dandiset.client.get_dandiset(text_dandiset.dandiset_id, lazy=lazy) - assert dandiset.version_id == DRAFT - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) versions = list(dandiset.get_versions()) assert len(versions) == 1 assert versions[0].identifier == DRAFT @@ -712,16 +710,7 @@ def test_get_dandiset_published(lazy: bool, text_dandiset: SampleDandiset) -> No d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, v, lazy=lazy) - assert dandiset.version_id == v - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -735,16 +724,7 @@ def test_get_dandiset_published_no_version_id( d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, lazy=lazy) - assert dandiset.version_id == v - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -758,16 +738,7 @@ def test_get_dandiset_published_draft( d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, DRAFT, lazy=lazy) - assert dandiset.version_id == DRAFT - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -788,16 +759,7 @@ def test_get_dandiset_published_other_version( assert v1 != v2 dandiset = text_dandiset.client.get_dandiset(d.identifier, v1, lazy=lazy) - assert dandiset.version_id == v1 - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v1 - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v2 - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v1, v2) versions = list(dandiset.get_versions()) assert len(versions) == 3 From 765a11b30d2a10ef0ed1d089508fa570d5916fdb Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:30:17 -0400 Subject: [PATCH 16/24] Deduplicate "no such asset/dandiset" tests in test_dandiarchive.py The 10 test_get_nonexistent_* tests all parsed a URL, asserted get_assets() returns empty, and asserted strict=True raises a NotFoundError with a specific message; extract this into _assert_no_assets(). Also factor the repeated "No such Dandiset: '999999'..." message into NO_SUCH_DANDISET_999999. test_utils.py: test_get_instance_dandi_with_api and test_get_instance_url mocked an identical server-info JSON payload; factor it into the SERVER_INFO constant. The remaining cross-file overlap between test_dandiarchive.py and test_utils.py is two mock server-info payloads that happen to share structural JSON keys but carry different actual data (different version/services) -- coincidental, not real duplication. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_dandiarchive.py | 142 ++++++++++++------------------- dandi/tests/test_utils.py | 40 +++------ 2 files changed, 70 insertions(+), 112 deletions(-) diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 1154b025b..0754f5323 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -6,6 +6,7 @@ import responses from dandi.consts import DandiInstance, known_instances +from dandi.dandiapi import DandiAPIClient from dandi.dandiarchive import ( AssetFolderURL, AssetGlobURL, @@ -663,6 +664,25 @@ def test_parse_arbitrary_api_url() -> None: ) +def _assert_no_assets( + url: str, client: DandiAPIClient, expected_message: str, use_list: bool = False +) -> None: + parsed_url = parse_dandi_url(url) + assert list(parsed_url.get_assets(client)) == [] + with pytest.raises(NotFoundError) as excinfo: + if use_list: + list(parsed_url.get_assets(client, strict=True)) + else: + next(parsed_url.get_assets(client, strict=True)) + assert str(excinfo.value) == expected_message + + +NO_SUCH_DANDISET_999999 = ( + "No such Dandiset: '999999'. " + "Verify the Dandiset ID is correct and that you have access. " +) + + @pytest.mark.parametrize("version_suffix", ["", "@draft", "@0.999999.9999"]) def test_get_nonexistent_dandiset( local_dandi_api: DandiAPI, version_suffix: str @@ -673,17 +693,8 @@ def test_get_nonexistent_dandiset( parsed_url.get_dandiset(client) # No error with pytest.raises(NotFoundError) as excinfo: parsed_url.get_dandiset(client, lazy=False) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + assert str(excinfo.value) == NO_SUCH_DANDISET_999999 + _assert_no_assets(url, client, NO_SUCH_DANDISET_999999) @pytest.mark.parametrize("version", ["draft", "0.999999.9999"]) @@ -694,15 +705,7 @@ def test_get_nonexistent_dandiset_asset_id( f"{local_dandi_api.api_url}/dandisets/999999/versions/{version}" "/assets/00000000-0000-0000-0000-000000000000/" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_dandiset_nonexistent_asset_id(text_dandiset: SampleDandiset) -> None: @@ -711,28 +714,22 @@ def test_get_dandiset_nonexistent_asset_id(text_dandiset: SampleDandiset) -> Non f"{text_dandiset.dandiset_id}/versions/draft/assets/" "00000000-0000-0000-0000-000000000000/" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + text_dandiset.client, "No such asset: '00000000-0000-0000-0000-000000000000' for" - f" DANDI-API-LOCAL-DOCKER-TESTS:{text_dandiset.dandiset_id}/draft" + f" DANDI-API-LOCAL-DOCKER-TESTS:{text_dandiset.dandiset_id}/draft", ) def test_get_nonexistent_asset_id(local_dandi_api: DandiAPI) -> None: url = f"{local_dandi_api.api_url}/assets/00000000-0000-0000-0000-000000000000/" - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + local_dandi_api.client, "No such asset: '00000000-0000-0000-0000-000000000000'. " "Verify the asset ID is correct. " - "Use 'dandi ls' to list available assets." + "Use 'dandi ls' to list available assets.", ) @@ -741,15 +738,7 @@ def test_get_nonexistent_dandiset_asset_path( local_dandi_api: DandiAPI, version_suffix: str ) -> None: url = f"dandi://{local_dandi_api.instance_id}/999999{version_suffix}/does/not/exist" - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_path(text_dandiset: SampleDandiset) -> None: @@ -757,15 +746,12 @@ def test_get_nonexistent_asset_path(text_dandiset: SampleDandiset) -> None: f"dandi://{text_dandiset.api.instance_id}/" f"{text_dandiset.dandiset_id}/does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + text_dandiset.client, "No asset at path 'does/not/exist' in version draft. " "Verify the path is correct and the asset exists in this version. " - "Use 'dandi ls' to list available assets." + "Use 'dandi ls' to list available assets.", ) @@ -777,15 +763,7 @@ def test_get_nonexistent_dandiset_asset_folder( f"dandi://{local_dandi_api.instance_id}/999999{version_suffix}" "/does/not/exist/" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_folder(text_dandiset: SampleDandiset) -> None: @@ -793,12 +771,12 @@ def test_get_nonexistent_asset_folder(text_dandiset: SampleDandiset) -> None: f"dandi://{text_dandiset.api.instance_id}/" f"{text_dandiset.dandiset_id}/does/not/exist/" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found under folder 'does/not/exist/'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found under folder 'does/not/exist/'", + use_list=True, + ) @pytest.mark.parametrize("version", ["draft", "0.999999.9999"]) @@ -809,15 +787,7 @@ def test_get_nonexistent_dandiset_asset_prefix( f"{local_dandi_api.api_url}/dandisets/999999/versions/{version}" "/assets/?path=does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_prefix(text_dandiset: SampleDandiset) -> None: @@ -825,12 +795,12 @@ def test_get_nonexistent_asset_prefix(text_dandiset: SampleDandiset) -> None: f"{text_dandiset.api.api_url}/dandisets/" f"{text_dandiset.dandiset_id}/versions/draft/assets/?path=does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found with path prefix 'does/not/exist'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found with path prefix 'does/not/exist'", + use_list=True, + ) def test_get_nonexistent_asset_glob(text_dandiset: SampleDandiset) -> None: @@ -838,12 +808,12 @@ def test_get_nonexistent_asset_glob(text_dandiset: SampleDandiset) -> None: f"{text_dandiset.api.api_url}/dandisets/" f"{text_dandiset.dandiset_id}/versions/draft/assets/?glob=d*/*/*st" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found matching glob 'd*/*/*st'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found matching glob 'd*/*/*st'", + use_list=True, + ) @pytest.mark.parametrize( diff --git a/dandi/tests/test_utils.py b/dandi/tests/test_utils.py index 3a15043c5..1208e6351 100644 --- a/dandi/tests/test_utils.py +++ b/dandi/tests/test_utils.py @@ -172,21 +172,22 @@ def test_flatten() -> None: ] +SERVER_INFO = { + "version": "1.0.0", + "cli-minimal-version": "0.5.0", + "cli-bad-versions": [], + "services": { + "webui": {"url": "https://gui.dandi"}, + "api": {"url": "https://api.dandi"}, + "jupyterhub": {"url": "https://hub.dandi"}, + }, +} + + @responses.activate def test_get_instance_dandi_with_api() -> None: responses.add( - responses.GET, - "https://api.dandiarchive.org/api/info/", - json={ - "version": "1.0.0", - "cli-minimal-version": "0.5.0", - "cli-bad-versions": [], - "services": { - "webui": {"url": "https://gui.dandi"}, - "api": {"url": "https://api.dandi"}, - "jupyterhub": {"url": "https://hub.dandi"}, - }, - }, + responses.GET, "https://api.dandiarchive.org/api/info/", json=SERVER_INFO ) _get_instance.cache_clear() assert get_instance("dandi") == DandiInstance( @@ -198,20 +199,7 @@ def test_get_instance_dandi_with_api() -> None: @responses.activate def test_get_instance_url() -> None: - responses.add( - responses.GET, - "https://example.dandi/server-info", - json={ - "version": "1.0.0", - "cli-minimal-version": "0.5.0", - "cli-bad-versions": [], - "services": { - "webui": {"url": "https://gui.dandi"}, - "api": {"url": "https://api.dandi"}, - "jupyterhub": {"url": "https://hub.dandi"}, - }, - }, - ) + responses.add(responses.GET, "https://example.dandi/server-info", json=SERVER_INFO) _get_instance.cache_clear() assert get_instance("https://example.dandi/") == DandiInstance( name="api.dandi", From e661ab6df1c39d57713f390745c8eda3cbdc7d88 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:34:27 -0400 Subject: [PATCH 17/24] Deduplicate test_metadata.py session-duration and nwb2asset tests - The 5 test_session_duration_with_* tests shared the same "session_start_time/session_end_time present, duration within 1s of expected" check; extract _assert_session_duration(). Two of them additionally shared the same "Session activity has matching start/end dates" check; extract _assert_session_activity_dates(). - test_nwb2asset and test_nwb2asset_remote_asset asserted against the same large BareAsset.model_construct(...) golden object, differing only in contentSize/digest/path/blobDateModified/strain; extract _expected_simple2_asset(). The remaining duplication is the metadata2asset @pytest.mark.parametrize data table (literal test-fixture dicts covering distinct corner cases, explicitly labeled as such in a comment) -- intrinsic to the parametrize pattern, not code duplication. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_metadata.py | 207 +++++++++++------------------------ 1 file changed, 65 insertions(+), 142 deletions(-) diff --git a/dandi/tests/test_metadata.py b/dandi/tests/test_metadata.py index 26f29acbb..2770bf057 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -489,6 +489,31 @@ def test_time_extract_gest() -> None: ) +def _assert_session_duration(metadata: dict[str, Any], expected_seconds: float) -> None: + assert "session_start_time" in metadata + assert "session_end_time" in metadata + duration = ( + metadata["session_end_time"] - metadata["session_start_time"] + ).total_seconds() + assert abs(duration - expected_seconds) < 1.0 # Allow small floating point errors + + +def _assert_session_activity_dates(nwb_path: Path, metadata: dict[str, Any]) -> None: + # Check that Session activity includes endDate + asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) + assert asset.wasGeneratedBy is not None + + # Find Session activities + sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] + assert len(sessions) > 0 + + session = sessions[0] + assert session.startDate is not None + assert session.endDate is not None + assert session.startDate == metadata["session_start_time"] + assert session.endDate == metadata["session_end_time"] + + @pytest.mark.ai_generated def test_session_duration_extraction(tmp_path: Path) -> None: """Test that session duration is extracted and included in Session activity""" @@ -522,29 +547,9 @@ def test_session_duration_extraction(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Calculate duration - should be 150 seconds (max) - 0 seconds (min) - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 150.0) < 1.0 # Allow small floating point errors - - # Check that Session activity includes endDate - asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) - assert asset.wasGeneratedBy is not None - - # Find Session activities - sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] - assert len(sessions) > 0 - - session = sessions[0] - assert session.startDate is not None - assert session.endDate is not None - assert session.startDate == metadata["session_start_time"] - assert session.endDate == metadata["session_end_time"] + _assert_session_duration(metadata, 150.0) + _assert_session_activity_dates(nwb_path, metadata) @pytest.mark.ai_generated @@ -580,29 +585,9 @@ def test_session_duration_with_trials(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Calculate duration - should be 200 (max from trials) - 5 (min from trials) = 195 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 195.0) < 1.0 # Allow small floating point errors - - # Check that Session activity includes endDate - asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) - assert asset.wasGeneratedBy is not None - - # Find Session activities - sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] - assert len(sessions) > 0 - - session = sessions[0] - assert session.startDate is not None - assert session.endDate is not None - assert session.startDate == metadata["session_start_time"] - assert session.endDate == metadata["session_end_time"] + _assert_session_duration(metadata, 195.0) + _assert_session_activity_dates(nwb_path, metadata) @pytest.mark.ai_generated @@ -636,15 +621,8 @@ def test_session_duration_with_units(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Duration should be 250 (max spike) - 5 (min spike) = 245 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 245.0) < 1.0 # Allow small floating point errors + _assert_session_duration(metadata, 245.0) @pytest.mark.ai_generated @@ -670,16 +648,12 @@ def test_session_duration_with_scattered_non_spiking_units(tmp_path: Path) -> No io.write(nwbfile) metadata = get_metadata(nwb_path) - assert "session_start_time" in metadata - assert "session_end_time" in metadata end_offset = (metadata["session_end_time"] - session_start).total_seconds() assert abs(end_offset - 245.0) < 1.0 - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 245.0) < 1.0 # max 250s and min 5s spike times + # max 250s and min 5s spike times + _assert_session_duration(metadata, 245.0) @pytest.mark.ai_generated @@ -734,15 +708,8 @@ def test_session_duration_with_events(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Duration should be 180 (100 + 80, max end) - 3 (min timestamp) = 177 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 157.0) < 1.0 # Allow small floating point errors + _assert_session_duration(metadata, 157.0) @mark_xfail_ontobee @@ -1259,11 +1226,20 @@ def test_ndtypes(ndtypes, asset_dict): assert metadata.variableMeasured[0].value == asset_dict.get(key)[0] -@mark.skipif_no_network -def test_nwb2asset(simple2_nwb: Path) -> None: +def _expected_simple2_asset( + *, + content_size: Any, + digest_value: str, + path: str, + blob_date_modified: Any, + strain: StrainType | None = None, +) -> BareAsset: + participant_kwargs: dict[str, Any] = {} + if strain is not None: + participant_kwargs["strain"] = strain # Classes with ANY_AWARE_DATETIME fields need to be constructed with # model_construct() - assert nwb2asset(simple2_nwb, digest=DUMMY_DANDI_ETAG) == BareAsset.model_construct( + return BareAsset.model_construct( schemaVersion=DANDI_SCHEMA_VERSION, keywords=["keyword1", "keyword 2"], access=[ @@ -1299,12 +1275,12 @@ def test_nwb2asset(simple2_nwb: Path) -> None: ], ), ], - contentSize=ANY_INT, + contentSize=content_size, encodingFormat="application/x-nwb", - digest={DigestType.dandi_etag: "dddddddddddddddddddddddddddddddd-1"}, - path=str(simple2_nwb), + digest={DigestType.dandi_etag: digest_value}, + path=path, dateModified=ANY_AWARE_DATETIME, - blobDateModified=ANY_AWARE_DATETIME, + blobDateModified=blob_date_modified, wasAttributedTo=[ Participant( identifier="mouse001", @@ -1324,7 +1300,7 @@ def test_nwb2asset(simple2_nwb: Path) -> None: identifier="http://purl.obolibrary.org/obo/NCBITaxon_10090", name="Mus musculus - House mouse", ), - strain=StrainType(schemaKey="StrainType", name="C57BL/6J"), + **participant_kwargs, ), ], variableMeasured=[], @@ -1334,6 +1310,17 @@ def test_nwb2asset(simple2_nwb: Path) -> None: ) +@mark.skipif_no_network +def test_nwb2asset(simple2_nwb: Path) -> None: + assert nwb2asset(simple2_nwb, digest=DUMMY_DANDI_ETAG) == _expected_simple2_asset( + content_size=ANY_INT, + digest_value="dddddddddddddddddddddddddddddddd-1", + path=str(simple2_nwb), + blob_date_modified=ANY_AWARE_DATETIME, + strain=StrainType(schemaKey="StrainType", name="C57BL/6J"), + ) + + @pytest.mark.timeout(120) @pytest.mark.xfail(reason="https://github.com/dandi/dandi-cli/issues/1450") def test_nwb2asset_remote_asset(nwb_dandiset: SampleDandiset) -> None: @@ -1343,73 +1330,9 @@ def test_nwb2asset_remote_asset(nwb_dandiset: SampleDandiset) -> None: mtime = ensure_datetime(asset.get_raw_metadata()["blobDateModified"]) assert isinstance(asset, RemoteBlobAsset) r = asset.as_readable() - # Classes with ANY_AWARE_DATETIME fields need to be constructed with - # model_construct() - assert nwb2asset(r, digest=digest) == BareAsset.model_construct( - schemaVersion=DANDI_SCHEMA_VERSION, - keywords=["keyword1", "keyword 2"], - access=[ - AccessRequirements( - schemaKey="AccessRequirements", status=AccessType.OpenAccess - ) - ], - wasGeneratedBy=[ - Session.model_construct( - schemaKey="Session", - identifier="session_id1", - name="session_id1", - description="session_description1", - startDate=ANY_AWARE_DATETIME, - ), - Activity.model_construct( - id=AnyFullmatch( - r"urn:uuid:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}" - ), - schemaKey="Activity", - name="Metadata generation", - description="Metadata generated by DANDI cli", - startDate=ANY_AWARE_DATETIME, - endDate=ANY_AWARE_DATETIME, - wasAssociatedWith=[ - Software( - schemaKey="Software", - identifier="RRID:SCR_019009", - name="DANDI Command Line Interface", - version=__version__, - url="https://github.com/dandi/dandi-cli", - ) - ], - ), - ], - contentSize=ByteSize(asset.size), - encodingFormat="application/x-nwb", - digest={DigestType.dandi_etag: digest.value}, + assert nwb2asset(r, digest=digest) == _expected_simple2_asset( + content_size=ByteSize(asset.size), + digest_value=digest.value, path="sub-mouse001.nwb", - dateModified=ANY_AWARE_DATETIME, - blobDateModified=mtime, - wasAttributedTo=[ - Participant( - identifier="mouse001", - schemaKey="Participant", - age=PropertyValue( - schemaKey="PropertyValue", - unitText="ISO-8601 duration", - value="P135DT43200S", - valueReference=PropertyValue( - schemaKey="PropertyValue", - value=AgeReferenceType.BirthReference, - ), - ), - sex=SexType(schemaKey="SexType", name="Unknown"), - species=SpeciesType( - schemaKey="SpeciesType", - identifier="http://purl.obolibrary.org/obo/NCBITaxon_10090", - name="Mus musculus - House mouse", - ), - ), - ], - variableMeasured=[], - measurementTechnique=[], - approach=[], - relatedResource=[], + blob_date_modified=mtime, ) From 552690208cfad54103dc4a972988e4bbbe13e9b7 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:36:10 -0400 Subject: [PATCH 18/24] Deduplicate cli/tests/test_download.py mock-call assertions Every test asserted dandi.download.download() was called with the same 10-kwarg set (existing/format/jobs/.../path_type), varying only one or two values. Factor the defaults into DEFAULT_DOWNLOAD_KWARGS and assert with `**{**DEFAULT_DOWNLOAD_KWARGS, "some_key": override}`. test_download_gui_instance_in_dandiset and test_download_api_instance_in_dandiset additionally shared their whole setup/invoke/assert body apart from the instance name and expected URL; extract _invoke_in_dandiset(). Co-Authored-By: Claude Sonnet 5 --- dandi/cli/tests/test_download.py | 133 +++++++++---------------------- 1 file changed, 37 insertions(+), 96 deletions(-) diff --git a/dandi/cli/tests/test_download.py b/dandi/cli/tests/test_download.py index 85fa33f79..e75377bf1 100644 --- a/dandi/cli/tests/test_download.py +++ b/dandi/cli/tests/test_download.py @@ -9,45 +9,32 @@ from ...consts import dandiset_metadata_file, known_instances from ...download import DownloadExisting, DownloadFormat, PathType +DEFAULT_DOWNLOAD_KWARGS = { + "existing": DownloadExisting.ERROR, + "format": DownloadFormat.PYOUT, + "jobs": 6, + "jobs_per_zarr": None, + "get_metadata": True, + "get_assets": True, + "preserve_tree": False, + "sync": None, + "zarr_filters": (), + "path_type": PathType.EXACT, +} + def test_download_defaults(mocker): mock_download = mocker.patch("dandi.download.download") r = CliRunner().invoke(download) assert r.exit_code == 0 - mock_download.assert_called_once_with( - (), - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, - ) + mock_download.assert_called_once_with((), os.curdir, **DEFAULT_DOWNLOAD_KWARGS) def test_download_all_types(mocker): mock_download = mocker.patch("dandi.download.download") r = CliRunner().invoke(download, ["--download", "all"]) assert r.exit_code == 0 - mock_download.assert_called_once_with( - (), - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, - ) + mock_download.assert_called_once_with((), os.curdir, **DEFAULT_DOWNLOAD_KWARGS) def test_download_metadata_only(mocker): @@ -55,18 +42,7 @@ def test_download_metadata_only(mocker): r = CliRunner().invoke(download, ["--download", "dandiset.yaml"]) assert r.exit_code == 0 mock_download.assert_called_once_with( - (), - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=False, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, + (), os.curdir, **{**DEFAULT_DOWNLOAD_KWARGS, "get_assets": False} ) @@ -75,18 +51,7 @@ def test_download_assets_only(mocker): r = CliRunner().invoke(download, ["--download", "assets"]) assert r.exit_code == 0 mock_download.assert_called_once_with( - (), - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=False, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, + (), os.curdir, **{**DEFAULT_DOWNLOAD_KWARGS, "get_metadata": False} ) @@ -102,27 +67,26 @@ def test_download_bad_type(mocker): mock_download.assert_not_called() -def test_download_gui_instance_in_dandiset(mocker, tmp_path, monkeypatch): +def _invoke_in_dandiset(mocker, tmp_path, monkeypatch, instance, expected_url): mock_download = mocker.patch("dandi.download.download") runner = CliRunner() with monkeypatch.context() as m: m.chdir(tmp_path) Path(dandiset_metadata_file).write_text("identifier: '123456'\n") - r = runner.invoke(download, ["-i", "dandi"]) + r = runner.invoke(download, ["-i", instance]) assert r.exit_code == 0 mock_download.assert_called_once_with( - ["https://dandiarchive.org/#/dandiset/123456/draft"], - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, + [expected_url], os.curdir, **DEFAULT_DOWNLOAD_KWARGS + ) + + +def test_download_gui_instance_in_dandiset(mocker, tmp_path, monkeypatch): + _invoke_in_dandiset( + mocker, + tmp_path, + monkeypatch, + "dandi", + "https://dandiarchive.org/#/dandiset/123456/draft", ) @@ -131,26 +95,12 @@ def test_download_gui_instance_in_dandiset(mocker, tmp_path, monkeypatch): reason="this instance now has GUI URL", ) def test_download_api_instance_in_dandiset(mocker, tmp_path, monkeypatch): - mock_download = mocker.patch("dandi.download.download") - runner = CliRunner() - with monkeypatch.context() as m: - m.chdir(tmp_path) - Path(dandiset_metadata_file).write_text("identifier: '123456'\n") - r = runner.invoke(download, ["-i", "dandi-api-local-docker-tests"]) - assert r.exit_code == 0 - mock_download.assert_called_once_with( - ["http://localhost:8000/api/dandisets/123456/"], - os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, + _invoke_in_dandiset( + mocker, + tmp_path, + monkeypatch, + "dandi-api-local-docker-tests", + "http://localhost:8000/api/dandisets/123456/", ) @@ -168,16 +118,7 @@ def test_download_url_instance_match(mocker): mock_download.assert_called_once_with( ("http://localhost:8000/api/dandisets/123456/",), os.curdir, - existing=DownloadExisting.ERROR, - format=DownloadFormat.PYOUT, - jobs=6, - jobs_per_zarr=None, - get_metadata=True, - get_assets=True, - preserve_tree=False, - sync=None, - zarr_filters=(), - path_type=PathType.EXACT, + **DEFAULT_DOWNLOAD_KWARGS, ) From e8e5f7571c180a8c15a7e61f2a1bb3252f21c80f Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:39:34 -0400 Subject: [PATCH 19/24] Deduplicate test_delete.py setup/assertion boilerplate Most tests repeated "monkeypatch the API key env var, grab instance_id, spy on RESTFullAPIClient.delete"; extract _setup_delete_spy(), taking either a text_dandiset.api or local_dandi_api (both DandiAPI instances). The skip_missing variants additionally repeated a "download and compare remaining asset paths" tail; extract _assert_remaining_assets(). The 3 remaining pairs (single-path vs whole-dandiset delete; path-confirm vs dandiset-confirm; missing-asset vs missing-folder) only share the mechanical call shape around genuinely different scenarios and assertions, so they're left as intrinsic duplication. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_delete.py | 112 +++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 60 deletions(-) diff --git a/dandi/tests/test_delete.py b/dandi/tests/test_delete.py index 9a6ff8dd1..b64483af7 100644 --- a/dandi/tests/test_delete.py +++ b/dandi/tests/test_delete.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +from typing import Any import pytest from pytest_mock import MockerFixture @@ -14,6 +15,24 @@ from ..utils import list_paths +def _setup_delete_spy( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, api: DandiAPI +) -> tuple[str, Any]: + api.monkeypatch_set_api_key_env(monkeypatch) + delete_spy = mocker.spy(RESTFullAPIClient, "delete") + return api.instance_id, delete_spy + + +def _assert_remaining_assets( + text_dandiset: SampleDandiset, tmp_path: Path, remaining: list[Path] +) -> None: + download(text_dandiset.dandiset.version_api_url, tmp_path) + dandiset_id = text_dandiset.dandiset_id + assert list_paths(tmp_path) == [ + tmp_path / dandiset_id / f for f in [Path("dandiset.yaml")] + remaining + ] + + @pytest.mark.parametrize( "paths,remainder", [ @@ -67,10 +86,8 @@ def test_delete_paths( remainder: list[Path], ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [p.format(instance=instance, dandiset_id=dandiset_id) for p in paths], dandi_instance=instance, @@ -78,10 +95,7 @@ def test_delete_paths( force=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / f for f in [Path("dandiset.yaml")] + remainder - ] + _assert_remaining_assets(text_dandiset, tmp_path, remainder) @pytest.mark.parametrize("confirm", [True, False]) @@ -92,10 +106,8 @@ def test_delete_path_confirm( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") confirm_mock = mocker.patch("click.confirm", return_value=confirm) delete(["subdir2/coconut.txt"], dandi_instance=instance, devel_debug=True) confirm_mock.assert_called_with( @@ -113,9 +125,7 @@ def test_delete_path_pyout( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) delete(["subdir2/coconut.txt"], dandi_instance=instance, force=True) delete_spy.assert_called() @@ -143,10 +153,8 @@ def test_delete_dandiset( paths: list[str], ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [p.format(instance=instance, dandiset_id=dandiset_id) for p in paths], dandi_instance=instance, @@ -166,10 +174,8 @@ def test_delete_dandiset_confirm( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") confirm_mock = mocker.patch("click.confirm", return_value=confirm) delete( [f"dandi://{instance}/{dandiset_id}"], dandi_instance=instance, devel_debug=True @@ -187,11 +193,9 @@ def test_delete_dandiset_mismatch( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id not_dandiset = str(int(dandiset_id) - 1).zfill(6) - delete_spy = mocker.spy(RESTFullAPIClient, "delete") for paths in [ [ "subdir1/apple.txt", @@ -216,10 +220,8 @@ def test_delete_instance_mismatch( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") for paths in [ [ "subdir1/apple.txt", @@ -242,9 +244,7 @@ def test_delete_instance_mismatch( def test_delete_nonexistent_dandiset( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) with pytest.raises(NotFoundError) as excinfo: delete( [f"dandi://{instance}/999999/subdir1/apple.txt"], @@ -262,9 +262,7 @@ def test_delete_nonexistent_dandiset( def test_delete_nonexistent_dandiset_skip_missing( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) delete( [f"dandi://{instance}/999999/subdir1/apple.txt"], dandi_instance=instance, @@ -280,10 +278,8 @@ def test_delete_nonexistent_asset( monkeypatch: pytest.MonkeyPatch, text_dandiset: SampleDandiset, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") with pytest.raises(NotFoundError) as excinfo: delete( [ @@ -307,10 +303,8 @@ def test_delete_nonexistent_asset_skip_missing( text_dandiset: SampleDandiset, tmp_path: Path, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [ f"dandi://{instance}/{dandiset_id}/file.txt", @@ -322,13 +316,15 @@ def test_delete_nonexistent_asset_skip_missing( skip_missing=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / "dandiset.yaml", - tmp_path / dandiset_id / "subdir1" / "apple.txt", - tmp_path / dandiset_id / "subdir2" / "banana.txt", - tmp_path / dandiset_id / "subdir2" / "coconut.txt", - ] + _assert_remaining_assets( + text_dandiset, + tmp_path, + [ + Path("subdir1", "apple.txt"), + Path("subdir2", "banana.txt"), + Path("subdir2", "coconut.txt"), + ], + ) def test_delete_nonexistent_asset_folder( @@ -336,10 +332,8 @@ def test_delete_nonexistent_asset_folder( monkeypatch: pytest.MonkeyPatch, text_dandiset: SampleDandiset, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") with pytest.raises(NotFoundError) as excinfo: delete( [ @@ -363,10 +357,8 @@ def test_delete_nonexistent_asset_folder_skip_missing( text_dandiset: SampleDandiset, tmp_path: Path, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [ f"dandi://{instance}/{dandiset_id}/subdir1/", @@ -378,21 +370,21 @@ def test_delete_nonexistent_asset_folder_skip_missing( skip_missing=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / "dandiset.yaml", - tmp_path / dandiset_id / "file.txt", - tmp_path / dandiset_id / "subdir2" / "banana.txt", - tmp_path / dandiset_id / "subdir2" / "coconut.txt", - ] + _assert_remaining_assets( + text_dandiset, + tmp_path, + [ + Path("file.txt"), + Path("subdir2", "banana.txt"), + Path("subdir2", "coconut.txt"), + ], + ) def test_delete_version( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) with pytest.raises(NotImplementedError) as excinfo: delete( [f"dandi://{instance}/999999@draft"], From 3b33aeb4f33447ddb5eba2218baed2f19019ade8 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:42:08 -0400 Subject: [PATCH 20/24] Deduplicate expected-asset literals in test_files.py test_find_dandi_files asserted three overlapping find_dandi_files() result lists that repeated the same VideoAsset/ImageAsset/ZarrAsset/ NWBAsset/DandisetMetadataFile literals; factor them into local variables reused across the three assertions. test_find_dandi_files_with_bids similarly repeated each BIDS asset literal between the top-level files list and the per-dataset dataset_files checks; factor into local variables (bidsignore, bids1_dd, bids1_file, ..., bids2_subdd). Equality is value-based (dataclasses), so reusing the same instance across multiple assertions is safe. Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_files.py | 294 ++++++++++++++------------------------ 1 file changed, 110 insertions(+), 184 deletions(-) diff --git a/dandi/tests/test_files.py b/dandi/tests/test_files.py index bc73aece2..00723eed1 100644 --- a/dandi/tests/test_files.py +++ b/dandi/tests/test_files.py @@ -66,37 +66,40 @@ def test_find_dandi_files(tmp_path: Path) -> None: ".ignored.dir/ignored.nwb", ) + video = VideoAsset( + filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path + ) + image = ImageAsset( + filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path + ) + zarr1 = ZarrAsset( + filepath=tmp_path / "sample01.zarr", + path="sample01.zarr", + dandiset_path=tmp_path, + ) + nwb2 = NWBAsset( + filepath=tmp_path / "sample02.nwb", + path="sample02.nwb", + dandiset_path=tmp_path, + ) + nwb3 = NWBAsset( + filepath=tmp_path / "subdir" / "sample03.nwb", + path="subdir/sample03.nwb", + dandiset_path=tmp_path, + ) + zarr4 = ZarrAsset( + filepath=tmp_path / "subdir" / "sample04.zarr", + path="subdir/sample04.zarr", + dandiset_path=tmp_path, + ) + metadata_file = DandisetMetadataFile( + filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path + ) + files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path), key=attrgetter("filepath") ) - assert files == [ - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), - ] + assert files == [video, image, zarr1, nwb2, nwb3, zarr4] files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, allow_all=True), @@ -106,26 +109,12 @@ def test_find_dandi_files(tmp_path: Path) -> None: GenericAsset( filepath=tmp_path / "bar.txt", path="bar.txt", dandiset_path=tmp_path ), - DandisetMetadataFile( - filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path - ), + metadata_file, GenericAsset(filepath=tmp_path / "foo", path="foo", dandiset_path=tmp_path), - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), + video, + image, + zarr1, + nwb2, GenericAsset( filepath=tmp_path / "subdir" / "cleesh.txt", path="subdir/cleesh.txt", @@ -136,53 +125,15 @@ def test_find_dandi_files(tmp_path: Path) -> None: path="subdir/gnusto", dandiset_path=tmp_path, ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), + nwb3, + zarr4, ] files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, include_metadata=True), key=attrgetter("filepath"), ) - assert files == [ - DandisetMetadataFile( - filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path - ), - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), - ] + assert files == [metadata_file, video, image, zarr1, nwb2, nwb3, zarr4] def test_find_dandi_files_with_bids(tmp_path: Path) -> None: @@ -202,6 +153,61 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: "bids2/subbids/data.json", ) + bidsignore = GenericBIDSAsset( + filepath=tmp_path / "bids1" / ".bidsignore", + path="bids1/.bidsignore", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_dd = BIDSDatasetDescriptionAsset( + filepath=tmp_path / "bids1" / "dataset_description.json", + path="bids1/dataset_description.json", + dandiset_path=tmp_path, + dataset_files=ANY, # type: ignore[arg-type] + ) + bids1_file = GenericBIDSAsset( + filepath=tmp_path / "bids1" / "file.txt", + path="bids1/file.txt", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_zarr = ZarrBIDSAsset( + filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", + path="bids1/subdir/glarch.zarr", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_nwb = NWBBIDSAsset( + filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", + path="bids1/subdir/quux.nwb", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_dd = BIDSDatasetDescriptionAsset( + filepath=tmp_path / "bids2" / "dataset_description.json", + path="bids2/dataset_description.json", + dandiset_path=tmp_path, + dataset_files=ANY, # type: ignore[arg-type] + ) + bids2_movie = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "movie.mp4", + path="bids2/movie.mp4", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_data = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "subbids" / "data.json", + path="bids2/subbids/data.json", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_subdd = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", + path="bids2/subbids/dataset_description.json", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, allow_all=False), key=attrgetter("filepath"), @@ -209,89 +215,24 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: assert files == [ NWBAsset(filepath=tmp_path / "bar.nwb", path="bar.nwb", dandiset_path=tmp_path), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / ".bidsignore", - path="bids1/.bidsignore", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - BIDSDatasetDescriptionAsset( - filepath=tmp_path / "bids1" / "dataset_description.json", - path="bids1/dataset_description.json", - dandiset_path=tmp_path, - dataset_files=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / "file.txt", - path="bids1/file.txt", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - ZarrBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", - path="bids1/subdir/glarch.zarr", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - NWBBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", - path="bids1/subdir/quux.nwb", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - BIDSDatasetDescriptionAsset( - filepath=tmp_path / "bids2" / "dataset_description.json", - path="bids2/dataset_description.json", - dandiset_path=tmp_path, - dataset_files=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "movie.mp4", - path="bids2/movie.mp4", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "data.json", - path="bids2/subbids/data.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", - path="bids2/subbids/dataset_description.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bidsignore, + bids1_dd, + bids1_file, + bids1_zarr, + bids1_nwb, + bids2_dd, + bids2_movie, + bids2_data, + bids2_subdd, ] bidsdd = files[2] assert isinstance(bidsdd, BIDSDatasetDescriptionAsset) assert sorted(bidsdd.dataset_files, key=attrgetter("filepath")) == [ - GenericBIDSAsset( - filepath=tmp_path / "bids1" / ".bidsignore", - path="bids1/.bidsignore", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / "file.txt", - path="bids1/file.txt", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - ZarrBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", - path="bids1/subdir/glarch.zarr", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - NWBBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", - path="bids1/subdir/quux.nwb", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bidsignore, + bids1_file, + bids1_zarr, + bids1_nwb, ] for asset in bidsdd.dataset_files: assert asset.bids_dataset_description is bidsdd @@ -299,24 +240,9 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: bidsdd = files[6] assert isinstance(bidsdd, BIDSDatasetDescriptionAsset) assert sorted(bidsdd.dataset_files, key=attrgetter("filepath")) == [ - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "movie.mp4", - path="bids2/movie.mp4", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "data.json", - path="bids2/subbids/data.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", - path="bids2/subbids/dataset_description.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bids2_movie, + bids2_data, + bids2_subdd, ] for asset in bidsdd.dataset_files: assert asset.bids_dataset_description is bidsdd From f6bb3b98b24b47b0ca79cf594ce09cd607937198 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:43:31 -0400 Subject: [PATCH 21/24] Deduplicate test_keyring.py "no keyring" mock setup 6 tests repeated "delenv PYTHON_KEYRING_BACKEND, patch dandi.keyring_utils.get_keyring" with only the returned backend instance varying; extract _mock_no_keyring(). Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_keyring.py | 40 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/dandi/tests/test_keyring.py b/dandi/tests/test_keyring.py index 9a7d4dab9..8ae57b0fe 100644 --- a/dandi/tests/test_keyring.py +++ b/dandi/tests/test_keyring.py @@ -2,8 +2,9 @@ from collections.abc import Callable from pathlib import Path +from unittest.mock import MagicMock -from keyring.backend import get_all_keyring +from keyring.backend import KeyringBackend, get_all_keyring from keyring.backends import fail, null from keyring.errors import KeyringError from keyrings.alt import file as keyfile @@ -170,14 +171,18 @@ class EncryptedFailure(fail.Keyring, keyfile.EncryptedKeyring): pass +def _mock_no_keyring( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, keyring: KeyringBackend +) -> MagicMock: + monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) + return mocker.patch("dandi.keyring_utils.get_keyring", return_value=keyring) + + @pytest.mark.usefixtures("tmp_home") def test_keyring_lookup_fail_default_encrypted( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=EncryptedFailure() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, EncryptedFailure()) with pytest.raises(KeyringError): keyring_lookup("testservice", "testusername") get_keyring.assert_called_once_with() @@ -187,10 +192,7 @@ def test_keyring_lookup_fail_default_encrypted( def test_keyring_lookup_encrypted_fallback_exists_no_password( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) kf = Path(keyfile.EncryptedKeyring().file_path) kf.parent.mkdir(parents=True, exist_ok=True) kf.touch() @@ -204,10 +206,7 @@ def test_keyring_lookup_encrypted_fallback_exists_no_password( def test_keyring_lookup_encrypted_fallback_exists_password( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) kb0 = keyfile.EncryptedKeyring() getpass = mocker.patch("getpass.getpass", return_value="file-password") kb0.set_password("testservice", "testusername", "testpassword") @@ -224,10 +223,7 @@ def test_keyring_lookup_encrypted_fallback_exists_password( def test_keyring_lookup_encrypted_fallback_not_exists_no_create( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=False) with pytest.raises(KeyringError): keyring_lookup("testservice", "testusername") @@ -241,10 +237,7 @@ def test_keyring_lookup_encrypted_fallback_not_exists_no_create( def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=True) kb, password = keyring_lookup("testservice", "testusername") assert isinstance(kb, keyfile.EncryptedKeyring) @@ -263,10 +256,7 @@ def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf( def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf_exists( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=True) rc = keyringrc_file() rc.parent.mkdir(parents=True, exist_ok=True) From f1be3e858ff4c73f561fb8a77826c0377d6d0103 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:45:42 -0400 Subject: [PATCH 22/24] Deduplicate test_upload.py zarr/dandidownload test bodies - test_upload_different_zarr/test_upload_loose_zarr shared the "replace the local zarr, re-upload, verify same zarr id, download and compare" tail; extract _replace_zarr_and_check(). - test_upload_rejects_dandidownload_paths's "Test 1" sub-case and test_upload_rejects_dandidownload_nwb_file both built an NWB file and expected upload() to reject it with the same message; extract _assert_upload_rejects_dandidownload(). - test_upload_zarr_patch_mode_no_delete/test_upload_zarr_full_mode_delete shared the "find and remove a local zarr data file" setup; extract _remove_a_zarr_data_file(). Co-Authored-By: Claude Sonnet 5 --- dandi/tests/test_upload.py | 102 ++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 59 deletions(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 22e1a09de..eb436cd4c 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -466,10 +466,9 @@ def test_upload_bids_zarr( bids_zarr_dandiset.upload() -def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - zarr_id = asset.zarr +def _replace_zarr_and_check( + tmp_path: Path, zarr_dandiset: SampleDandiset, zarr_id: str +) -> None: rmtree(zarr_dandiset.dspath / "sample.zarr") zarr.save(zarr_dandiset.dspath / "sample.zarr", np.eye(5)) zarr_dandiset.upload() @@ -483,22 +482,18 @@ def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> ) +def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: + asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") + assert isinstance(asset, RemoteZarrAsset) + _replace_zarr_and_check(tmp_path, zarr_dandiset, asset.zarr) + + def test_upload_loose_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") assert isinstance(asset, RemoteZarrAsset) zarr_id = asset.zarr asset.delete() - rmtree(zarr_dandiset.dspath / "sample.zarr") - zarr.save(zarr_dandiset.dspath / "sample.zarr", np.eye(5)) - zarr_dandiset.upload() - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - assert asset.zarr == zarr_id - download(zarr_dandiset.dandiset.version_api_url, tmp_path) - assert_dirtrees_eq( - zarr_dandiset.dspath / "sample.zarr", - tmp_path / zarr_dandiset.dandiset_id / "sample.zarr", - ) + _replace_zarr_and_check(tmp_path, zarr_dandiset, zarr_id) def test_upload_different_zarr_entry_conflicts( @@ -832,30 +827,35 @@ def mock_put(self, url, **kwargs): ), "All-same-type failures should be flagged as systematic" -@pytest.mark.ai_generated -def test_upload_rejects_dandidownload_paths( - new_dandiset: SampleDandiset, tmp_path: Path +def _assert_upload_rejects_dandidownload( + new_dandiset: SampleDandiset, badfile_path: Path, identifier: str ) -> None: - """Test that upload rejects assets with .dandidownload paths""" - dspath = new_dandiset.dspath - - # Test 1: Regular file with .dandidownload in path - badfile_path = dspath / f"test{DOWNLOAD_SUFFIX}" / "file.nwb" - badfile_path.parent.mkdir(parents=True) make_nwb_file( badfile_path, session_description="test session", - identifier="test123", + identifier=identifier, session_start_time=datetime(2017, 4, 15, 12, tzinfo=timezone.utc), subject=pynwb.file.Subject(subject_id="test"), ) - with pytest.raises( UploadError, match=f"contains {DOWNLOAD_SUFFIX} path which indicates incomplete download", ): new_dandiset.upload(allow_any_path=True) + +@pytest.mark.ai_generated +def test_upload_rejects_dandidownload_paths( + new_dandiset: SampleDandiset, tmp_path: Path +) -> None: + """Test that upload rejects assets with .dandidownload paths""" + dspath = new_dandiset.dspath + + # Test 1: Regular file with .dandidownload in path + badfile_path = dspath / f"test{DOWNLOAD_SUFFIX}" / "file.nwb" + badfile_path.parent.mkdir(parents=True) + _assert_upload_rejects_dandidownload(new_dandiset, badfile_path, "test123") + # Clean up for next test rmtree(badfile_path.parent) @@ -900,37 +900,15 @@ def test_upload_rejects_dandidownload_paths( @pytest.mark.ai_generated def test_upload_rejects_dandidownload_nwb_file(new_dandiset: SampleDandiset) -> None: """Test that upload rejects NWB files with .dandidownload in their path""" - dspath = new_dandiset.dspath - # Create an NWB file with .dandidownload in its name - bad_nwb_path = dspath / f"test{DOWNLOAD_SUFFIX}.nwb" - make_nwb_file( - bad_nwb_path, - session_description="test session", - identifier="test456", - session_start_time=datetime(2017, 4, 15, 12, tzinfo=timezone.utc), - subject=pynwb.file.Subject(subject_id="test"), - ) - - with pytest.raises( - UploadError, - match=f"contains {DOWNLOAD_SUFFIX} path which indicates incomplete download", - ): - new_dandiset.upload(allow_any_path=True) + bad_nwb_path = new_dandiset.dspath / f"test{DOWNLOAD_SUFFIX}.nwb" + _assert_upload_rejects_dandidownload(new_dandiset, bad_nwb_path, "test456") # ---------- Partial Zarr upload (patch mode) tests ---------- -@pytest.mark.ai_generated -def test_upload_zarr_patch_mode_no_delete( - tmp_path: Path, zarr_dandiset: SampleDandiset -) -> None: - """Upload with patch mode should NOT delete remote-only files.""" - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - - # Delete a local file so it becomes remote-only +def _remove_a_zarr_data_file(zarr_dandiset: SampleDandiset) -> str: local_zarr = zarr_dandiset.dspath / "sample.zarr" # Find and remove a data file (not .zgroup etc) data_files = [ @@ -940,6 +918,19 @@ def test_upload_zarr_patch_mode_no_delete( removed_file = data_files[0] removed_relpath = removed_file.relative_to(local_zarr).as_posix() removed_file.unlink() + return removed_relpath + + +@pytest.mark.ai_generated +def test_upload_zarr_patch_mode_no_delete( + tmp_path: Path, zarr_dandiset: SampleDandiset +) -> None: + """Upload with patch mode should NOT delete remote-only files.""" + asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") + assert isinstance(asset, RemoteZarrAsset) + + # Delete a local file so it becomes remote-only + removed_relpath = _remove_a_zarr_data_file(zarr_dandiset) # Upload in patch mode — remote file should be preserved zarr_dandiset.upload(zarr_mode="patch") @@ -961,14 +952,7 @@ def test_upload_zarr_full_mode_delete( assert isinstance(asset, RemoteZarrAsset) # Delete a local file - local_zarr = zarr_dandiset.dspath / "sample.zarr" - data_files = [ - f for f in list_paths(local_zarr) if f.is_file() and not f.name.startswith(".") - ] - assert data_files, "Expected data files in zarr" - removed_file = data_files[0] - removed_relpath = removed_file.relative_to(local_zarr).as_posix() - removed_file.unlink() + removed_relpath = _remove_a_zarr_data_file(zarr_dandiset) # Upload in full mode — remote file should be deleted zarr_dandiset.upload(zarr_mode="full") From ef2fda4b8d9797ce367441f00e0ce7a783c0eb58 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 29 Sep 2026 15:48:06 -0400 Subject: [PATCH 23/24] Deduplicate sample ValidationResult issues in test_cmd_validate.py test_render_text_grouping and test_render_text_multilevel_grouping built the same two-issue ValidationResult list; extract _sample_validation_issues() and have the multilevel test append its extra ERROR-severity issue on top. The remaining test_cmd_validate.py/test_core.py overlap (test_validate_bids_error vs test_validate_bids_errors) exercises the same BIDS error-example scenario through two different entry points (the CLI command vs the underlying validate() function) with different assertions; left as intrinsic duplication across module/layer boundaries rather than merging two different-layer tests together. Co-Authored-By: Claude Sonnet 5 --- dandi/cli/tests/test_cmd_validate.py | 49 +++++++++------------------- dandi/cli/tests/test_digest.py | 23 +++++++------ 2 files changed, 27 insertions(+), 45 deletions(-) diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 6d0b503a0..9bcb395c0 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -345,13 +345,7 @@ def test_validate_load_mutual_exclusivity(simple2_nwb: Path, tmp_path: Path) -> assert "mutually exclusive" in r.output -@pytest.mark.ai_generated -@pytest.mark.parametrize( - "grouping", - ["severity", "id", "validator", "standard", "dandiset"], -) -def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> None: - """Test extended grouping renders section headers with counts.""" +def _sample_validation_issues() -> tuple[Origin, list[ValidationResult]]: origin = Origin( type=OriginType.VALIDATION, validator=Validator.nwbinspector, @@ -377,6 +371,17 @@ def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> N dandiset_path=Path("/data/ds001"), ), ] + return origin, issues + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "grouping", + ["severity", "id", "validator", "standard", "dandiset"], +) +def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> None: + """Test extended grouping renders section headers with counts.""" + _, issues = _sample_validation_issues() _render_text(issues, grouping=(grouping,)) captured = capsys.readouterr().out @@ -435,30 +440,8 @@ def test_validate_grouping_text_cli( @pytest.mark.ai_generated def test_render_text_multilevel_grouping(capsys: pytest.CaptureFixture) -> None: """Test multi-level grouping renders nested section headers.""" - origin = Origin( - type=OriginType.VALIDATION, - validator=Validator.nwbinspector, - validator_version="", - ) - issues = [ - ValidationResult( - id="NWBI.check_data_orientation", - origin=origin, - scope=Scope.FILE, - message="Data may be in the wrong orientation.", - path=Path("sub-01/sub-01.nwb"), - severity=Severity.WARNING, - dandiset_path=Path("/data/ds001"), - ), - ValidationResult( - id="NWBI.check_missing_unit", - origin=origin, - scope=Scope.FILE, - message="Missing text for attribute 'unit'.", - path=Path("sub-02/sub-02.nwb"), - severity=Severity.WARNING, - dandiset_path=Path("/data/ds001"), - ), + origin, issues = _sample_validation_issues() + issues.append( ValidationResult( id="NWBI.check_data_orientation", origin=origin, @@ -467,8 +450,8 @@ def test_render_text_multilevel_grouping(capsys: pytest.CaptureFixture) -> None: path=Path("sub-03/sub-03.nwb"), severity=Severity.ERROR, dandiset_path=Path("/data/ds001"), - ), - ] + ) + ) _render_text(issues, grouping=("severity", "id")) captured = capsys.readouterr().out diff --git a/dandi/cli/tests/test_digest.py b/dandi/cli/tests/test_digest.py index 9c8fcccbf..00fd01ad0 100644 --- a/dandi/cli/tests/test_digest.py +++ b/dandi/cli/tests/test_digest.py @@ -57,9 +57,9 @@ def test_digest( assert r.output == f"file.txt: {filehash}\n" -def test_digest_zarr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - # Expected digest is selected by the Zarr serialisation format that - # ``zarr.save`` actually produced (V2 vs V3 layouts have different digests). +def _make_sample_zarr( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> tuple[CliRunner, str]: runner = CliRunner() monkeypatch.chdir(tmp_path) dt = np.dtype(" None: expected = _EXPECTED_SAMPLE_ZARR_DIGEST_BY_FORMAT[ zarr_format_of(Path("sample.zarr")) ] + return runner, expected + + +def test_digest_zarr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # Expected digest is selected by the Zarr serialisation format that + # ``zarr.save`` actually produced (V2 vs V3 layouts have different digests). + runner, expected = _make_sample_zarr(tmp_path, monkeypatch) r = runner.invoke(digest, ["--digest", "zarr-checksum", "sample.zarr"]) assert r.exit_code == 0 assert r.output == f"sample.zarr: {expected}\n" @@ -87,15 +94,7 @@ def test_digest_zarr_with_excluded_dotfiles( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: # See comment in `test_digest_zarr` regarding V2 vs V3 serialisation. - runner = CliRunner() - monkeypatch.chdir(tmp_path) - dt = np.dtype(" Date: Tue, 29 Sep 2026 15:49:18 -0400 Subject: [PATCH 24/24] Tighten duplication threshold to 2% after cleanup Baseline was 4.38% (133 clones) across dandi/ + tools/. After the mitigation commits on this branch, duplication is down to 1.46% (42 clones), all now residual and documented: - Intrinsic @pytest.mark.parametrize data tables (test_download.py's progress-event table, test_metadata.py's metadata2asset corner cases) -- literal fixture data, not code duplication. - Intentional parallel-layer test coverage (public bids_validate() vs private _bids_validate(); CLI `validate` command vs core validate() function; single-path vs whole-dandiset delete scenarios) where merging would conflate independent test layers. - Python-level unavoidable boilerplate (method-override signatures repeating parameter lists; Sphinx `.. versionchanged::` directives that must appear on each documented method). - One deliberately-decoupled duck-typing case (RemoteZarrEntry explicitly stopped inheriting misctypes.BasePath in 0.48.0; the residual is just per-class property-delegation boilerplate). Threshold set to 2% (just above the current 1.46%) rather than 0% to leave headroom for this residual without treating it as a bug. Co-Authored-By: Claude Sonnet 5 --- .jscpd.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.jscpd.json b/.jscpd.json index 6421b2211..0a74fb2cf 100644 --- a/.jscpd.json +++ b/.jscpd.json @@ -1,5 +1,5 @@ { - "threshold": 5, + "threshold": 2, "reporters": ["console"], "ignore": [ "**/.git/**",