-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
test(item): migrate tests to playwright #25479
Conversation
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.
Need to update alignment tests so that the ion-datetime
has a fixed date (prevent it from defaulting to today).
Also need to check how to resize the viewport correctly to accommodate the "form" screenshot tests, which have a large horizontal viewport.
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.
Great job!
@@ -27,7 +27,7 @@ | |||
<ion-list-header>Leading Icons</ion-list-header> | |||
<ion-item> | |||
<ion-icon slot="start" name="time"></ion-icon> | |||
<ion-datetime display-format="DDDD MMMM D YYYY hh:mm:ss a" value="2019-10-01T15:43:40.394Z"></ion-datetime> | |||
<ion-datetime value="2019-10-01T15:43:40.394Z"></ion-datetime> |
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.
Do we need to use ion-datetime
here? Wondering if we can use something else instead to make the screenshots a bit smaller
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 don't believe so, I can swap those out with inputs 👍
|
||
await page.setIonViewport(); | ||
|
||
await page.click('text=Edit'); |
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.
~ Might be better to use a more stable selector, like an ID.
import { test } from '@utils/test/playwright'; | ||
|
||
test.describe('item: inputs', () => { | ||
test('should not have visual regressions', async ({ 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.
At least one of the datetimes is missing entirely here:
core/src/components/item/test/inputs/item.e2e.ts-snapshots/item-inputs-ios-rtl-Mobile-Firefox-linux.png
core/src/components/item/test/inputs/item.e2e.ts-snapshots/item-inputs-md-ltr-Mobile-Safari-linux.png
core/src/components/item/test/inputs/item.e2e.ts-snapshots/item-inputs-md-rtl-Mobile-Safari-linux.png
You may need to do await page.waitForSelector('.datetime-ready')
?
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.
Great work! Better to deal with all that now, than later when the test would inevitably start flaking on us 🤭
Pull request checklist
Please check if your PR fulfills the following requirements:
ionic-docs
repo, in a separate PR. See the contributing guide for details.npm run build
) was run locally and any changes were pushednpm run lint
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Item tests are written in Puppeteer.
Issue URL: N/A
What is the new behavior?
Migrates tests to Playwright
setIonViewport
that allows resizing the horizontal size of the viewport to match the scroll contents (for niche tests like the form test for item).Does this introduce a breaking change?
Other information