Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
dannykopping merged 4 commits intocoder:mainfromdannykopping:dk/e2eea
Apr 18, 2024

Conversation

dannykopping
Copy link
Contributor

Closes#12585

Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykopping requested review fromParkreiner andmtojek and removed request forParkreinerApril 17, 2024 09:36
@dannykoppingdannykopping changed the titlechore: add e2e test for authenticating against an external auth provider during workspace creationchore: add e2e test against an external auth provider during workspace creationApr 17, 2024
Copy link
Member

@mtojekmtojek left a comment
edited
Loading

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}
Copy link
Member

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.

dannykopping reacted with thumbs up emoji
Copy link
Member

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

Copy link
Member

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
Copy link
ContributorAuthor

dannykopping commentedApr 17, 2024
edited
Loading

CI seems to be still failing.

Hhmm yeah seems like the server is already running (I just copied some code from another test for the server):
Error: listen EADDRINUSE: address already in use 0.0.0.0:50516.

I guess it's not shutdown once the test completes...?

Copy link
Member

@ParkreinerParkreiner left a 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

Comment on lines 70 to 72
const externalAuthLoginButton = page.locator(
"#external-auth-" + useExternalAuthProvider,
);
Copy link
Member

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

dannykopping reacted with thumbs up emoji
Comment on lines 50 to +53
templateName: string,
richParameters: RichParameter[] = [],
buildParameters: WorkspaceBuildParameter[] = [],
useExternalAuthProvider: string | undefined = undefined,
Copy link
Member

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;}>

Copy link
ContributorAuthor

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.

@mtojek
Copy link
Member

I guess it's not shutdown once the test completes...?

This or maybe tests are just running in parallel

@dannykopping
Copy link
ContributorAuthor

I guess it's not shutdown once the test completes...?

This or maybe tests are just running in parallel

AFAICS they seem to run serially.
I chased this down a little yesterday and found that there'salso a shared state issue between tests; the auth tokens are persistent across test runs, so the "Login with ..." button is replaced with "Authenticated with ..." since there is a valid token. I'm looking into clearing the db state between runs.

…st to reauthenticateSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping
Copy link
ContributorAuthor

dannykopping commentedApr 18, 2024
edited
Loading

Wow, OK. That was a bit tricky...

Whenexternal auth web ran, it'd create a valid external auth link such that whensuccessful external auth from workspace ran subsequently, theLogin with GitHub button was inactive because the session was already authenticated; this kind of leakage between tests is not ideal.

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.

mtojek reacted with rocket emoji

@mtojekmtojek self-requested a reviewApril 18, 2024 14:03
Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

Nice job 👍!

@dannykoppingdannykopping merged commit319fd5b intocoder:mainApr 18, 2024
@dannykoppingdannykopping deleted the dk/e2eea branchApril 18, 2024 17:43
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsApr 18, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@ParkreinerParkreinerParkreiner left review comments

@mtojekmtojekmtojek approved these changes

Assignees

@dannykoppingdannykopping

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

e2e: test authenticating with external auth when creating workspace
3 participants
@dannykopping@mtojek@Parkreiner

[8]ページ先頭

©2009-2025 Movatter.jp