TST: cover the paths excluded by coverage pragmas - #785
Conversation
Seven "pragma: no cover" markers hid code from the coverage report rather than from the test suite's reach. All seven are now tested: - The remote HDF5 writer: committing on a clean context exit, reopening an existing remote file (which downloads it first), item assignment and membership, and cleaning up the temp file when the local open fails. Its five markers sat on def lines, so they excluded whole methods whose other lines were already exercised. - get_format re-raising RemoteCacheError instead of treating a failed fetch as a wrong-format signal. - The chunk-plan guard against a source disjoint from its output. The partitioner splits on gaps so this cannot arise through the public API, but _build_members can be handed such a pair directly, which pins the behavior the guard exists for. The suite is now 100% covered without any pragma in these files.
📝 WalkthroughWalkthroughChangesRemote I/O validation
Chunk mapping validation
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25b1e6eb32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| outputs = pd.DataFrame({"output_id": [0], "time_min": [3.0], "time_max": [6.0]}) | ||
|
|
||
| members = _build_members(sub, outputs, "time") |
There was a problem hiding this comment.
This test directly calls private _build_members with a disjoint source/output state that its own docstring acknowledges public partitioning cannot produce, so it couples coverage to an implementation detail without validating supported chunk-planning behavior. Exercise an equivalent boundary through build_chunk_plan, or retain the coverage exclusion if this is intentionally unreachable defensive code; .agents/agents.md specifically requires tests to focus on boundaries rather than implementation details. .agents/agents.mdL66-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and reverted in 005d3cd. Reaching that guard meant handing _build_members a source/output pair the partitioner cannot produce, which is an implementation detail rather than a boundary.
I did check whether it is reachable publicly first: sources within a partition are continuous (partitioning splits on gaps) and start-corrected, so the source searchsorted selects always overlaps the output. That matches the comment already on the guard, so it stays as documented defensive code with its exclusion.
The other six exclusions in this PR are gone, each replaced by a test of real behavior: the remote HDF5 writer's commit, append-downloads-existing, item access and temp-file cleanup paths, and get_format re-raising RemoteCacheError instead of treating a failed fetch as a wrong-format signal.
There was a problem hiding this comment.
Follow-up: the guard is now an assertion (523884d), which resolves this without either an artificial test or an exclusion.
assert lo <= hi, f"source {src_num} does not overlap output {out_num}"The statement runs on every iteration, so it needs no pragma, and it states the invariant rather than hiding it. Skipping was the wrong response anyway: silently dropping a source loses data, where failing is at least visible. It holds across the full suite (8229 passed).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #785 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17589 17619 +30
=========================================
+ Hits 17589 17619 +30
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:
|
Reverts the test which reached that guard by calling _build_members directly. It could only get there by handing the function a state the partitioner cannot produce, which tests an implementation detail rather than a boundary, contrary to .agents/agents.md. Sources within a partition are continuous and start-corrected, so no public call can offer a source disjoint from its output; the guard stays as documented defensive code with its exclusion. The other six exclusions in this PR are covered by tests of real behavior and remain removed.
The guard covered a state the partitioner cannot produce: sources within a partition are continuous and start-corrected, so searchsorted never offers one which does not overlap its output. Skipping was the wrong response to that anyway, since dropping a source silently loses data. Asserting says what the invariant is, fails loudly if it ever breaks, and needs no coverage exclusion because the statement runs every iteration. It holds across the whole suite.
d-chambers
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@dascore/utils/chunk_plan.py`:
- Around line 428-433: Replace the assert in the chunk-plan source/output
overlap handling with an explicit runtime check that raises the appropriate
failure when lo > hi, preventing an invalid member row from being appended under
python -O. Add a regression test covering this non-overlapping interval with
optimization enabled.
🪄 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: dae5b05c-c2e6-4040-a989-792ce0452bdd
📒 Files selected for processing (1)
dascore/utils/chunk_plan.py
| # Sources within a partition are continuous (partitioning | ||
| # splits on gaps) and start-corrected, so searchsorted never | ||
| # offers a source which does not overlap the output. Assert it | ||
| # rather than skipping: silently dropping a source would lose | ||
| # data, and the state cannot be reached from the public API. | ||
| assert lo <= hi, f"source {src_num} does not overlap output {out_num}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python -O - <<'PY'
def guard(lo, hi):
assert lo <= hi
return "invalid interval accepted"
assert guard(2, 1) == "invalid interval accepted"
PYRepository: DASDAE/dascore
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)dascore/utils/chunk_plan\.py$' || true
echo "== relevant source =="
sed -n '400,445p' dascore/utils/chunk_plan.py
echo "== python -O assert semantics demonstration =="
tmpf="$(mktemp)"
cat > "$tmpf" <<'PY'
def guard(lo, hi):
assert lo <= hi
return "invalid interval accepted"
print(guard(2, 1))
PY
echo "without -O:"
python3 "$tmpf" || true
echo "with -O:"
python3 -O "$tmpf"
rm "$tmpf"
echo "== inspect pytest/optimize-related config =="
git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|tox\.ini|noxfile\.py|pytest\.ini|Makefile)$' || true
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|tox\.ini|noxfile\.py|pytest\.ini|Makefile)$'); do
echo "--- $f ---"
rg -n "opti|python.O|pytest|assert|noassert|assertions" "$f" || true
doneRepository: DASDAE/dascore
Length of output: 2990
Use an explicit runtime check instead of assert.
assert statements are skipped under python -O, so a non-overlapping lo > hi interval would fall through and append an invalid member row instead of failing.
Proposed fix
- assert lo <= hi, f"source {src_num} does not overlap output {out_num}"
+ if lo > hi:
+ raise AssertionError(
+ f"source {src_num} does not overlap output {out_num}"
+ )Add a regression test for this path with optimization enabled.
📝 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.
| # Sources within a partition are continuous (partitioning | |
| # splits on gaps) and start-corrected, so searchsorted never | |
| # offers a source which does not overlap the output. Assert it | |
| # rather than skipping: silently dropping a source would lose | |
| # data, and the state cannot be reached from the public API. | |
| assert lo <= hi, f"source {src_num} does not overlap output {out_num}" | |
| # Sources within a partition are continuous (partitioning | |
| # splits on gaps) and start-corrected, so searchsorted never | |
| # offers a source which does not overlap the output. Assert it | |
| # rather than skipping: silently dropping a source would lose | |
| # data, and the state cannot be reached from the public API. | |
| if lo > hi: | |
| raise AssertionError( | |
| f"source {src_num} does not overlap output {out_num}" | |
| ) |
🤖 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 `@dascore/utils/chunk_plan.py` around lines 428 - 433, Replace the assert in
the chunk-plan source/output overlap handling with an explicit runtime check
that raises the appropriate failure when lo > hi, preventing an invalid member
row from being appended under python -O. Add a regression test covering this
non-overlapping interval with optimization enabled.
There was a problem hiding this comment.
Keeping the assert, deliberately.
The -O behaviour is real, but this is an internal invariant of the partitioner rather than validation of anything a caller supplies: sources within a partition are continuous (partitioning splits on gaps) and start-corrected, so searchsorted cannot offer a source disjoint from its output. That is the case asserts exist for, and it holds across the full suite (8229 tests).
An explicit raise would need its own coverage exclusion, since the raising line still never executes — which is what this PR set out to remove. A test could only reach it by handing _build_members a state the public API cannot produce, and @codex rightly pushed back on exactly that earlier in this PR (see the _build_members thread), since .agents/agents.md asks for boundary tests rather than implementation-detail tests.
One thing your comment did surface: dropping the old continue means that under -O an impossible state would append an invalid row instead of being skipped. Worth stating plainly rather than leaving implied — though it trades a check on an unreachable state for a clearer statement of the invariant, and skipping silently would have lost a source's data anyway.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
1 participant
Description
Removes every
# pragma: no coverfrom these files: six are replaced by tests, the last by an assertion.The markers were hiding code from the coverage report, not from the test suite's reach. That has a cost: a pragma on dead code means coverage cannot tell you it is dead (one such function,
_get_remote_cache_dir, is deleted in #781 after exactly that happened), and a pragma on adefline excludes the whole method even when most of it is exercised.What is now tested
The remote HDF5 writer (
_RemoteH5Writer, five markers). All five sat ondeflines, so they excluded entire methods, though coverage showed most of__init__and__exit__were already being run. New tests cover what was genuinely unreached:h5pyopen fails.get_formatre-raisingRemoteCacheError. The loop over FiberIOs swallows exceptions so a reader which does not recognize a file can be skipped; a failed remote fetch has to escape that handler rather than be reported as an unknown format. The test monkeypatchesIOResourceManager.get_resourceso it does not depend on which plugins happen to be installed.The chunk-plan boundary guard becomes an assertion. This one is genuinely unreachable through the public API — sources within a partition are continuous (partitioning splits on gaps) and start-corrected, so the source
searchsortedselects always overlaps its output. A test could only reach it by handing_build_membersa state the partitioner cannot produce, which is an implementation detail rather than a boundary (thanks @codex for pushing back on the first attempt).assert lo <= hiis the better shape for that: it states the invariant, runs on every iteration so it needs no exclusion, and fails loudly if the invariant ever breaks. Skipping was the wrong response in any case — dropping a source silently loses data, where an inverted interval would at least be visible. The assertion holds across the whole suite.Result
No
# pragma: no coverremains indascore/utils/hdf5.py,dascore/utils/chunk_plan.pyordascore/io/core.py, and every line in them is covered. The three markers indascore/utils/io.pyanddascore/utils/remote_io.pyare removed by #781, after which the package has none.Validation
examples.pythat CI covers through the generated doc-code tests.pre-commit run --all: passed.Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
in/item access.Tests