- Notifications
You must be signed in to change notification settings - Fork926
test(site): add e2e tests for user auth#12971
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Non-blocking: I'm somewhat worried about all of the test logic getting stuffed intoverifyConfigFlag
. Any other special cases will also need to get stuffed in there.
I wonder if we could instead refactorverifyConfigFlag
to accept a function that takes a function as an argument. This function should just run whatever assertions are necessary for the particular config flag.
Then the responsibility for asserting the content of the node moves back up to the test instead of in this function, e.g.
verifyConfigFlag(page, config, flag, (configOpt) -> { expect(configOpt.locator(`...`)).toBeVisible() })
andverifyConfigFlag
is just concerned with finding the relevant option on the page and passing it to the function.
WDYT?
I agree, this is a reasonable idea, although I wouldn't pack refactoring here. I refactor the logic in the follow-up to this PR. Thanks for the review 👍 |
Uh oh!
There was an error while loading.Please reload this page.
Related:#12508
This PR adds e2e tests for user authentication config.