Restore 100% coverage and enforce it in codecov - #767
Merged
Conversation
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe pull request adds 100% unittest coverage gates and edge-case coverage for indexer updates, PRODML handling, coordinate indexing, remote-cache warnings, and warning suppression. CI and indexer resilience
PRODML edge cases
Coordinate and utility edge cases
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)codecov.ymlTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_io/test_indexer.py`:
- Around line 205-214: The test’s global Path.is_dir patch can delete
vanisher.h5 before _walk reaches its stat() error handler. Update the test
around is_dir_then_vanish to mock the iterator used by _iter_local_filesystem so
it deterministically yields both candidate paths, then make the vanished path’s
stat() raise the expected OSError and assert that _walk handles it while
preserving the normal candidate behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cadf1244-f756-4db0-b0d5-e1cfd728e74d
📒 Files selected for processing (2)
codecov.ymltests/test_io/test_indexer.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #767 +/- ##
============================================
+ Coverage 48.75% 100.00% +51.24%
============================================
Files 163 163
Lines 17081 17221 +140
============================================
+ Hits 8327 17221 +8894
+ Misses 8754 0 -8754
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_utils/test_misc.py`:
- Around line 578-582: Update test_message_filter_applies_action to pass an
explicit stacklevel argument to warnings.warn, using the appropriate positive
level while preserving the existing UserWarning and message matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c411970f-ad73-4911-a291-4d306393e89b
📒 Files selected for processing (7)
codecov.ymldascore/io/prodml/utils.pytests/test_io/test_index/test_planned.pytests/test_io/test_prodml/test_prod_ml.pytests/test_io/test_prodml/test_prodml_write.pytests/test_utils/test_io_utils.pytests/test_utils/test_misc.py
🚧 Files skipped from review as they are similar to previous changes (1)
- codecov.yml
Comment on lines
+578
to
+582
| 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) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an explicit stacklevel to satisfy Ruff B028.
- warnings.warn("boom", UserWarning)
+ warnings.warn("boom", UserWarning, stacklevel=2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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) | |
| 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, stacklevel=2) |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 582-582: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_utils/test_misc.py` around lines 578 - 582, Update
test_message_filter_applies_action to pass an explicit stacklevel argument to
warnings.warn, using the appropriate positive level while preserving the
existing UserWarning and message matching behavior.
Source: Linters/SAST tools
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.
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.
d-chambers
commented
Jul 23, 2026
Comment on lines
+9
to
+13
| # 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. |
Contributor
Author
There was a problem hiding this comment.
remove these lines; too verbose.
Contributor
Author
There was a problem hiding this comment.
Done in 4024759 — trimmed to a single line.
Drop the multi-line rationale block to a single line per review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
devcoverage slipped below 100% and CI never flagged it. Tracing Codecov history, coverage was a clean 100.0% through #758 and first read 99.96% at #759 ("Prune dead code and collapse single-implementation abstractions"), whose patch coverage was 89.29% — it added 6 uncovered defensive lines indascore/io/index/indexer.py.It slipped through because
codecov.ymlnever set an explicit coverage target. The 100% "gate" was only Codecov's defaultautostatus, which compares against the base commit rather than an absolute floor. Since the network-test split (#752) +after_n_builds: 7(#751) made per-commit uploads land as partial (unittests-only, ~48%), #759's head (99.96%) was compared against a broken ~48% base and read as an increase — so both project and patch checks passed.What
project+patchtarget: 100%(threshold 0%), scoped to theunittestsflag — the non-network + doctest suite, which now covers every line. The gate no longer depends on the carried-forwardnetworkupload, so a skipped or stale network report cannot mask a drop.indexer.py(the Prune dead code and collapse single-implementation abstractions #759 regression): atomic-swap temp-file cleanup, and the_walkguard that skips a file deleted mid-scan._coord_record_from_row: empty-string units normalize toNone.suppress_warnings: the message-filter branch._warn_remote_cache_download: metadata-scope guidance text._get_prodml_version_strreturns""for a non-ProdML file._round_times_to_microsecondsNaT guard is marked# pragma: no cover: a NaT breaks even sampling, so_get_single_patch'srequire_evenly_sampledcheck rejects such coords first (raisingCoordError, aPatchErrorsubclass), making the guard unreachable viadc.write.Verification
unittestsflag), coverage is 100.0% — 0 misses.codecov.ymlvalidated against Codecov's validator (Valid!).Network tests aren't run locally (no credentials); the
networkflag remains informational and no longer participates in the gate.Summary by CodeRabbit
Bug Fixes
Tests
Changelog
none