remove needless hovers and visibility checks from e2e #3440
Conversation
sqs
force-pushed
the
sqs/no-at-prefix-context-list
branch
from
1e31830
to
b69baec
Compare
sqs
force-pushed
the
sqs/no-at-prefix-context-list
branch
from
b69baec
to
7ac8e2c
Compare
sqs
force-pushed
the
sqs/no-at-prefix-context-list
branch
from
7ac8e2c
to
a0f2ddd
Compare
sqs
force-pushed
the
sqs/no-at-prefix-context-list
branch
from
a0f2ddd
to
004ecb5
Compare
In our e2e tests, there were many patterns like (where `x` is some Playwright locator, ie a selector query resolved by polling): ``` await x.hover() await x.click() ``` The intent of this was to give extra time for `x` to become visible. But this is unnecessary because Playwright's `.click()` already auto-waits for the locator to become [actionable (see Playwright's definition)](https://playwright.dev/docs/actionability). The effect of these 2 lines is just to double the timeout (from 3s to 6s), which is not usually necessary. If a longer timeout is indeed needed, you can use `.click({timeout: N})`, and if a longer timeout than 3s is ever needed by default, we can update `playwright.config.ts` (but I don't think that's needed). Same goes for this pattern: ``` await expect(x).toBeVisible() x.click() ``` Playwright's definition of actionability (linked above) already checks for visibility. In some cases where the intent of `await x.hover()` was to wait for something to become visible, I changed it to `await expect(x).toBeVisible()` for clarity. There should be no behavior change (other than 3s not 6s timeouts) with these changes.
|
@philipp-spiess It does throw. It worked (aka was harmless) because it was being used as either a visibility check or a way to give a click an extra timeout. |
|
I don't recall the specifics (and can't access the slack threads), but I remember having some weird issues on Windows where clicks just didn't work without a hover before them and I don't believe it was visibility related. When running some tests locally (on Windows) I could see the buttons on the screen and the test was just paused waiting to click (and eventually would time out), but if I manually moved my mouse over the button then suddenly the test would continue. It was almost like clicks didn't always work unless there had been a mouseover event first. I couldn't find any good reasons for it though, but I noticed some other tests were doing the same and passing, so I added the hovers and things passed 😞 |
In our e2e tests, there were many patterns like (where
xis some Playwright locator, ie a selector query resolved by polling):The intent of this pattern is (except in rare cases where hovering is actually needed) to give extra time for
xto become visible. But this is unnecessary because Playwright's.click()already auto-waits for the locator to become actionable (see Playwright's definition). The effect of these 2 lines is just to double the timeout (from 3s to 6s), which is not usually necessary. If a longer timeout is indeed needed, you can use.click({timeout: N}), and if a longer timeout than 3s is ever needed by default, we can updateplaywright.config.ts(but I don't think that's needed).Same goes for this pattern:
Playwright's definition of actionability (linked above) already checks for visibility.
In some cases where the intent of
await x.hover()was to wait for something to become visible, I changed it toawait expect(x).toBeVisible()for clarity.There should be no behavior change (other than 3s not 6s timeouts) with these changes.
Test plan
CI