Fixed flaky email verification trigger acceptance test#28006
Conversation
ref https://linear.app/ghost/issue/BER-3680 - the test asserted email verification triggers on the exact member that crosses the admin threshold; that assertion is racy because the trigger counts members_created_events rows written by a separate MemberCreatedEvent subscriber, with no ordering guarantee between the two handlers - when the trigger's count wins the race it undercounts by one and verification fires one member creation late, so the assertion failed intermittently on CI - added a buffer member past the threshold so verification has deterministically triggered by the time it is asserted, regardless of how the race lands - the underlying product off-by-one is low impact and tracked in BER-3680
WalkthroughThe "Email verification trigger" test in the members.test.js file is modified to address flakiness caused by an off-by-one race condition between independent webhook subscribers. The test now adds a recovery member after the initial threshold-crossing attempt, settles domain events, and then relaxes its webhook assertion from requiring an exact 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/test/e2e-api/admin/members.test.js`:
- Around line 1114-1117: The current assertion uses a too-broad check
(matchingRequest.body.amountTriggered >= 2); narrow it to only allow the two
documented outcomes (2 or 3) by replacing that condition with an explicit check
that matchingRequest.body.amountTriggered is either 2 or 3 (e.g., assert that
matchingRequest.body.amountTriggered equals 2 or equals 3) so overcount
regressions will fail; update the assertion around
matchingRequest.body.amountTriggered accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44c0dd1d-d228-46e2-8504-2cecedc77743
📒 Files selected for processing (1)
ghost/core/test/e2e-api/admin/members.test.js
| // amountTriggered is the member count observed when verification fired. | ||
| // It is past the threshold (1); the exact value (2 or 3) depends on the | ||
| // off-by-one race described above. | ||
| assert.ok(matchingRequest.body.amountTriggered >= 2, 'Expected the webhook to report a member count past the threshold'); |
There was a problem hiding this comment.
Tighten the relaxed amountTriggered assertion.
>= 2 is broader than the behavior this test documents, so an overcount regression would still pass. With one buffer member and the known race, the only expected values here should be 2 or 3.
🎯 Narrow the assertion to the two valid outcomes
- assert.ok(matchingRequest.body.amountTriggered >= 2, 'Expected the webhook to report a member count past the threshold');
+ assert.ok([2, 3].includes(matchingRequest.body.amountTriggered), 'Expected the webhook to report a member count of 2 or 3 when verification fired');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // amountTriggered is the member count observed when verification fired. | |
| // It is past the threshold (1); the exact value (2 or 3) depends on the | |
| // off-by-one race described above. | |
| assert.ok(matchingRequest.body.amountTriggered >= 2, 'Expected the webhook to report a member count past the threshold'); | |
| // amountTriggered is the member count observed when verification fired. | |
| // It is past the threshold (1); the exact value (2 or 3) depends on the | |
| // off-by-one race described above. | |
| assert.ok([2, 3].includes(matchingRequest.body.amountTriggered), 'Expected the webhook to report a member count of 2 or 3 when verification fired'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/test/e2e-api/admin/members.test.js` around lines 1114 - 1117, The
current assertion uses a too-broad check (matchingRequest.body.amountTriggered
>= 2); narrow it to only allow the two documented outcomes (2 or 3) by replacing
that condition with an explicit check that matchingRequest.body.amountTriggered
is either 2 or 3 (e.g., assert that matchingRequest.body.amountTriggered equals
2 or equals 3) so overcount regressions will fail; update the assertion around
matchingRequest.body.amountTriggered accordingly.
1 participant
What
Makes the acceptance test
members.test.js→ "Email verification trigger › Can add a member and trigger host email verification limits" deterministic. It has been failing intermittently on the mysql8 acceptance lane.Why it was flaky
The test asserted that email verification becomes required on the exact member that crosses the admin threshold. That assertion is racy:
VerificationTriggerdecides whether the threshold is exceeded by countingmembers_created_eventsrows, but that table is written by a separateMemberCreatedEventsubscriber (EventStorage). The two handlers run concurrently with no ordering guarantee — when the trigger's count query wins the race it misses the just-created member's row, undercounts by one, and fires one member creation late.What this PR does
Adds a buffer member past the threshold. By the time its event is handled, the earlier members' rows are guaranteed committed, so verification has deterministically triggered before it is asserted — regardless of how the race lands. The webhook assertion is relaxed accordingly (
amountTriggeredis now2or3). The buffer member uses a raw POST so the response-body snapshots are untouched.This is a test-only change. The underlying product off-by-one is low impact — it self-heals on the next member creation and has no user-facing effect — and is tracked separately in BER-3680. The test carries a
TODOto restore the precise assertion once that is fixed.