test(tools): collect test_whoop_spot_hrv too — 8 more tests that never ran - #815
Merged
Conversation
#814 fixed unittest collection for test_whoop_activity.py and missed the identical instance sitting in the same directory, in a file that PR moved: test_whoop_spot_hrv.py is also pytest-style — 8 bare `def test_*`, no unittest.TestCase — so `python3 -m unittest` collected nothing from it and exited 0. The file's own docstring told people to run it with pytest, which is not in requirements.txt and is not what the README documents, so in practice these eight tests ran for nobody. All eight take no fixtures, so the same load_tests hook is enough; all pass. Docstring corrected to name both runners. Suite: 159 -> 167 tests, OK (1 skipped). A repo-wide sweep now finds no Python test file that unittest silently skips. Found by asking whether #814's bug class existed anywhere else rather than assuming one instance was the whole problem — it was not, and the second instance was in a file that PR had already touched.
ryanbr
added a commit
that referenced
this pull request
Jul 26, 2026
…hat described two wrongly (#816) Closes the coverage gap #815 recorded rather than fixed, and corrects a docstring found while doing it. MUTATION MATRIX (__pycache__ cleared between every run) drop min_dist spacing filter survived -> killed drop min_prom prominence gate survived -> killed min_prom > becomes >= killed (new test) neighbours symmetric >= killed (new test) neighbours symmetric > killed (new test) invert local-maximum test killed (2 tests) -> killed (4) 5/5, up from 1/3. Suite 167 -> 170, OK (1 skipped). WHY THE EXISTING TEST COULD NOT COVER THESE test_find_peaks_counts_beats asserts a beat count on synthetic PPG whose beats are already well separated and well above the noise floor. Both filters are no-ops on that input, so it passes whether they exist or not. The new tests use minimal hand-built arrays and each asserts BOTH directions, so none can pass vacuously: min_dist [0,5,0,9,0] maxima two apart: min_dist=3 keeps only the taller [3], min_dist=2 keeps both [1,3] — pinning the documented >= contract min_prom [0,2,0,9,0]: min_prom=5 rejects the 2.0 maximum, min_prom=1 keeps it boundary [0,5,0] at min_prom=5.0 -> [] (strict >), at 4.9 -> [1] plateau [0,5,5,0] and [0,5,5,5,0] -> [1] regardless of plateau length THE DOCSTRING WAS WRONG ON TWO COUNTS said: "Local maxima >= neighbours and >= min_prom" code: v[i] > v[i-1] and v[i] >= v[i+1] and v[i] > min_prom min_prom is a strict >, so a maximum exactly on the threshold is rejected. And the neighbour test is asymmetric, not ">= neighbours". The code is right on both and the docstring is what changed. The asymmetry is the standard way to take one index from a plateau: symmetric >= emits a duplicate per plateau sample and inflates the beat count, symmetric > drops plateaus entirely. Verified empirically rather than asserted — [0,5,5,0] gives [1,2], [] and [1] under the three variants respectively. That docstring had been wrong for as long as it existed and no test could have caught it, because nothing exercised either boundary. Same shape as the rest of today's tooling work: the defect was not a failing test, it was the absence of one. Verified under both runner shapes — discover (170) and the explicit module list the README documents (53) — from a cleared bytecode cache, after a stale .pyc earlier reported failures from a mutant whose source had already been restored. Tests and one docstring. No behaviour change, no schema, no scores, no strings.
2 tasks
ryanbr
pushed a commit
that referenced
this pull request
Jul 26, 2026
…#815) (#818) DataRange.pagesBehind read the write/read/ring-capacity triplet two bytes early — cmdOff+10/14/22 instead of +12/16/24 — on both platforms. The word grid starts at payload offset 3, not 1. Verified independently against the four real captures in the fixtures, decoded both ways: capture new +12/16/24 old +10/14/22 W U T W T #791 4.0 67491 67408 131072 128122881 0 #815 5/MG 100790 100788 131072 2310406145 0 #815 5/MG 100837 100829 131072 2313486337 0 #815 5/MG 112968 112474 131072 3108503553 0 Ring capacity is 2^17 on every frame across both families, the pointers are plausible page counters, and pagesBehind comes out 83 / 2 / 8 / 494 exactly. Under the old offsets capacity reads 0 and the pointers are in the billions. So this was worse than "RE'd but unconfirmed": because T always landed on padding, the `t > 0` plausibility guard rejected 100% of real frames and the feature never fired once on hardware. The doc comment said "not yet confirmed against real captures" and that read as "probably fine" to everyone who passed it. Renaming the synthetic-only whoop5CmdOff10 test is the same lesson — a fixture named as though it were hardware-validated is how the wrong offsets survived. Kotlin twin is byte-for-byte: same payloadOffset + 11/15/23, same four fixtures, same expected values. It cannot run in CI (android.yml is disabled, so no Kotlin test executes there) — the PR's "should pass under android.yml" will not happen automatically. All 9 Swift checks green including test (WhoopProtocol). Class-of-bug sweep came back clean: BodyLocationProbe and ExtendedBatteryProbe also take cmdOff + 1 as the payload start, which is correct, and both dump raw payload hex rather than indexing a word grid — so the i*4+1 assumption had no sibling. Follow-up to come: the nil path is silent. `if let pages = … { log }` means a rejected decode logs nothing, which is exactly why this went unnoticed — the strap log looked identical to a strap that never answered. The raw-frame dump one line above is logged unconditionally for precisely that reason; pagesBehind should match it. Diagnostic only: never gates sync or backfill. Wraparound branch still unverified (no capture has crossed it), left as-is.
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.
Follow-up to #814, which fixed unittest collection for
test_whoop_activity.pyand missed the identical instance in the same directory — in a file that PR had already moved.test_whoop_spot_hrv.pyis also pytest-style: 8 baredef test_*, nounittest.TestCase. Sopython3 -m unittestcollected nothing from it and exited 0, which looks exactly like passing. Its own docstring told readers to run it withpytest— which isn't inrequirements.txtand isn't what the README documents — so in practice these eight tests ran for nobody.All eight take no fixtures, so the same
load_testshook is enough. All pass. Docstring corrected to name both runners.A repo-wide sweep now finds no Python test file that unittest silently skips.
I found this by asking whether #814's bug class existed anywhere else instead of assuming one instance was the whole problem. It wasn't — and the second instance was in a file #814 had touched, which is the part I should have caught before merging.
Docs and tests only. No app code, no schema, no scores, no strings.