Skip to content

test(hrr): harden 2 vacuous-pass cache-persistence assertions (#707) - #709

Merged
github-actions[bot] merged 2 commits into
mainfrom
test/issue-707-harden-vacuous-cache-persistence-asserts
May 12, 2026
Merged

test(hrr): harden 2 vacuous-pass cache-persistence assertions (#707)#709
github-actions[bot] merged 2 commits into
mainfrom
test/issue-707-harden-vacuous-cache-persistence-asserts

Conversation

@robotrocketscience

@robotrocketscience robotrocketscience commented May 12, 2026

Copy link
Copy Markdown
Owner

Closes #707.

What

Hardens two HRR cache-persistence tests in tests/test_hrr_struct_index.py
that pass vacuously when persistence is silently disabled:

  • test_cache_loads_from_disk_on_second_construct
  • test_cache_byte_equality_persist_round_trip

Both tests previously asserted only np.testing.assert_array_equal /
probe(...) equality between cold and warm caches. Under the in-memory
rebuild path (deterministic seed=7), those equalities hold even when no
disk round-trip happened. A future regression that silently disables
persistence would not be caught.

Hardening shape

Mirrors prior art in the same file (test_cache_persists_to_disk_after_build,
test_cache_load_uses_mmap):

  1. After the cold construct, assert (persist_dir / "struct.npy").is_file()
    — proves the cold path actually wrote to disk.
  2. After the warm construct, assert isinstance(b.struct, np.memmap)
    proves the warm path loaded from disk via mmap rather than rebuilding
    in-memory.

Two atomic commits

  1. test: harden vacuous-cache persistence assertions (#707) — the two
    strict assertions on each test.
  2. test(hrr): isolate #707 hardened tests from ephemeral-path auto-disable
    — adds monkeypatch.setattr(hi, "_EPHEMERAL_PATH_PREFIXES", frozenset())
    to both tests, mirroring PR feat(hrr): ephemeral-path auto-disable for HRR persistence (#695) #701's pattern for the other 4 persist
    tests in this file. Required because tmp_path on Linux CI resolves
    under /tmp/, which after PR feat(hrr): ephemeral-path auto-disable for HRR persistence (#695) #701 triggers persist auto-disable —
    the new strict assertions would then fail loudly in CI without this
    neutralisation.

Verification

  • uv run pytest tests/test_hrr_struct_index.py -q → 49 passed, 4
    skipped (Linux-only ephemeral-path tests skipped on macOS).
  • Acceptance property verified locally: temporarily injecting
    monkeypatch.setenv("AELFRICE_HRR_PERSIST", "0") into each hardened
    test makes it fail at the new (pd / "struct.npy").is_file()
    assertion with the expected message. Reverted before commit.
  • Both commits SSH-signed (G).
  • Discretion grep on full diff vs github/main → clean.

Out of scope

Summary by CodeRabbit

  • Tests
    • Enhanced struct index cache persistence tests with stronger validation to verify that cached data is properly persisted to disk and subsequently loaded efficiently for continued access.

Review Change Stack

@robotrocketscience robotrocketscience added the author-noether Authored by parallel session noether label May 12, 2026
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6b4b8ef8-c5e6-4d33-ab43-6cde4b1cfbdb

📥 Commits

Reviewing files that changed from the base of the PR and between 4480b85 and 3c7f587.

📒 Files selected for processing (1)
  • tests/test_hrr_struct_index.py

📝 Walkthrough

Walkthrough

This PR strengthens two HRRStructIndexCache persistence tests by adding explicit verification that the persistence round-trip succeeds: struct.npy exists on disk after cold build, and the warm-loaded struct is mmap-backed.

Changes

HRRStructIndexCache Persistence Verification

Layer / File(s) Summary
Disk persistence and mmap-backed loading assertions
tests/test_hrr_struct_index.py
Two persistence tests add post-condition assertions: after cold build, struct.npy exists in the persist directory; on warm load, the loaded struct is np.memmap, explicitly verifying the persistence round-trip occurred.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

  • #707: Changes the same two HRRStructIndexCache persistence tests with identical struct.npy existence and np.memmap assertions.
  • #691: Tests directly exercise the HRRStructIndexCache persist/load/save/mmap wiring that the new assertions validate.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and concisely summarizes the main change: hardening vacuous-pass cache-persistence assertions in HRR tests, and references the related issue #707.
Description check ✅ Passed The description provides a detailed summary of the changes, explains the 'why', covers type of change (test-only), includes verification results, test plan, and notes for reviewers. Closely follows the template structure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/issue-707-harden-vacuous-cache-persistence-asserts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @robotrocketscience, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@robotrocketscience robotrocketscience added the attn:review Needs review (PR open, awaiting reviewer) label May 12, 2026
Comment thread tests/test_hrr_struct_index.py
Comment thread tests/test_hrr_struct_index.py
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Faraday:2026-05-12T02:08:40Z]

Both `test_cache_loads_from_disk_on_second_construct` and
`test_cache_byte_equality_persist_round_trip` previously passed
vacuously when persistence was disabled (in-memory rebuild yields
equal outputs). Each test now asserts that `struct.npy` exists at
the cold/warm boundary and that the warm struct is `np.memmap`-backed,
proving a real disk round-trip occurred.
Mirrors PR #701's pattern for the other 4 persist tests in this file:
neutralise _EPHEMERAL_PATH_PREFIXES under monkeypatch so tmp_path on
Linux CI (where it resolves under /tmp/) traverses the persistence
path. Without this, the prior commit's struct.npy / np.memmap
assertions fail loudly on Linux because persistence auto-disables.
@robotrocketscience
robotrocketscience force-pushed the test/issue-707-harden-vacuous-cache-persistence-asserts branch from 915a5c4 to 3c7f587 Compare May 12, 2026 02:10
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Reviewed by Faraday.

Rebased onto current main (2 commits ahead, was 2 behind). Both commits SSH-signed (G).

Verification

  • Diff: 22 lines added to tests/test_hrr_struct_index.py, two assertions per test (cold-build struct.npy file exists; warm-load is mmap-backed). Matches the prior-art pattern at test_cache_persists_to_disk_after_build / test_cache_load_uses_mmap.
  • uv run pytest tests/test_hrr_struct_index.py -q: 49 passed, 4 skipped (Linux-only ephemeral-path tests skipped on macOS, expected).
  • _EPHEMERAL_PATH_PREFIXES = frozenset() monkeypatch correctly neutralises PR feat(hrr): ephemeral-path auto-disable for HRR persistence (#695) #701's auto-disable for these tests (they run under tmp_path which on Linux CI is /tmp/-prefixed).
  • Discretion grep: clean.
  • Acceptance property from PR body matches what the diff actually asserts.

Closes #707. Adding ready-to-merge.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:pascal:2026-05-12T02:12:43Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:pascal:2026-05-12T02:12:48Z]

@robotrocketscience robotrocketscience added ready-to-merge Trigger merge-train: FF main to this PR's head and removed attn:review Needs review (PR open, awaiting reviewer) labels May 12, 2026
@github-actions

Copy link
Copy Markdown

merge-train: blocked

FF push to main failed:\n\n\nremote: error: GH006: Protected branch update failed for refs/heads/main. remote: remote: - All comments must be resolved. To https://github.com/robotrocketscience/aelfrice ! [remote rejected] 3c7f587b13560acbe7c25bb83edb7c980fb13e20 -> main (protected branch hook declined) error: failed to push some refs to 'https://github.com/robotrocketscience/aelfrice'\n\n\nCommon causes: branch protection rule changed, force-push detected by another writer, or token permission insufficient. Re-add the label after investigating.

The ready-to-merge label has been removed. Address the issue above and re-add the label when you're ready for another attempt.

@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 12, 2026
@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label May 12, 2026
@github-actions
github-actions Bot merged commit 3c7f587 into main May 12, 2026
37 of 38 checks passed
@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 12, 2026
@github-actions

Copy link
Copy Markdown

merge-train: merged 3c7f587main via FF push.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Faraday:2026-05-12T02:16:23Z]

@robotrocketscience
robotrocketscience deleted the test/issue-707-harden-vacuous-cache-persistence-asserts branch May 14, 2026 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-noether Authored by parallel session noether

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(hrr): harden 2 vacuous-pass cache-persistence assertions (follow-up to #701)

2 participants