-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Be more accepting of select options #1604
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.
Works well, just added some thoughts about the tests! Feel free to disagree.
await frame.locator('text="foo typography"').waitFor({ state: 'visible' }); | ||
await frame.locator('label:has-text("foo select")').waitFor({ state: 'visible' }); | ||
|
||
const optionsSelect = frame.getByRole('button', { name: /select with options/ }); |
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.
Should this more specific testing for the Select be in its own test?
Just because so far what this test was doing was just checking that every component renders in a very simple way, but now it's becoming a bit more complex.
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.
Maybe, I'm sort of avoiding splitting tests too much as to keep the footprint down. The whole test with setup and page load takes about 2/3 seconds. While just interacting with this select takes an order of magnitude less.
But perhaps it is sensible to add one test per component
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.
Yeah that's right, with a page load in every test things can get a lot slower...
One test per component sounds like a good future-proof approach - we could also maybe slowly get rid of this general test as we create one test for each component.
I think the way you changed it is good!
} | ||
|
||
test('components', async ({ page, browserName, api }) => { | ||
test('components runtime', async ({ page, api }) => { |
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 think it would be helpful for the test names to be more descriptive of what's being tested, like for example must render components in the runtime
. Just an example!
await page.locator('text="foo typography"').waitFor({ state: 'visible' }); | ||
await page.locator('label:has-text("foo select")').waitFor({ state: 'visible' }); | ||
async function waitForComponents(page: Page, frame: Page | FrameLocator = page, isEditor = false) { | ||
await frame.locator('text="foo button"').waitFor({ state: 'visible' }); |
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'm also thinking if it would be useful to have comments clarifying which component is being checked in each of these lines? The selector might not always be explicit enough.
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'll assign those locators to variables with more descriptive names. That makes it self-documenting.
Make sure we always show something, even if the options array contains numbers, or
undefined