Skip to content

fix(test): stop awaiting the bad-credentials login through expect().rejects - #75

Merged
sebyx07 merged 1 commit into
mainfrom
fix/auth-e2e-rejects-timeout
Sep 8, 2026
Merged

sebyx07 merged 1 commit into
mainfrom
fix/auth-e2e-rejects-timeout

Conversation

@sebyx07

@sebyx07 sebyx07 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

a persona whose credentials are wrong fails the run instead of opening it signed out has been red on main since 2026-09-02 (runs 33969855127, 33647731476), timing out at exactly 30,000ms. This is the failure that was already red when #74 merged.

The cause is the assertion, not the behaviour

Awaiting a long, multi-round-trip browser flow through the .rejects matcher starves the loop driving it: every CDP call inside the pending promise then costs ~1s. This login types 20 characters and presses 6 keys, so it could never finish inside the test's 30s ceiling.

Isolated with an A/B in the same process, same test file — same performLogin, same adapter, same page, same config, same profile dir, same log sink. Only the await differs:

how the flow is awaited time
await performLogin(...) in try/catch 1.4s
await expect(performLogin(...)).rejects.toThrow(...) 31.5s

Everything else was ruled out first by elimination: browser binary (system Chrome vs Playwright's), the log sink (real FindingsStore vs no-op — it receives 6 lines total), the profile dir, launch options (byte-identical), single vs double navigation, and launch order.

Why the previous fix wasn't enough

login.ts's NAVIGATION_SETTLE_MS comment already chased this exact timeout and fixed a genuine contributor — a settle wait that could run the full 30s budget, now capped at 5s. That fix is real and stays. It just wasn't the whole cost: instrumented here, the settle wait measures 3ms while the typing measured 32s.

The fix

Catch the rejection and assert on the caught error, taking the flow out of the matcher. 30s timeout → 1.84s, well inside the existing ceiling — so no timeout needed raising, which is the outcome worth having: the test is fast because the work is fast, not because it was given more room.

.rejects is left alone everywhere else in the suite. Every other use wraps a call that fails on one round trip or none, where a single stall costs nothing. The rule is about how much work is in flight, not about the matcher — the comment in the test says so, so the next person doesn't "tidy" it back.

Verified

  • the previously failing test: 1.84s, passing
  • bun test src/services — 127/127
  • bun run lint, bun run typecheck — clean

The 29 failures in browser-adapter.integration.test.ts are pre-existing and environmental (they need the dummy/web fixture CI builds, which isn't built locally); they fail identically on stock main, before this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_015KQipy9s9tJFkGBeGWnZCX


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Tests
    • Updated browser coverage for invalid credentials to verify that authentication failures are reported as the expected error type.

…ejects

"a persona whose credentials are wrong fails the run instead of opening it
signed out" has been failing on `main` since 2026-09-02 (runs 33969855127,
33647731476), timing out at exactly 30,000ms.

The cause is the assertion, not the behaviour it asserts. Awaiting a long,
multi-round-trip browser flow THROUGH the `.rejects` matcher starves the loop
driving it, so every CDP call inside the pending promise costs ~1s. The login
types 20 characters and presses 6 keys, so it never finished inside the test's
30s ceiling.

Measured, same `performLogin` / adapter / page, only the await differing:

  awaited directly       1.4s
  awaited via .rejects  31.5s

`login.ts`'s `NAVIGATION_SETTLE_MS` note already chased this timeout once and
fixed a real contributor (a 30s idle wait, now 5s). It was not the whole cost —
the settle wait measures 3ms here, while the typing measured 32s.

Catching the rejection with try/catch and asserting on the caught error takes
the flow out of the matcher: 30s timeout -> 1.84s, well inside the existing
ceiling, so no timeout needed raising.

Left `.rejects` alone everywhere else in the suite: every other use wraps a call
that fails on one round trip or none, where a single stall costs nothing. The
rule is about how much work is in flight, not about the matcher.

Verified: `bun test src/services` 127/127, lint clean, typecheck clean. The 29
failures in browser-adapter.integration.test.ts are pre-existing and
environmental (the dummy/web fixture CI builds is not built locally) — they fail
identically on stock main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015KQipy9s9tJFkGBeGWnZCX
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Essentials

Run ID: f8b1f21a-58f2-430c-8470-92d570c14628

📥 Commits

Reviewing files that changed from the base of the PR and between fd35055 and 20cdd16.

📒 Files selected for processing (1)
  • src/services/session-builder.test.ts

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: bun (lint + typecheck + test)
🧰 Additional context used
📓 Path-based instructions (4)
Never expose authentication secrets to model context or logs.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • src/services/session-builder.test.ts
Use strict TypeScript and never use `any`.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • src/services/session-builder.test.ts
Keep MCP handlers thin and place business logic in services.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • src/services/session-builder.test.ts
Maintain fast unit tests, with the documented test suite completing in under 10 seconds, and test session isolation and other correctness invariants.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • src/services/session-builder.test.ts
🔇 Additional comments (1)
src/services/session-builder.test.ts (1)

553-573: LGTM!


📝 Walkthrough

Walkthrough

The authentication failure test now awaits the browser login flow directly, captures the thrown error, and verifies that it is an AuthError.

Changes

Authentication Test

Layer / File(s) Summary
Capture authentication errors
src/services/session-builder.test.ts
The test replaces .rejects.toThrow(AuthError) with try/catch and an explicit AuthError assertion.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 20cdd

The bad-credentials browser test now captures and verifies the authentication error directly, avoiding the prior timeout while preserving the expected failure behavior. The change is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: replacing Jest's .rejects handling for the bad-credentials test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/auth-e2e-rejects-timeout

A rabbit awaits the login gate
The error hops out, clear and straight
AuthError stands in view
The test knows what to do
And the browser rests its feet

Comment @coderabbitai help to get the list of available commands.

@sebyx07
sebyx07 merged commit c5c488b into main Sep 8, 2026
2 checks passed
@sebyx07
sebyx07 deleted the fix/auth-e2e-rejects-timeout branch September 8, 2026 17:35
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