-
Notifications
You must be signed in to change notification settings - Fork 60
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
playwright e2e tests #487
playwright e2e tests #487
Conversation
425fbbd
to
f20c8bf
Compare
e2e/basic.spec.ts
Outdated
['iframe', 'svelte'], | ||
['iframe', 'vue'], | ||
['iframe', 'htm'], | ||
].forEach(([sandbox, example]) => { |
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.
there's no test.each in playwright: https://playwright.dev/docs/test-parameterize#parameterized-tests
f20c8bf
to
f79056c
Compare
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.
Thanks so much for doing this, Robin! Left a few small requests and questions, but I am very glad to have this layer of validation.
e2e/basic.spec.ts
Outdated
|
||
await expect(page.getByText('Click Count: 2')).toBeVisible(); | ||
|
||
page.once('dialog', (dialog) => { |
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.
Can we store this as a promise and make sure it resolves, to ensure that the contained expect()
is actually hit?
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.
Good catch!
Got tests to fail when I removed the dialog
e2e/basic.spec.ts
Outdated
['iframe', 'preact'], | ||
['iframe', 'svelte'], | ||
['iframe', 'vue'], | ||
['iframe', 'htm'], |
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.
Can you add the other tuples for the worker, and comment out the ones that are currently broken?
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.
done
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.
/test-results/ | ||
/playwright-report/ | ||
/blob-report/ | ||
/playwright/.cache/ |
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.
Is there any way to configure playwright to shove all this in a single file, instead of spraying a bunch of directories all over the root of the project?
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.
Not that I'm aware of
e2e/basic.spec.ts
Outdated
@@ -0,0 +1,27 @@ | |||
import {test, expect} from '@playwright/test'; |
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.
Is x.spec.ts
the required/ conventional name for Playwright tests? I find it hard to know what .spec.ts
vs .test.ts
means at first glance, in the past I have used .e2e.ts
to make these tests more explicit.
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.
f79056c
to
878c917
Compare
878c917
to
51990d8
Compare
This adds a first set of playwright based e2e that leverage the existing kitchen sink example application to prevent further accidental regressions.
In this PR, we're adding tests for all the sandbox/framework constellations that currently work.
Everything using webworker is currently broken as well as react in the iframe. This is a separate issue to address that actually triggered me wanting to add e2e tests.
Once these work again, we can add them to the matix.
The tests are checking that the modal can be opened and that the counter works.
Successful run: https://github.com/Shopify/remote-dom/actions/runs/11808443962/job/32897020650?pr=487
Here's a job run with an error to prove that failing tests actually fail the build: https://github.com/Shopify/remote-dom/actions/runs/11808338067/job/32896744315