test(ads): cover the house rotation rate in serveAd - #186
Merged
Merged
Conversation
#183 de-flaked tests/contract/ads-short-code-serving.test.ts by mocking HOUSE_AD_ROTATION_RATE to 0 for that file, which was the right call — the tests there are about the click URL of a paid fill, and the coin flip made roughly one CI run in three go red on whichever test lost it. That commit notes rotation is still covered by ads-house-rotation.test.ts. It isn't, quite: that file calls houseFill() directly and never goes through serveAd, so it pins what a house ad looks like, not when one is served. With the rate pinned to 0 in the only file that reached it, the branch at lib/ads/serve.ts:213 has no test left. That branch is what keeps the network advertising itself on slots that are already selling, and it is one comparison — the kind of thing that can be deleted or inverted in a refactor and show up as a revenue question months later, not a red build. So it is pinned here against serveAd, by driving the draw to each side of the threshold rather than sampling a ~10% frequency. Sampling would mean reintroducing exactly the coin flip that made the other file flaky; a controlled draw tests the same behaviour and cannot fail intermittently. Verified by mutation: '<' to '<=' fails 1 test, inverting the comparison fails 3, deleting the branch fails 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan52 finding(s) HIGH/CRITICAL: 10 | MEDIUM: 42
…and 2 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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 #183. Not a fix — it restores one piece of coverage that PR removed.
What happened
#183 de-flaked
tests/contract/ads-short-code-serving.test.tsby mockingHOUSE_AD_ROTATION_RATEto0for that file. That was the right call: those tests are about the click URL of a paid fill, and the ~10% house-ad coin flip made roughly one CI run in three go red on whichever test lost it. (That's what took down #179 and #183 itself.)That commit notes rotation is "covered by tests/contract/ads-house-rotation.test.ts, so nothing is lost." It isn't, quite — that file calls
houseFill()directly and never goes throughserveAd:So it pins what a house ad looks like, not when one is served. With the rate pinned to
0in the only file that reached it,lib/ads/serve.ts:213has no test left:That branch is what keeps the network advertising itself on slots that are already selling. It's a single comparison — the kind of thing that gets dropped or inverted in a refactor and surfaces as a revenue question months later rather than a red build.
Approach
Pinned against
serveAdby driving the draw to each side of the threshold, not by sampling a frequency. Sampling a ~10% rate over N fills would mean reintroducing the exact coin flip that made the other file flaky; a controlled draw tests the same behaviour and cannot fail intermittently.Four tests: draw under the rate → house fill; house fill writes no impression (unmetered — a house ad that metered would bill a publisher for our own promotion); draw exactly on the rate → still paid (the comparison is
<); a high draw → paid, which catches an inverted branch that the boundary tests alone would not.Nothing in
ads-short-code-serving.test.tsis touched, so this cannot conflict with #183 or reintroduce its flake.Verification
Mutation-tested — each mutation applied to
lib/ads/serve.ts, suite re-run, source restored:<→<=<→>(inverted)Full suite on this branch: 1299 passed, 7 skipped.
tsc --noEmitclean. The new file runs in ~200ms.🤖 Generated with Claude Code