-
Notifications
You must be signed in to change notification settings - Fork 281
Update e2e tests for new flag #6275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
41d0b97 to
2885300
Compare
The `signIn` function is only available on the client side, so pressing the button before the app has hydrated will lead to an error.
| }, | ||
| ) { | ||
| await page.goto(`${getBaseTestEnvUrl()}/`); | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The US-specific landing page had a menu on mobile, the hitherto-ROW version doesn't.
| const hasRenderedClientSide = useHasRenderedClientSide(); | ||
|
|
||
| if (typeof session.data?.user.email === "string") { | ||
| if (typeof session.data?.user.email === "string" || !hasRenderedClientSide) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a change in behaviour, but when the page loads, client-side JS needs to execute to activate React and attach the signIn event handler. Clicking the button beforehand triggered the Next-Auth error page, which happened often locally.
(Another "fix" was for Playwright to wait before networkidle to click the Sign In button, but that is discouraged by the Playwright docs, and actual users can run into this error case as well if they click the button too fast. Thus, I'm now just hiding the button until it's functional.)
| import { useL10n } from "../../hooks/l10n"; | ||
| import { Button, ButtonProps } from "./Button"; | ||
| import { useTelemetry } from "../../hooks/useTelemetry"; | ||
| import { useHasRenderedClientSide } from "../../hooks/__mocks__/useHasRenderedClientSide"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment(blocking): I think this import is incorrect (shouldn't use __mocks__)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 I was already surprised by how fast the rendering happened during my testing. I remember when I first started out as a programmer I was annoyed at our Java codebase that needs tens of imports everywhere, and thus couldn't really do without auto-imports, but also where I had no way to tell whether the auto-imported modules actually were the right ones. And now this is my life 🫠
Long story short, fixed in 5013721.
This will fail against prod until the flag is flipped there as well. The PR runs also fail (and so will local runs), because the flag is off by default.
Edit: ah right, I can align the e2e test flags with stage. Updated the tests to remove the subscription tests and updating some selectors, but they seem flaky running locally, so I just pushed it and am hoping that it succeeds reliably in CI now, for PRs at least.
Edit 2: ah alright, figured out the flakiness issues. Will add some inline comments to clarify. (Netlify is failing because of the Cloudflare outage.)