Skip to content

chore: skip global.setup if first user already exists #12930

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

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 10, 2024

Treat global.setup.ts as a setup, rather than a test. This allows running in --ui mode and running tests more than once.

All tests currently share the same Coder instance. When running in UI mode, it allows running tests by choice. This should emulate the same behavior as running them as 1 set.

If you run a test more than once, it would have to be able to handle that, given it would run the same test in the same coderd state.


Since this is for debugging, I think that is ok for now.

Screencast.from.2024-04-10.15-56-28.webm

treat test as a setup, rather than a test
Comment on lines 19 to 20
"//": "If we are in a coder workspace, use the `ui-port` to allow opening remotely. If native, use the default app. It's generally a bit smoother to use.",
"playwright:test-ui": "playwright test --config=e2e/playwright.config.ts --ui $([[ \"$CODER\" == \"true\" ]] && echo --ui-port=7500 --ui-host=0.0.0.0)",
Copy link
Member Author

Choose a reason for hiding this comment

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

@aslilac This works on local or in coder.

Copy link
Member

Choose a reason for hiding this comment

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

why not just set these flags all the time? they're harmless options and seem like they should always work.

@Emyrk Emyrk requested a review from aslilac April 10, 2024 20:31
site/e2e/api.ts Outdated
export const setupApiCalls = async (page: Page) => {
const token = await findSessionToken(page);
API.setSessionToken(token);
export const setupApiCalls = async (page: Page, unauthenticated?: boolean) => {
Copy link
Member

Choose a reason for hiding this comment

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

having a boolean as an optional argument is almost always a bad idea imo, because from the call site setupApiCalls(page, true) is incredibly vague. it should either be on an options object, or ideally imo, it would just gracefully do nothing if there is no token, and not take any additional params.

try {
  const token = await findSessionToken(page);
  API.setSessionToken(token);
} catch {}

Comment on lines 19 to 20
"//": "If we are in a coder workspace, use the `ui-port` to allow opening remotely. If native, use the default app. It's generally a bit smoother to use.",
"playwright:test-ui": "playwright test --config=e2e/playwright.config.ts --ui $([[ \"$CODER\" == \"true\" ]] && echo --ui-port=7500 --ui-host=0.0.0.0)",
Copy link
Member

Choose a reason for hiding this comment

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

why not just set these flags all the time? they're harmless options and seem like they should always work.

@@ -16,6 +16,8 @@
"lint:types": "tsc -p .",
"playwright:install": "playwright install --with-deps chromium",
"playwright:test": "playwright test --config=e2e/playwright.config.ts",
"//": "If we are in a coder workspace, use the `ui-port` to allow opening remotely. If native, use the default app. It's generally a bit smoother to use.",
Copy link
Member

Choose a reason for hiding this comment

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

pls no, I hate these hacky "json comments" 😭

@Emyrk
Copy link
Member Author

Emyrk commented Apr 10, 2024

why not just set these flags all the time? they're harmless options and seem like they should always work.

Running playwright:test is easier and runs the whole suite. The UI mode is extra clicks, only really needed to debug a certain test. Idk what the general take here is in package.jsons. I wish you could add a flag and make --ui expand to what I have.

@aslilac
Copy link
Member

aslilac commented Apr 10, 2024

but this weird interpolation is only in the playwright:test-ui script, which will either expand to...

playwright test --config=e2e/playwright.config.ts --ui # without the $CODER env variable

...or...

playwright test --config=e2e/playwright.config.ts --ui --ui-port=7500 --ui-host=0.0.0.0 # with the $CODER env variable

which are identical, except the explicit port and host. I don't get why we wouldn't just want those to be explicit all the time.

@aslilac
Copy link
Member

aslilac commented Apr 10, 2024

I'm not saying we should add all the --ui* flags to the playwright:test script, I'm trying to figure out why the new script is so weird. 😅

@Emyrk
Copy link
Member Author

Emyrk commented Apr 11, 2024

but this weird interpolation is only in the playwright:test-ui script, which will either expand to...

playwright test --config=e2e/playwright.config.ts --ui # without the $CODER env variable

...or...

playwright test --config=e2e/playwright.config.ts --ui --ui-port=7500 --ui-host=0.0.0.0 # with the $CODER env variable

which are identical, except the explicit port and host. I don't get why we wouldn't just want those to be explicit all the time.

Ah. If you run --ui, it pops up in it's own chromium window, and will only work if running locally. I can't tell you why, but for some reason this popup window seems to behave better for me. Maybe it's unrelated, but the ui hosted on a port would randomly freeze for me, and I have to close the tab and open a new one 🤷

Co-authored-by: Kayla Washburn-Love <mckayla@hey.com>
@Emyrk Emyrk enabled auto-merge (squash) April 11, 2024 21:10
@Emyrk Emyrk merged commit 93b46fe into main Apr 11, 2024
25 checks passed
@Emyrk Emyrk deleted the stevenmasley/playwright-ui branch April 11, 2024 21:10
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants