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

initial playwright setup and test #1039

Merged
merged 77 commits into from
Nov 23, 2023
Merged

initial playwright setup and test #1039

merged 77 commits into from
Nov 23, 2023

Conversation

Irev-Dev
Copy link
Collaborator

@Irev-Dev Irev-Dev commented Nov 9, 2023

Resolves to #1038

Because clicking around in our app will often only change pixel in the stream and not HTML, it kind of breaks how these e2e tests usually work. I've explained our workaround in a few places, but probably the best explanation because it also has a bit of a tutorial on how to get started writing a test for our app is here (15min watch)

Screen.Recording.2023-11-21.at.11.37.07.am.mp4

But the nuts and bolts of how we wait for changes from the engine is the react component in src/components/EngineCommands.tsx, but some changes to src/lang/std/engineConnection.ts were needed to.

Other random notes:

  • We run on two different runners and hopefully will unify to just Ubuntu at some point, more explanation here
  • data- attributes is a sanctioned way to add arbitrary attributes to HTML, and they give us something easy to query for in the command log/debug panel. Note that data-testid is a common convention for testing frameworks, having said that if you had the element <button data-testid="my-el">My Button</button> the following query page.getByRole('button', { name: 'My Button' }) is better than await page.getByTestId('my-el') because the former more closely represents how the user identities and interacts with the button. If the text within the button changed a lot because it uses a variable then using the testid might be warranted.

Copy link

vercel bot commented Nov 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Nov 23, 2023 9:56pm

playwright.config.ts Outdated Show resolved Hide resolved
@Irev-Dev
Copy link
Collaborator Author

Irev-Dev commented Nov 21, 2023

c341ed8 failed for reasons unrelated to playwright.

@mxschmitt
Copy link

Looks very nice! Saw this PR on Twitter and wanted to give some feedback:

I saw that there are a bunch of awaits missing in setViewportSize / addInitScript / emulateMedia. We recommend the following ESLint rule '@typescript-eslint/no-floating-promise' to prevent that as per here to prevent missing awaits which will/could lead to unexpected behaviour, since you don't ensure that the api call was completed.

Also instead of doing waitForFunction with querySelector we recommend waitFor() on the locator, e.g. page.getByRole(...).waitFor(). This will give better error reporting in trace-viewer.

I saw you are doing toMatchSnapshot for screenshots, if you use toHaveScreenshot instead, they will retry and wait for two consecutive page screenshots yield the same result etc. which is more robust.

nit: getUtils could be converted into a fixture, so that you can use it in other tests as well instead of having to call it all the time.

Its also possible to run an individual test by line number, e.g. yarn playwright test foo:23 (file which contains foo). Have you tried UI Mode?

All these are minor issues, looks great!

…ests (#1114)

* Ensure that Vite is serving before tests run

* Don't break secrets if developer has extra blank line in env file
@Irev-Dev
Copy link
Collaborator Author

Awesome @mxschmitt!

Heaps of good tips there thank you!

I'll get cracking on those and no I hadn't tried UI mode. I'll give that a go.

Copy link
Collaborator Author

@Irev-Dev Irev-Dev left a comment

Choose a reason for hiding this comment

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

Thanks again @mxschmitt, and of course thanks for your work on such a great tool!

I integrated all of your suggestions in 5f4cec1 with the exception of fixtures, I want to think about that a little more but created #1116.

I also have one remaining waitForFunction as noted below.

Also, UI mode is great, one minor inconvenience is the video only shows up in the timeline, but only minor.

image

But one great thing about UI mode is that it works with Google Chrome, as the "record new" and "record at cursor" extension features seemed to only launch with chromium, and we have a video encoding compat issue with that, so being able to use locator feature in UI-mode is great.

Comment on lines +39 to +43
await page.waitForFunction(
() =>
document.querySelectorAll('[data-receive-command-type="object_visible"]')
.length >= 3
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't figure out how to make a locator that waited for three of the same selector to appear, so kept this for the time being.

@Irev-Dev
Copy link
Collaborator Author

I'm going to merge this, even though the playwright CI is failing, it's because our dev engine has an issue right now. CI was passing before and the tests work locally.

@Irev-Dev Irev-Dev merged commit b88425e into main Nov 23, 2023
10 of 11 checks passed
@Irev-Dev Irev-Dev deleted the kurt-initial-playwright-1038 branch November 23, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants