From de59d6f9e13ec002e9e4077c44ebad0ff157ba87 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 11:26:07 +0200 Subject: [PATCH 1/5] Restore 100% coverage and enforce it in codecov dev coverage slipped to 99.96% at #759, which added two uncovered defensive branches in the directory indexer. Codecov did not block it: codecov.yml never set an explicit coverage target, so the default `auto` status only compares against the base commit, and a partial base upload made the drop read as an increase. - Add explicit project + patch `target: 100%` (threshold 0%) so the gate is absolute instead of relative to a possibly-partial base. - Cover the two previously-missed indexer branches: the atomic-swap temp-file cleanup in `_update_index_map`, and the `_walk` stat guard that skips a file deleted between the walk and its stat. --- codecov.yml | 18 +++++++++++++ tests/test_io/test_indexer.py | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/codecov.yml b/codecov.yml index 70d65c692..67d5f05a2 100644 --- a/codecov.yml +++ b/codecov.yml @@ -5,6 +5,24 @@ codecov: comment: after_n_builds: 7 +# Require full coverage. `target: 100%` makes the gate absolute rather than +# relative to the base commit, so a regression cannot slip through when a base +# upload is partial (see the coverage drop introduced by #759). The gate is on +# combined coverage (all flags); it cannot be scoped to `unittests` alone +# because some lines are only exercised by the network suite. Note the network +# flag carries forward (below): if a network upload is skipped, its prior report +# is reused, so a regression in network-only code could still read as 100%. +coverage: + status: + project: + default: + target: 100% + threshold: 0% + patch: + default: + target: 100% + threshold: 0% + flags: unittests: carryforward: false diff --git a/tests/test_io/test_indexer.py b/tests/test_io/test_indexer.py index 26304f702..9ef551370 100644 --- a/tests/test_io/test_indexer.py +++ b/tests/test_io/test_indexer.py @@ -172,6 +172,54 @@ def test_get_reads_fresh_each_call(self, tmp_path): cache_path.write_text(json.dumps({"a": "1", "b": "2"})) assert _get_index_map(str(cache_path)) == {"a": "1", "b": "2"} + def test_failed_swap_cleans_up_temp(self, tmp_path, monkeypatch): + """A failure during the atomic swap unlinks the temp file and re-raises.""" + from dascore.io.index import indexer as indexer_mod + + cache_path = tmp_path / "cache_paths.json" + + def boom(*args, **kwargs): + raise RuntimeError("swap failed") + + monkeypatch.setattr(indexer_mod.os, "replace", boom) + with pytest.raises(RuntimeError, match="swap failed"): + indexer_mod._update_index_map({"a": "1"}, cache_path=str(cache_path)) + # No temp debris and no half-written target left behind. + assert list(tmp_path.iterdir()) == [] + + +class TestWalkResilience: + """Tests for the filesystem walk tolerating concurrent changes.""" + + def test_walk_skips_file_removed_mid_scan(self, tmp_path, monkeypatch): + """A file vanishing between the walk and its stat is skipped, not fatal.""" + (tmp_path / "good.h5").write_bytes(b"") + (tmp_path / "vanisher.h5").write_bytes(b"") + indexer = DBDirectoryIndexer(tmp_path) + # Keep the walk off the format-detection path; this test targets the + # stat guard, not directory-format probing of the root directory. + monkeypatch.setattr(indexer, "_directory_format", lambda path: False) + + real_is_dir = Path.is_dir + + def is_dir_then_vanish(self, *args, **kwargs): + # Delete the file immediately after the is_dir() probe so the walk's + # subsequent real stat() hits the concurrent-deletion guard. This + # reproduces the race without assuming how is_dir() is implemented. + result = real_is_dir(self, *args, **kwargs) + if self.name == "vanisher.h5": + self.unlink() + return result + + monkeypatch.setattr(Path, "is_dir", is_dir_then_vanish) + try: + walked = indexer._walk() + finally: + indexer.close() + names = {Path(entry[-1]).name for entry in walked.values()} + assert "good.h5" in names + assert "vanisher.h5" not in names + class TestBasics: """Basic tests for indexer.""" From 169ff00fdd68c3a2bbe414c57dca72b83172449e Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 12:02:54 +0200 Subject: [PATCH 2/5] Cover remaining branches so the unittests flag hits 100% Add non-network tests for every line that was previously exercised only by the network suite, so the deterministic unittests + doctest coverage reaches 100% on its own: - planned `_coord_record_from_row`: empty-string units normalize to None. - misc `suppress_warnings`: the message-filter branch. - remote_io `_warn_remote_cache_download`: metadata-scope guidance text. - prodml relative (non-datetime) time is rejected on write. - prodml non-positive optional measures are dropped rather than written. - prodml `_get_prodml_version_str` returns "" for a non-ProdML file. Mark the `_round_times_to_microseconds` NaT guard `# pragma: no cover`: a NaT breaks even sampling, so `_get_single_patch`'s evenly-sampled check rejects such coords first, making the guard unreachable via `dc.write`. Scope the codecov project + patch 100% gate to the `unittests` flag so it no longer depends on the carried-forward network upload; a skipped or stale network report can no longer mask a real coverage drop. --- codecov.yml | 13 ++++++++----- dascore/io/prodml/utils.py | 4 +++- tests/test_io/test_index/test_planned.py | 12 ++++++++++++ tests/test_io/test_prodml/test_prod_ml.py | 13 +++++++++++++ tests/test_io/test_prodml/test_prodml_write.py | 15 +++++++++++++++ tests/test_utils/test_io_utils.py | 8 ++++++++ tests/test_utils/test_misc.py | 11 +++++++++++ 7 files changed, 70 insertions(+), 6 deletions(-) diff --git a/codecov.yml b/codecov.yml index 67d5f05a2..c8b2df65b 100644 --- a/codecov.yml +++ b/codecov.yml @@ -7,21 +7,24 @@ comment: # Require full coverage. `target: 100%` makes the gate absolute rather than # relative to the base commit, so a regression cannot slip through when a base -# upload is partial (see the coverage drop introduced by #759). The gate is on -# combined coverage (all flags); it cannot be scoped to `unittests` alone -# because some lines are only exercised by the network suite. Note the network -# flag carries forward (below): if a network upload is skipped, its prior report -# is reused, so a regression in network-only code could still read as 100%. +# upload is partial (see the coverage drop introduced by #759). The gate is +# scoped to the `unittests` flag (the deterministic non-network + doctest +# suite, which covers every line): it never depends on the carried-forward +# `network` upload, so a skipped or stale network report cannot mask a drop. coverage: status: project: default: target: 100% threshold: 0% + flags: + - unittests patch: default: target: 100% threshold: 0% + flags: + - unittests flags: unittests: diff --git a/dascore/io/prodml/utils.py b/dascore/io/prodml/utils.py index 498baa881..a07ade919 100644 --- a/dascore/io/prodml/utils.py +++ b/dascore/io/prodml/utils.py @@ -275,7 +275,9 @@ def _round_times_to_microseconds(coord): if not np.issubdtype(values.dtype, np.datetime64) or len(values) < 2: msg = "ProdML writing requires at least two absolute time samples." raise PatchError(msg) - if np.any(np.isnat(values)): + # Defensive: a NaT breaks even-sampling, so _get_single_patch's + # require_evenly_sampled check rejects such coords before they reach here. + if np.any(np.isnat(values)): # pragma: no cover raise PatchError("ProdML time coordinates cannot contain NaT.") microsecond_values = values.astype("datetime64[us]") if np.array_equal(microsecond_values.astype(values.dtype), values): diff --git a/tests/test_io/test_index/test_planned.py b/tests/test_io/test_index/test_planned.py index 163017bdc..fdc375ea1 100644 --- a/tests/test_io/test_index/test_planned.py +++ b/tests/test_io/test_index/test_planned.py @@ -53,6 +53,18 @@ def test_coord_record_zero_step_length(self): record = _coord_record_from_row(row, "time") assert record.length is None + def test_coord_record_empty_units_dropped(self): + """An empty-string units cell normalizes to None (a real step keeps length).""" + row = { + "distance_min": 0.0, + "distance_max": 10.0, + "distance_step": 1.0, + "_distance_units": "", + } + record = _coord_record_from_row(row, "distance") + assert record.units is None + assert record.length == 11 + def test_plan_resolver_requires_output_id(self): """member_rows without output_id is a construction error.""" with pytest.raises(ValueError, match="output_id"): diff --git a/tests/test_io/test_prodml/test_prod_ml.py b/tests/test_io/test_prodml/test_prod_ml.py index 5e8ad374d..6cf6d84af 100644 --- a/tests/test_io/test_prodml/test_prod_ml.py +++ b/tests/test_io/test_prodml/test_prod_ml.py @@ -11,6 +11,7 @@ import dascore as dc from dascore.core.coords import get_coord from dascore.io.core import read +from dascore.io.prodml.utils import _get_prodml_version_str from dascore.utils.downloader import fetch @@ -107,3 +108,15 @@ def test_precision_of_time_array(self, quantx_v2_das_patch): time = quantx_v2_das_patch.coords.get_array("time") dtype = time.dtype assert "[ns]" in str(dtype) + + +class TestVersionDetection: + """Tests for the ProdML version fingerprint helper.""" + + def test_acquisition_without_expected_attrs(self, tmp_path): + """An Acquisition group lacking the fingerprint attrs is not ProdML.""" + path = tmp_path / "not_prodml.h5" + with h5py.File(path, "w") as file: + file.create_group("Acquisition").attrs["unrelated"] = "x" + with h5py.File(path, "r") as file: + assert _get_prodml_version_str(file) == "" diff --git a/tests/test_io/test_prodml/test_prodml_write.py b/tests/test_io/test_prodml/test_prodml_write.py index 1e87048e6..4da29aef7 100644 --- a/tests/test_io/test_prodml/test_prodml_write.py +++ b/tests/test_io/test_prodml/test_prodml_write.py @@ -423,6 +423,21 @@ def test_nat_time(self, prodml_patch, tmp_path): with pytest.raises(PatchError, match=r"NaT|time"): dc.write(patch, tmp_path / "nat.h5", "PRODML") + def test_relative_time_rejected(self, prodml_patch, tmp_path): + """A relative (non-absolute) time coordinate has no PRODML representation.""" + length = len(prodml_patch.get_coord("time")) + relative = dc.get_coord(data=np.arange(length) * 1.0, units="s") + patch = prodml_patch.new(coords=prodml_patch.coords.update(time=relative)) + with pytest.raises(PatchError, match="two absolute time samples"): + dc.write(patch, tmp_path / "relative_time.h5", "PRODML") + + def test_nonpositive_optional_measure_ignored(self, prodml_patch, tmp_path): + """A non-positive optional measure is dropped rather than written.""" + patch = prodml_patch.update_attrs(pulse_width=0.0, pulse_width_units="ns") + path = dc.write(patch, tmp_path / "nonpositive_measure.h5", "PRODML") + with h5py.File(path, "r") as file: + assert "PulseWidth" not in file["Acquisition"].attrs + def test_time_irregular_after_rounding(self, prodml_patch, tmp_path): """Uniform nanoseconds that round to irregular microseconds are invalid.""" base = np.datetime64("2020-01-01", "ns") diff --git a/tests/test_utils/test_io_utils.py b/tests/test_utils/test_io_utils.py index 2f80fe881..096ae4d17 100644 --- a/tests/test_utils/test_io_utils.py +++ b/tests/test_utils/test_io_utils.py @@ -34,6 +34,7 @@ from dascore.utils.remote_io import ( _FallbackFileObj, _get_cached_local_file, + _warn_remote_cache_download, clear_remote_file_cache, get_remote_cache_path, get_remote_cache_scope, @@ -849,6 +850,13 @@ def test_remote_cache_scope_restores_previous_value_after_exception(self): raise RuntimeError("boom") assert get_remote_cache_scope() == "default" + def test_metadata_scope_download_warning_guidance(self, tmp_path): + """Metadata scope yields metadata-specific download-warning guidance.""" + resource = UPath("memory://dascore/metadata_warning.txt") + with remote_cache_scope("metadata"): + with pytest.warns(UserWarning, match="allow_remote_cache_for_metadata"): + _warn_remote_cache_download(resource, tmp_path / "metadata_warning.txt") + class TestRemoteIOFallback: """Tests for remote fallback helpers.""" diff --git a/tests/test_utils/test_misc.py b/tests/test_utils/test_misc.py index 7d87e8764..8bc23f04b 100644 --- a/tests/test_utils/test_misc.py +++ b/tests/test_utils/test_misc.py @@ -4,6 +4,7 @@ import os import time +import warnings from io import BytesIO from pathlib import Path @@ -571,6 +572,16 @@ def test_nothing(self): warn_or_raise(msg, behavior=None) +class TestSuppressWarnings: + """Tests for the suppress_warnings context manager.""" + + def test_message_filter_applies_action(self): + """A message pattern applies the action to matching warnings.""" + with suppress_warnings(message="boom", action="error"): + with pytest.raises(UserWarning, match="boom"): + warnings.warn("boom", UserWarning) + + class TestToObjectArray: """Tests for converting a sequence of objects to an object array.""" From 9f580078a0da86f57b9bef00ade3acce79219ba9 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 12:19:28 +0200 Subject: [PATCH 3/5] Delete the unreachable ProdML NaT guard The NaT check in _round_times_to_microseconds can never fire: a NaT makes the time coordinate unevenly sampled, so _get_single_patch's require_evenly_sampled check rejects it first (CoordError). Remove the dead branch instead of excluding it from coverage; test_nat_time still verifies NaT is rejected via the earlier guard. --- dascore/io/prodml/utils.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dascore/io/prodml/utils.py b/dascore/io/prodml/utils.py index a07ade919..c1cbf442a 100644 --- a/dascore/io/prodml/utils.py +++ b/dascore/io/prodml/utils.py @@ -275,10 +275,8 @@ def _round_times_to_microseconds(coord): if not np.issubdtype(values.dtype, np.datetime64) or len(values) < 2: msg = "ProdML writing requires at least two absolute time samples." raise PatchError(msg) - # Defensive: a NaT breaks even-sampling, so _get_single_patch's - # require_evenly_sampled check rejects such coords before they reach here. - if np.any(np.isnat(values)): # pragma: no cover - raise PatchError("ProdML time coordinates cannot contain NaT.") + # A NaT could not survive _get_single_patch's require_evenly_sampled check + # (uneven spacing), so it never reaches here and needs no explicit guard. microsecond_values = values.astype("datetime64[us]") if np.array_equal(microsecond_values.astype(values.dtype), values): microseconds = microsecond_values.astype(np.int64) From 3823cbdfd5e4c39b18f2b30edb7de392622f3b8d Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 12:26:05 +0200 Subject: [PATCH 4/5] Make the walk-skip test deterministic (CodeRabbit) Replace the global Path.is_dir patch and mid-test file deletion with a mocked _iter_filesystem that yields a real file plus a never-created "vanished" path. The vanished path's real stat() raises FileNotFoundError, exercising the concurrent-deletion guard without depending on is_dir/stat call ordering or filesystem timing. --- tests/test_io/test_indexer.py | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/tests/test_io/test_indexer.py b/tests/test_io/test_indexer.py index 9ef551370..6bf09165d 100644 --- a/tests/test_io/test_indexer.py +++ b/tests/test_io/test_indexer.py @@ -193,32 +193,29 @@ class TestWalkResilience: def test_walk_skips_file_removed_mid_scan(self, tmp_path, monkeypatch): """A file vanishing between the walk and its stat is skipped, not fatal.""" - (tmp_path / "good.h5").write_bytes(b"") - (tmp_path / "vanisher.h5").write_bytes(b"") + from dascore.io.index import indexer as indexer_mod + + good = tmp_path / "good.h5" + good.write_bytes(b"") + # Never created: models a file deleted between the walk yielding it and + # _walk's stat() call, so its real stat() raises FileNotFoundError. + vanished = tmp_path / "vanished.h5" indexer = DBDirectoryIndexer(tmp_path) - # Keep the walk off the format-detection path; this test targets the - # stat guard, not directory-format probing of the root directory. - monkeypatch.setattr(indexer, "_directory_format", lambda path: False) - - real_is_dir = Path.is_dir - - def is_dir_then_vanish(self, *args, **kwargs): - # Delete the file immediately after the is_dir() probe so the walk's - # subsequent real stat() hits the concurrent-deletion guard. This - # reproduces the race without assuming how is_dir() is implemented. - result = real_is_dir(self, *args, **kwargs) - if self.name == "vanisher.h5": - self.unlink() - return result - - monkeypatch.setattr(Path, "is_dir", is_dir_then_vanish) + + def fake_iter(*args, **kwargs): + # Deterministically feed _walk both candidates, independent of the + # real filesystem, so the stat guard is exercised on the vanished one. + yield good + yield vanished + + monkeypatch.setattr(indexer_mod, "_iter_filesystem", fake_iter) try: walked = indexer._walk() finally: indexer.close() names = {Path(entry[-1]).name for entry in walked.values()} assert "good.h5" in names - assert "vanisher.h5" not in names + assert "vanished.h5" not in names class TestBasics: From 40247596f116e99add8fa23300de9ce6c36530a5 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 13:13:21 +0200 Subject: [PATCH 5/5] Trim verbose codecov comment Drop the multi-line rationale block to a single line per review. --- codecov.yml | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/codecov.yml b/codecov.yml index c8b2df65b..a425a208a 100644 --- a/codecov.yml +++ b/codecov.yml @@ -5,12 +5,7 @@ codecov: comment: after_n_builds: 7 -# Require full coverage. `target: 100%` makes the gate absolute rather than -# relative to the base commit, so a regression cannot slip through when a base -# upload is partial (see the coverage drop introduced by #759). The gate is -# scoped to the `unittests` flag (the deterministic non-network + doctest -# suite, which covers every line): it never depends on the carried-forward -# `network` upload, so a skipped or stale network report cannot mask a drop. +# Require 100% coverage on the unittests flag. coverage: status: project: