- Notifications
You must be signed in to change notification settings - Fork928
chore: add e2e test against an external auth provider during workspace creation#12985
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
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
mtojek left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
A simple mock looks like a decent solution 👍 Anyway, CI seems to be still failing.
@@ -27,6 +27,7 @@ export const ExternalAuthButton: FC<ExternalAuthButtonProps> = ({ | |||
<> | |||
<div css={{ display: "flex", alignItems: "center", gap: 8 }}> | |||
<LoadingButton | |||
id={"external-auth-" + auth.id} |
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.
if this is just for testing purposes, then probablydata-testid
.
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.
Yeah, IDs are mainly supposed to be used for associating HTML elements with other elements, or associating elements with browser URL state
I want to say that if you need some kind of "ID-like value"/testing hook for your elements, and the value doesn't have any functionality for the end-user, data attributes (likedata-testid
) are the way to go
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.
Another concern is that HTML IDs have to be globally unique. If multiple HTML elements have the same ID, that can cause some nasty jank (e.g., clicking a form input, and a different input getting highlighted)
React has a built-in hook for making it easy to have unique IDs, even when you're working with reusable components
https://react.dev/reference/react/useId
dannykopping commentedApr 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hhmm yeah seems like the server is already running (I just copied some code from another test for the server): I guess it's not shutdown once the test completes...? |
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.
Had a couple of comments on how some of the functions were set up, but I think this looks good overall
As long as getting the test to pass won't dramatically change the code, I can approve this
site/e2e/helpers.ts Outdated
const externalAuthLoginButton = page.locator( | ||
"#external-auth-" + useExternalAuthProvider, | ||
); |
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.
Regardless of the main way we end up selecting the element, I would recommend making sure the selector asserts that the element we're grabbing is actually exposed to the user as a button (ideally through something likepage.getByRole
)
It's possible to add click behavior to non-button elements, but that can get janky quick. Changing the selector would add an extra security net to ensure that users can use all the behavior that buttons bake in automatically
templateName: string, | ||
richParameters: RichParameter[] = [], | ||
buildParameters: WorkspaceBuildParameter[] = [], | ||
useExternalAuthProvider: string | undefined = undefined, |
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 think that with the new parameter, the function is getting a little unwieldy, and if you're looking at things from the consumer side, it's hard to tell which of the five arguments correspond to what
Maybe this could be refactored to an object, so that we have sort of have named arguments?
typeCreateWorkspaceInputs=Readonly<{// Required propertiespage:Page;templateName:string;// Optional propertiesrichParameters?:readonlyRichParameter[];buildParameters?:readonlyWorkspaceBuildParameter[];externalAuthProvider?:string;}>
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 agree in principle, but I think 5 params is still manageable and not worth refactoring the 26 usages of this function IMHO. Happy to discuss.
This or maybe tests are just running in parallel |
AFAICS they seem to run serially. |
…st to reauthenticateSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
dannykopping commentedApr 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Wow, OK. That was a bit tricky... When I needed to delete the external auth link between the session & the oauth token to force each test to authenticate afresh. Additionally I had to make the mock oauth server start before all the tests rather than creating one per test. @mtojek@Parkreiner would you mind taking another look? I've addressed the other review comments, too. |
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.
Nice job 👍!
Closes#12585