test(e2e): implement welcome OTP email scenario (email-delivery)#91
Conversation
Implements atproto-78w: the first of three scenarios in
features/email-delivery.feature. Adds two new cucumber step definitions:
- Given 'a mail trap is capturing outbound emails' — Mailpit health
check against /api/v1/info; fails the background step fast if the
mail trap is unreachable, so scenario failures point at the trap
rather than at the subsequent OTP-arrival step.
- Then 'the email body contains a numeric OTP code' — asserts the OTP
extracted by waitForEmail/extractOtp is all digits.
Reworks the feature file to match the existing 'unique test email' /
'returning user' fixture pattern used by passwordless-authentication.
Literal emails (alice@example.com, bob@example.com) required either a
DB-reset hook (not yet plumbed through to e2e runner) or a seeded test
env — neither pragmatic. The unique-email pattern is hermetic and
matches how the active scenarios already run.
The Feature stays @pending for now: scenarios 2 and 3 still reference
step definitions that don't exist yet (atproto-8mx, atproto-a21). The
@pending tag is removed by the cleanup bead atproto-c7i once all three
scenarios are implemented and passing in CI.
Dry-run confirms scenario 1 maps all 6 steps with 0 undefined.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds Mailpit readiness check and OTP validation steps to Cucumber tests; updates feature scenarios to use Mailpit, abstracts hard-coded emails, and changes OTP expectation to follow a configurable charset. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
🚅 Deployed to the ePDS-pr-91 environment in ePDS
|
Coverage Report for CI Build 24663700868Coverage remained the same at 36.52%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/email.steps.ts`:
- Around line 10-23: The Mailpit health check can hang; wrap the fetch call in
an explicit timeout using AbortSignal.timeout to fail fast: create an
AbortSignal via AbortSignal.timeout(timeoutMs) (e.g., 5_000) and pass it as the
signal option to fetch inside the Given step (the async function in
email.steps.ts that checks testEnv.mailpitPass), keep the existing headers
(mailpitAuthHeader()) and ensure the signal is aborted/used so fetch throws on
timeout; handle the thrown error the same way (so the existing res.ok check
stays) and propagate a clear error if the probe times out or fails.
- Around line 78-86: The step Then('the email body contains a numeric OTP code',
implemented as the anonymous step function in email.steps.ts, should use the
configured charset instead of hard-coding digits and must not log the actual OTP
value; update the validation to read testEnv.otpCharset (available from
e2e/support/env.ts) and choose the correct pattern (e.g., numeric → /^\d+$/,
alphanumeric → /^[A-Za-z0-9]+$/) to test this. Keep the existing guard that
throws if this.otpCode is missing, but change the validation error to a
non-sensitive message like "OTP does not match configured charset" (no token
included). Optionally, if you prefer runtime detection, implement the same
charset detection used by buildIncorrectOtpCode in auth.steps.ts and skip
validation when detection is ambiguous.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cf39ec6-eda8-4228-9791-c4fad120b275
📒 Files selected for processing (2)
e2e/step-definitions/email.steps.tsfeatures/email-delivery.feature
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -1,27 +1,26 @@ | |||
| @email @pending | |||
There was a problem hiding this comment.
🔴 The @pending tag is placed on the Feature itself (line 1), which Gherkin propagates to all three Scenarios — including Scenario 1, which this PR claims is fully implemented. Because e2e/cucumber.mjs excludes all @pending scenarios via not @pending, CI will produce 0 runs for this feature, making the PR test plan item "E2E CI run against Railway PR env executes scenario 1 successfully" impossible to satisfy. Fix: move @pending from the Feature to only Scenarios 2 and 3.
Extended reasoning...
What the bug is and how it manifests
The file features/email-delivery.feature opens with @email @pending on line 1, decorating the Feature: keyword itself. In Gherkin, any tag applied to a Feature node is automatically inherited by every Scenario and Scenario Outline within that feature. This means all three scenarios — including Scenario 1 ("New user receives a welcome OTP email"), which the PR describes as fully implemented — carry the @pending tag.
The specific code path that triggers it
e2e/cucumber.mjs line 8 adds 'not @pending' to tagExclusions, and line 17 joins those into the tag expression used by both pnpm test:e2e and pnpm test:e2e:headless. The CI workflow .github/workflows/e2e-tests.yml line 609 runs pnpm test:e2e:headless, which uses that config. The resulting tag filter causes the cucumber runner to skip every scenario tagged @pending, which is every scenario in this feature.
Why existing code doesn't prevent it
The individual step definitions are correctly implemented and the dry-run check in the test plan (pnpm test:e2e --dry-run --name "New user receives a welcome OTP email") passes because --name overrides tag filtering. However, the actual CI invocation uses tag filtering without a name filter, so the tag exclusion wins and all three scenarios are skipped.
What the impact would be
The PR's stated goal — having CI verify Scenario 1 — is impossible as written. The CI job will report 0 scenarios executed for this feature, not a passing scenario. The reviewer checkpoint "E2E CI run against Railway PR env executes scenario 1 successfully (verified by reviewer after CI completes)" cannot be satisfied, leaving the PR's testing claim unverifiable.
Step-by-step proof
features/email-delivery.featureline 1:@email @pending→ Feature inherits@pending- Gherkin propagation: all 3 Scenarios inherit
@pendingfrom the Feature e2e/cucumber.mjsline 8:tagExclusions = [..., 'not @pending']- Line 17 joins these into the final tag expression passed to cucumber
.github/workflows/e2e-tests.ymlline 609: CI runspnpm test:e2e:headless- Cucumber evaluates tag expression → Scenario 1 matches
@pending→ excluded - Result: 0 scenario runs for the entire feature in CI
How to fix it
Remove @pending from the Feature and add it only to Scenarios 2 and 3:
@email
Feature: Email delivery
...
Scenario: New user receives a welcome OTP email
...
@pending
Scenario: Returning user receives a sign-in OTP email
...
@pending
Scenario: Backup email verification link is delivered
...This allows Scenario 1 to run in CI while keeping Scenarios 2 and 3 pending until their step definitions are implemented.
| ) | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🟡 The Mailpit health check error message reports at ${testEnv.mailpitUrl} but the actual endpoint fetched is ${testEnv.mailpitUrl}/api/v1/info, omitting the path. This makes the error less actionable when debugging failures. Fix by appending /api/v1/info to the URL in the error message.
Extended reasoning...
What the bug is and how it manifests
In e2e/step-definitions/email.steps.ts lines 14–22, the Given step fetches ${testEnv.mailpitUrl}/api/v1/info to verify Mailpit is reachable. When the response is not OK, the thrown error message says Mailpit health check failed: ${res.status} at ${testEnv.mailpitUrl}, which omits the /api/v1/info path portion of the URL.
The specific code path that triggers it
The discrepancy is on line 14 vs line 20:
- Fetch target:
${testEnv.mailpitUrl}/api/v1/info - Error message:
at ${testEnv.mailpitUrl}(missing path)
This means whenever the health check fails (Mailpit down, wrong credentials, network issue), the developer sees only the base URL with no indication of which endpoint was probed.
Why existing code doesn't prevent it
There is no lint rule or convention enforcement to catch this mismatch. The established pattern in common.steps.ts (e.g. PDS health check failed: ${res.status} at ${testEnv.pdsUrl}/health) correctly includes the full path, but this new step deviates from that pattern.
What the impact would be
Purely a debugging inconvenience in test/e2e code. The health check itself is functionally correct — if Mailpit is reachable, the step passes; if not, it throws. The only consequence is that a developer seeing the failure message cannot immediately tell which specific endpoint was queried without reading the source code.
How to fix it
Change line 20 from:
`Mailpit health check failed: ${res.status} at ${testEnv.mailpitUrl}`to:
`Mailpit health check failed: ${res.status} at ${testEnv.mailpitUrl}/api/v1/info`Step-by-step proof
- Mailpit is unreachable (e.g. wrong
MAILPIT_URLin env). fetch(${testEnv.mailpitUrl}/api/v1/info)returns a non-OK status (e.g. 404 or connection error).- The
if (\!res.ok)branch fires. - The thrown error reads:
Mailpit health check failed: 404 at http://localhost:8025. - Developer sees the base URL only — they cannot tell from the message alone that the
/api/v1/infoendpoint was the one that failed, requiring them to read the source to understand what was checked.
- Move @pending from Feature to scenarios 2 & 3. The Feature-level tag was propagating to scenario 1, so cucumber.mjs's 'not @pending' exclusion skipped scenario 1 entirely — making the Railway CI "pass" meaningless. Scenario 1 now runs; scenarios 2 & 3 stay pending until atproto-8mx / atproto-a21. - Add AbortSignal.timeout(5_000) to the Mailpit health probe so DNS or connectivity stalls fail fast instead of hanging CI; include the full /api/v1/info path in the error message. - Rename 'the email body contains a numeric OTP code' to '... an OTP code matching the configured charset' and read the regex from testEnv.otpCharset so the step works whether OTP_CHARSET is numeric (default) or alphanumeric. Drop the extracted OTP from the error message — a mismatched charset shouldn't leak the token. Addresses: claude-bot (@pending propagation, error URL path), CodeRabbit (timeout, charset detection, OTP leakage). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implement the two remaining scenarios in email-delivery.feature: * Returning user receives a sign-in OTP email * Backup email verification link is delivered Remove @pending tags so both now run in CI. Add supporting step defs: * "the user requests an OTP for the test email" (reuses world.testEmail set by the returning-user Given; clears mailpit to avoid racing with the prior welcome OTP). * "the user adds a unique backup email" (drives the /account/backup-email/add form on the account settings page). * "a verification email arrives in the mail trap for the backup email" and "the email contains a verification link". Add fetchEmailBody helper to mailpit support. Add backupEmail and lastEmailBody fields to EpdsWorld. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
1 participant
Summary
features/email-delivery.feature(welcome OTP email for a new user)Given 'a mail trap is capturing outbound emails'— Mailpit health check against/api/v1/infoThen 'the email body contains a numeric OTP code'— asserts the extracted OTP is all digitsunique test email/returning userfixture pattern (hermetic, matchespasswordless-authentication.feature). Literal emails (alice@example.com,bob@example.com) required either a DB-reset hook not yet plumbed to the e2e runner or a seeded test env — neither pragmatic.The feature stays
@pendingbecause scenarios 2 and 3 reference steps that don't exist yet (tracked by atproto-8mx and atproto-a21). The@pendingtag is removed by the cleanup bead atproto-c7i once all three are implemented and passing.Supersedes PR #63 (closed; force-pushed branch).
Test plan
pnpm format:checkcleanpnpm lintcleanpnpm typecheckcleanpnpm test— 442 unit tests passpnpm test:coverage— no regressionpnpm test:e2e --dry-run --name "New user receives a welcome OTP email"— 6 steps, 0 undefined🤖 Generated with Claude Code
Summary by CodeRabbit