Skip to content
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

chore: add e2e for user login/logout flow #13185

Closed
wants to merge 22 commits into from
Closed

Conversation

Parkreiner
Copy link
Contributor

@Parkreiner Parkreiner commented May 6, 2024

Fully closes #13130 by filling in the gaps that #13137 and #13145 can't cover

Changes made

  • Added e2e helper for asserting that no uncaught runtime errors are available anywhere in the current UI
  • Wrote test for log in/log out flow
  • Updated some of the components to have better a11y hooks for Playwright's getByRole selectors
  • Added a couple more words to the cword dictionary

Notes

  • This was my first-ever Playwright test, so please let me know if there's anything wonky, or if I'm doing things that I don't actually need to do

@Parkreiner Parkreiner self-assigned this May 6, 2024
@Parkreiner
Copy link
Contributor Author

@BrunoQuaresma Do you happen to know why an e2e test would succeed locally, but not remotely, especially if it doesn't have anything to do with enterprise features?

@Parkreiner Parkreiner marked this pull request as draft May 6, 2024 19:18
@Parkreiner
Copy link
Contributor Author

Parkreiner commented May 6, 2024

Taking this out of review for the moment – I think I've realized what the problem is (not enough test isolation), and I'm going to tinker with this a bit more

orgId: string,
username: string = randomName(),
password: string = "s3cure&password!",
) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: instead of having the user params passed as an argument what about creating a param type that could merge/override the default values?

* JS-based runtime for the test environments to the Go-based runtime for the
* production backend.
*/
export async function assertNoUncaughtRenderError(page: Page): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe this function could have a more friendly name like assertNoUnexpectedUIError. I think the word Uncaught is not very common at least for me.

await setupNewUser();

await handleSignIn();
await handleSignOut();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the sign-in and sign-out tests are encapsulated into functions? I would expect each one to be a separate test.

@@ -19,6 +20,8 @@ export interface UserDropdownProps {
onSignOut: () => void;
}

export const accessibleDropdownLabel = "Open dropdown menu for user options";
Copy link
Collaborator

Choose a reason for hiding this comment

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

off: I'm always wondering how long labels should be. Imagining myself using an interface via audio... long texts could slow me down. Just a random thought.

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

So far I think you are in the right direction.

@Parkreiner Parkreiner closed this May 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants