test: cover sync-progress helpers and heartbeat.sh output - #170
Open
OlufemiAdeOlusile wants to merge 4 commits into
Open
test: cover sync-progress helpers and heartbeat.sh output#170OlufemiAdeOlusile wants to merge 4 commits into
OlufemiAdeOlusile wants to merge 4 commits into
Conversation
None of the code added for CI heartbeat sync progress had any automated test coverage. Only manual review and live dispatched runs verified it, and the repo's own automatic PR gate (unit_tests.yaml, pytest framework_tests) would not have caught any of the real bugs found in review: the crash on a corrupt status file, the heartbeat line disappearing entirely on a null percent, or the jq empty-string fallback. Adds framework_tests/test_sync_progress.py covering: - write_json_to_file: correct content, atomic (no leftover temp file), creates parent dirs. - update_json_file: creates a missing file, preserves unrelated keys, overwrites its own key, recovers from an empty/truncated/ non-dict existing file instead of raising, still raises on a genuine write failure (permission denied) since that's the caller's decision, not this helper's. - node.write_progress_file: correct content, workdir=None is a no-op, a write failure is swallowed (never aborts a sync run), does not clobber the other component's key. - db_sync._log_sync_progress: node and dbsync keys are proven to come from independent sources via separate monkeypatched functions, not from each other; the dbsync key is skipped before db-sync has produced any data; a node-tip failure does not stop the dbsync key from being written. - heartbeat.sh, run as a real subprocess for one tick: renders the percent line, falls back to "syncProgress unavailable" with era/epoch/slot when the percent is null, shows "?" for an empty era, prints nothing for a key not yet present in the file, and renders both node and dbsync lines independently when both exist.
An independent review ran actual mutation testing against these tests, not just a read of the diff, and found seven real gaps where a broken implementation would still pass. Critical: - The atomic write, the whole point of this PR, had zero coverage. Reverting write_json_to_file to its old non-atomic form still passed all 23 tests. Added a test that monkeypatches json.dump to fail partway through and asserts the original file survives - fails on the non-atomic version, passes on this one. - _require_jq was autouse at module scope, so a missing jq silently skipped all 23 tests, not just the 5 that need it, with exit code 0 and no signal anything was wrong. Scoped it to the heartbeat tests only via @pytest.mark.usefixtures, and switched from shelling out to `which` (itself not guaranteed present) to shutil.which. Important: - Two tests simulated a permission failure with chmod. Root bypasses that check entirely, so on a CI runner running as root one test would pass for the wrong reason (no exception ever raised) and the other would fail for an environment reason, not a code reason. Both now use monkeypatch instead, which is deterministic and uid-independent. - The independence test only checked 3 of 6 fields per key, missing block and sync_time_h_m_s - exactly the kind of field a copy-paste mirroring bug would get wrong. Now asserts full dict equality, matching the node-side test above it. - The skip-before-db-sync-starts test guarded its only assertion behind `if progress_file.exists()`, which was always true, but meant a regression that wrote nothing at all would still pass. Removed the guard and added a positive assertion that the node write did happen. - All five heartbeat assertions stopped before "(as of ...)", so a frozen or wrong timestamp - the main real-world symptom of a dead writer - was never checked. Extended every assertion to the full line. - _newest_file's mtime comparison was never exercised with an actual choice between files; every test wrote exactly one. Added a test with two files at different explicit mtimes. Also added a note to the independence test's docstring: it proves node and dbsync are populated from separate function calls, not that their real-world values converge or diverge for any particular reason - that was checked separately, against two real data sources on a live run. Verified each fix by reproducing the reviewer's exact failure mode and confirming it now fails without the fix: reverted the atomic write and watched the new test catch it, simulated jq both absent (clean, isolated skip) and present-but-broken (loud failure, not a silent pass), in both cases confirming the other tests in the file are unaffected either way.
It only triggers on pull_request to main. A branch stacked on an unmerged PR (like this one) never gets a base of main, so its own tests never actually run on CI until the whole stack merges. Adds workflow_dispatch so it can be fired manually against any branch in the meantime.
It runs pytest against framework_tests/, not a directory or concept called "unit tests". The old name oversold what it actually checks. Rename the file, the workflow name, the job id, and the step name to match what it runs.
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
1 participant
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.
Stacked on #168.
None of #168's code had test coverage. Adds
framework_tests/test_sync_progress.py: 23 tests for the JSONstatus-file helpers,
write_progress_file,_log_sync_progress'sprogress writes, and
heartbeat.sh(run as a real subprocess).Also fixes 7 gaps a mutation-testing review found in the first
version of these tests, most notably: the atomic-write test didn't
actually test atomicity, and a jq-availability fixture would have
silently skipped all 23 tests instead of just the 5 that need it.
All tests pass locally (
nix develop .#python). Not yet verified onGitHub's own CI - see PR comments.