Skip to content

Cleaned up login page loading in E2E tests#26251

Merged
EvanHahn merged 1 commit intomainfrom
give-better-errors-when-login-fails-in-e2e-tests
Feb 5, 2026
Merged

Cleaned up login page loading in E2E tests#26251
EvanHahn merged 1 commit intomainfrom
give-better-errors-when-login-fails-in-e2e-tests

Conversation

@EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Feb 4, 2026

towards https://linear.app/ghost/issue/NY-1000

This test-only change should have no user impact.

This change:

  • Logs a more helpful error when visiting the signup page. This seems to be failing in CI a lot, and it'd be useful to know more about the error.

  • Removes an unnecessary waitFor on the email address field. locator.fill() automatically waits for this.

  • Stops an unnecessary check when going to the sign-in page. A user should already be available, and the extra wait will probably never happen--we visit a URL and then check that we're not at a different URL, which it will never be. (And if it did redirect, we wouldn't catch it this way.)

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

Walkthrough

This pull request updates navigation handling across multiple page helper classes in the e2e test suite. The BasePage class's goto method now explicitly returns a Playwright Response object (or null) instead of void, with corresponding signature updates to child classes SettingsPage and PublicPage. Additionally, the LoginPage class is refactored to replace a multi-attempt wait loop with a single navigation call followed by explicit response validation, and removes an unused private field while reducing initial synchronization overhead.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: cleaning up the login page loading in E2E tests, which aligns with the core modifications across multiple test helper files.
Description check ✅ Passed The description is directly related to the changeset, explaining the rationale for removing unnecessary waits and checks in the login flow, along with improved error handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch give-better-errors-when-login-fails-in-e2e-tests

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.

❤️ Share

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

@EvanHahn EvanHahn force-pushed the give-better-errors-when-login-fails-in-e2e-tests branch from bdb3dae to fe2a5af Compare February 4, 2026 23:07
towards https://linear.app/ghost/issue/NY-1000

*This test-only change should have no user impact.*

This change:

- Logs a more helpful error when visiting the signup page. This seems to
  be failing in CI a lot, and it'd be useful to know more about the
  error.

- Removes an unnecessary `waitFor` on the email address field.
  `locator.fill()` [automatically waits for this][0].

- Stops an unnecessary check when going to the sign-in page. A user
  should already be available, and the extra wait will probably never
  happen--we visit a URL and then check that we're not at a different
  URL, which it will never be. (And if it *did* redirect, we wouldn't
  catch it this way.)

[0]: https://playwright.dev/docs/actionability
@EvanHahn EvanHahn force-pushed the give-better-errors-when-login-fails-in-e2e-tests branch from fe2a5af to d5dcad5 Compare February 5, 2026 17:06
@EvanHahn EvanHahn marked this pull request as ready for review February 5, 2026 17:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@e2e/helpers/pages/public/public-page.ts`:
- Around line 80-93: In goto(), remove the redundant unconditional call to
enableAnalyticsRequests() so analytics are only enabled for the analytics
project; keep the call inside the if (testInfo.project.name === 'analytics')
block along with pageHitRequestPromise() and ensure the existing logic that
awaits pageHitPromise and returns result remains unchanged (references: goto,
enableAnalyticsRequests, pageHitRequestPromise, test.info()).

@EvanHahn EvanHahn merged commit b4eca23 into main Feb 5, 2026
30 checks passed
@EvanHahn EvanHahn deleted the give-better-errors-when-login-fails-in-e2e-tests branch February 5, 2026 19:22
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.

2 participants