fix(android): repositoryInsertV18Aux_insertsThenPrunes has been red on main since #888 - #897
Closed
vishk23 wants to merge 1 commit into
Closed
fix(android): repositoryInsertV18Aux_insertsThenPrunes has been red on main since #888#897vishk23 wants to merge 1 commit into
vishk23 wants to merge 1 commit into
Conversation
…ft twin got #888 amortised the v18AuxSample retention sweep on both platforms, but only the Swift side got the seam its tests need. `WhoopStore.insert` gained an internal `v18AuxPruneEveryRows:` parameter and its retention tests pass 1, so they still prove newest-N-rows without banking 10,000 rows first. `WhoopRepository.insert` read the 10,000 constant directly, and `DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes` was left asserting a sweep that amortisation now defers. So that test has been RED on main since #888 landed. It is not visible in CI: the Android workflow builds and tests on push/PR to this repo's own branches, and a Kotlin unit-test regression on a merge does not gate anything. Verified on a clean checkout of upstream main at 2ff2af1 with nothing else applied: `7 tests completed, 1 failed`. Mirrors the Swift seam exactly rather than inventing a second one: `insert` takes `v18AuxRetentionRows` and `v18AuxPruneEveryRows` as defaulted parameters, so every production caller is unchanged and only the tests pass anything. The failing test now passes `v18AuxPruneEveryRows = 1` for the same reason the Swift ones do, and proves what it always meant to — that a written batch is followed by the rolling prune, carrying the shipped retention constant. Adds the four amortisation cases as Kotlin twins of the Swift ones, since the Kotlin path had the behaviour but no coverage of it: deferred below the threshold, swept once crossed (across batches, so the counter's accumulation is what is being pinned), the counter resetting so a long offload sweeps repeatedly, and the budget not being shared between devices — that last one fails against a shared counter, which is the invariant #888's per-device map exists for. The shared Proxy-DAO recorder replaces the per-test one so the five cases read as variations on one setup rather than five copies of it. Verification: ./gradlew testFullDebugUnitTest — 3193 tests, 0 failures, 5 skipped (DeepCaptureMigrationTest: 11 passed). Same command on main before this change: 1 failed.
Author
|
Superseded by #904, which landed the same fix upstream — the injectable prune-threshold seam is now on |
ryanbr
added a commit
that referenced
this pull request
Jul 28, 2026
…ct (#914) The #103 probe's no-answer verdict printed one sentence — "neither GET_FF_VALUE(128) nor GET_DEVICE_CONFIG_VALUE(121) is served by this firmware" — for three runs that establish three different things, and for one that establishes the opposite. Two timeouts got it. But `BLEManager.send` can return without transmitting at all: there is an early return when `cmdCharacteristic` is missing, and another in the 5/MG allowlist else-branch. So a run in which nothing ever reached the strap rendered a claim about what the strap's firmware serves. The file's own header names answered / rejected as UNSUPPORTED / silent as three distinct outcomes and then the verdict collapsed silence into the same factual claim as a refusal. One refusal got it for both verbs. The `||` meant 128 answering UNSUPPORTED(3) while 121 timed out printed "neither ... is served — rejected as UNSUPPORTED". 121 was never refused; it was never heard from. An undecodable reply got the bare "is not served" sentence. A CRC or envelope failure is affirmative evidence the strap DID transmit, which is the opposite of what that sentence says. The sentence is now per-verb and tied to the evidence that produced it: UNSUPPORTED is "refused by firmware (UNSUPPORTED)", a timeout is "served no reply in Ns — unconfirmed" (the window is now recorded, not just traced), an undecodable reply is "replied but the frame did not decode — unconfirmed", and an untried verb is "not asked". Only UNSUPPORTED is the firmware answering, so the strong "not served by this firmware" wording survives in exactly one case: the firmware refused BOTH verbs itself, where the `||` is now an `&&`. Same defect class as the ECG probe fix on #896: a verdict asserting a conclusion the run's own inputs could not support. Mirrored byte-for-byte in com.noop.protocol.DeviceConfigReadProbe; the golden cross-platform report test is unaffected because it exercises the answered path. swift test 428 passed, 0 failures; gradlew testFullDebugUnitTest 3199 tests, 0 failures, 5 skipped, verified from the JUnit XML (the DeepCaptureMigrationTest failure noted in #897 is fixed on current main and passes here). Refs #103. Co-authored-by: Fanboynz <mp3geek@gmail.com>
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.
DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPruneshas been red onmainsince #888 landed.Verified on a clean checkout of
mainat2ff2af18with nothing else applied:I found it while running
testFullDebugUnitTestas the local gate for an unrelated branch, and traced it back rather than assuming it was mine.Why it broke, and why nothing caught it
#888 amortised the
v18AuxSampleretention sweep on both platforms. On the Swift side it also added the seam the tests need:WhoopStore.insertgained an internalv18AuxPruneEveryRows:parameter, and the existing retention tests pass1so they still prove newest-N-rows without banking 10,000 rows first. That is exactly what the PR body describes under TESTS.The Kotlin twin got the behaviour but not the seam.
WhoopRepository.insertreadsV18_AUX_PRUNE_EVERY_ROWS(10,000) directly, so the existing test — which inserts one row and then assertspruneV18Auxwas called — now asserts a sweep that amortisation correctly defers.It stays invisible because the Android workflow gates pushes and PRs against this repo's own branches; a Kotlin unit-test regression arriving on a merge does not fail anything. The four new amortisation tests #888 added are Swift-only, so the Kotlin path has had the behaviour with no coverage of it since.
The change
Mirrors the Swift seam rather than inventing a second one.
WhoopRepository.inserttakesv18AuxRetentionRowsandv18AuxPruneEveryRowsas defaulted parameters, so every production caller is unchanged and only tests pass anything.The failing test now passes
v18AuxPruneEveryRows = 1for the same reason the Swift ones do, and proves what it always meant to: a batch that wrote rows is followed by the rolling prune, carrying the shipped retention constant.Adds the four amortisation cases as Kotlin twins of the Swift ones:
A shared
Proxy-DAO recorder replaces the per-test one so the five cases read as variations on one setup instead of five copies of it.No production behaviour changes: same constants, same sweep, same best-effort
runCatchingwith theCancellationExceptionrethrow.Verification
./gradlew testFullDebugUnitTeston this branch: 3193 tests, 0 failures, 5 skipped —DeepCaptureMigrationTest: 11 passedmainimmediately before: 1 failed