test(webhooknotify): end-to-end coverage for the sweep→River→SMTP path - #862
Merged
Conversation
webhooknotify had no end-to-end test, unlike hitlnotify. Each seam was covered separately -- internal/identity proves the sweep enqueues inside its transaction, worker_test proves the worker delivers -- but nothing joined them, and the seam BETWEEN them is precisely where all four of this feature's merge-blocking defects lived. Three tests, mirroring hitlnotify's TestEndToEnd_AcceptTxThroughRiverToSMTP with a real River client and a real notifier against a fake SMTP: - disable path: seeded terminal failures -> sweep flips the row AND enqueues in one tx -> River -> worker -> SMTP. Asserts recipient, subject, that the captured reason reaches the body, that a configured reply_to survives the whole path, and that the row really transitioned (enabled=false, auto_disabled_at, auto_disable_reason) -- the state change and the email are two halves of one transaction, so checking only the mail would miss half of it. - warn path: pending rows with one failed attempt each. Asserts the BREAKER does not fire on that evidence (the point of warning on attempts rather than terminal rows), that the warning does, that it uses warning copy rather than disabled copy, and that the webhook stays enabled. - no-transition: one short of both thresholds sends nothing. Guards against a future change that enqueues per candidate row rather than per state transition, which would mail a customer every five minutes for as long as their endpoint stayed broken. Two things learned writing it, both encoded in the test: - testutil's SMTP drain closure CLOSES the listener, so it is one-shot "stop and collect". Polling it kills the server before the worker connects. These poll river_job instead and drain once. - testutil.TestDB hands out a SHARED database, so an unscoped "count completed jobs" sees a sibling test's job and returns before this test's own mail is sent. The poll filters on args->>'webhook_id'. On timeout the failure prints the job's state, attempt and errors, so a break says WHICH seam failed rather than just "no email arrived". Verified non-vacuous: with the sweep's enqueue silently no-op'd the disable test fails with "(no webhook_notify job was ever enqueued)". Ran -count=3 for flakiness; repo-wide `go test -short` at 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
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.
Why
webhooknotifyhad no end-to-end test, unlikehitlnotify. Each seam was covered separately —internal/identityproves the sweep enqueues inside its transaction,worker_testproves the worker delivers — but nothing joined them.That gap matters: the seam between them is precisely where all four of this feature's merge-blocking defects lived.
What
Three tests using a real River client and a real notifier against a fake SMTP, mirroring
hitlnotify'sTestEndToEnd_AcceptTxThroughRiverToSMTP:Disable path — seeded terminal failures → sweep flips the row and enqueues in one tx → River → worker → SMTP. Asserts recipient, subject, that the captured reason reaches the body, that a configured
reply_tosurvives the whole path, and that the row really transitioned (enabled=false,auto_disabled_at,auto_disable_reason). The state change and the email are two halves of one transaction, so checking only the mail would miss half of it.Warn path — pending rows with one failed attempt each. Asserts the breaker does not fire on that evidence (the entire point of warning on attempts rather than terminal rows), that the warning does, that it uses warning copy rather than disabled copy, and that the webhook stays enabled.
No-transition — one short of both thresholds sends nothing. Guards against a future change that enqueues per candidate row rather than per state transition, which would mail a customer every five minutes for as long as their endpoint stayed broken.
Two traps, both encoded in the test
testutil's SMTP drain closure callslistener.Close()— it's one-shot "stop and collect", not a repeatable drain. Polling it kills the server before the worker ever connects. These pollriver_joband drain once.testutil.TestDBhands out a shared database, so an unscoped "count completed jobs" sees a sibling test's job and returns before this test's own mail is sent. The poll filters onargs->>'webhook_id'.On timeout the failure prints the job's state, attempt and errors, so a break reports which seam failed rather than just "no email arrived".
Verification
Non-vacuous: with the sweep's enqueue silently no-op'd, the disable test fails with exactly the right diagnosis:
Ran
-count=3for flakiness (these involve real timing); repo-widego test -shortat 0 failures.Test-only change — no product code touched.
🤖 Generated with Claude Code