Skip to content

465d5198 - test(scrypt): cover the cooldown interval formula and the since hand-off - #4455

Merged
TaprootFreak merged 5 commits into
developfrom
test/scrypt-uncertain-lookup-coverage
Jul 29, 2026
Merged

465d5198 - test(scrypt): cover the cooldown interval formula and the since hand-off#4455
TaprootFreak merged 5 commits into
developfrom
test/scrypt-uncertain-lookup-coverage

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #4438. Two properties of that PR's behaviour were provably untested — both gaps were found by mutating the production code and observing that the suite stayed green.

The gaps, and how they were measured

Mutation Before this PR With this PR
ageMs / 10 and both clamps replaced by the flat UNCERTAIN_RESOLVE_MIN_INTERVAL_MS 47/47 green red
Math.min(..., UNCERTAIN_RESOLVE_MAX_INTERVAL_MS) cap removed green red
rate halved: ageMs / 10ageMs / 5 green red
cap lowered: 30 minutes → 10 minutes green red
boundary loosened: <<= green red
getOrderStatus ignores its since argument and fetches the fixed 30-day window 81/81 green red

Every existing cooldown test creates its order with created: new Date(), so the interval always lands on the one-minute floor — neither the age formula nor the 30-minute cap was ever exercised. And both since tests live in scrypt.adapter.spec.ts, where getOrderStatus is mocked away: they prove the adapter computes and passes the bound, not that it reaches the venue request.

What is added

liquidity-management-pipeline.service.spec.ts, inside the existing venue-lookup cooldown block. Both tests bracket their threshold from both sides — a one-sided assertion would miss any mutation that enlarges the interval, since the order simply stays in cooldown and a "still waiting" assertion keeps passing:

  • the rate: for a 100-minute-old order the wait is satisfied once elapsed >= (100 min + elapsed) / 10, i.e. at 11 min 6.7 s — so one call at 11 minutes, two at 11 min 20 s
  • the cap: an 8-hour-old order's uncapped wait would be 48 minutes at the first pass and 51 by the boundary check, so one call one millisecond before 30 minutes and two at exactly 30 minutes. Checking a single millisecond early leaves the cap no other whole-millisecond value to take, and landing on the boundary rather than past it pins < against <=

scrypt.service.spec.ts, next to the existing getOrderStatus tests:

  • a caller-supplied since reaches connection.fetch as StartDate
  • omitting it still yields the 30-day window, pinned from both sides since the bound derives from "now"

No production code is touched, no existing test is modified, and both files use the helpers and fake-timer setup already present.

Verification

format:check, lint and type-check clean; the affected suites pass in band; each of the six mutations above turns at least one of the new tests red, and the control run returns to green.

Both properties were provably untested. Replacing `ageMs / 10` and the
30-minute cap with the flat one-minute floor left the suite green, and so
did making getOrderStatus ignore its `since` argument and fetch the fixed
30-day window again — every existing cooldown test uses a freshly created
order, where the interval is always the floor, and the adapter tests mock
getOrderStatus away entirely.

- pipeline service: a 100-minute-old order still owes its 10-minute wait
  after 9 minutes, and an 8-hour-old one is asked again after 31 because
  the cap holds it at 30 rather than the uncapped 48
- scrypt service: a caller-supplied `since` reaches the venue request as
  StartDate, and omitting it still yields the 30-day window
Review follow-up. Both cooldown tests asserted only the lower side of the
interval, so a mutation that ENLARGES it stayed green: with `ageMs / 5`
the order simply remains in cooldown and a "still waiting" assertion keeps
passing, and the cap test is dominated by the cap whatever the divisor.

Each test now brackets its exact threshold. For a 100-minute-old order the
wait is satisfied once elapsed >= (100 min + elapsed) / 10, i.e. at
11 min 6.7 s — hence one call at 11 minutes and two at 11 min 20 s. The cap
is bracketed at 29 min 59 s and 30 min 1 s, which also pins the boundary
against a cap set too low.
The cap test bracketed 29 min 59 s and 30 min 1 s but never the boundary
between them, which is the only place `<` and `<=` differ: at exactly
1_800_000 ms elapsed the production check admits the lookup, a `<=` variant
would hold it back, and neither assertion point could tell them apart.
Moving the second check onto 30 minutes exactly pins that too.
Checking at 29 min 59 s left the cap a whole second of room: any value
between 1_799_001 and 1_799_999 ms produced the same two observations as
the real 1_800_000, so a cap set slightly too low passed unnoticed.
Checking one millisecond before the boundary closes that gap entirely.

Also corrects the comment's arithmetic: 48 minutes is the uncapped wait at
the first pass; by the boundary check the order has aged another 30
minutes and its uncapped wait is 51. Both are far past the cap, so the
conclusion held, but the number did not.
The comment claimed the bracket leaves the cap "no room to sit anywhere
else". That holds for every whole millisecond, which is every value the
constant can actually take, but the test alone does not rule out a
fractional one — so say whole-millisecond and mean it. Also fixes the
grammar of the preceding clause.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Five review passes were needed to reach zero findings, across two dimensions each (conformance and logic). What changed along the way:

  1. Both cooldown tests only pinned one side of the interval. A mutation that enlarges it — ageMs / 10ageMs / 5 — left them green, because the order simply stays in cooldown and a "still waiting" assertion keeps passing. Rebuilt as three-pass tests that bracket the threshold from both sides.
  2. The < / <= boundary was untested. The bracket ran 29 min 59 s / 30 min 1 s and never touched the boundary itself, which is the only place the two differ.
  3. A comment claimed 48 minutes at the boundary check, where the order has aged another 30 minutes and its uncapped wait is 51. The conclusion held, the number did not.
  4. Checking at 29 min 59 s left the cap a full second of slack — any value between 1_799_001 and 1_800_000 ms produced identical observations. Now checked one millisecond before the boundary, which leaves no other whole-millisecond value.
  5. The PR description drifted behind the code twice and was brought back in line each time.

Verification on the final commit: format:check, lint, type-check clean, 132/132 tests passing in band, and nine mutants each turning at least one of the new tests red — ageMs / 5, ageMs / 20, cap to 10 min, cap to 60 min, floor to 1 ms, <<=, the formula removed entirely, and getOrderStatus ignoring its since argument. The control run returns to green.

One note for whoever picks this up next: neither touched file is anywhere near the coverage ratchet's bar. Measured on this branch, scrypt.service.ts sits at 50.59 % statements / 36.78 % branches and liquidity-management-pipeline.service.ts at 71.63 % / 81.25 %, so neither can be pinned. Worth remembering that line coverage would not have caught either of the gaps this PR closes — both mutated lines were executed by the existing tests all along; only their results went unchecked.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 29, 2026 16:18
@TaprootFreak
TaprootFreak requested a review from Danswar July 29, 2026 16:25
@TaprootFreak
TaprootFreak merged commit 52468f3 into develop Jul 29, 2026
12 checks passed
@TaprootFreak
TaprootFreak deleted the test/scrypt-uncertain-lookup-coverage branch July 29, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant