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

Lower playwright default timeout #577

Open
1 task
sarayourfriend opened this issue Apr 11, 2022 · 2 comments
Open
1 task

Lower playwright default timeout #577

sarayourfriend opened this issue Apr 11, 2022 · 2 comments
Assignees
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend

Comments

@sarayourfriend
Copy link
Contributor

Problem

Our current default playwright timeout is very high (1 minute) and makes debugging new tests annoying. It's also potentially too flexible if we want to rely on playwright tests to give us some indication of the general performance of the application.

Description

Our current end to end tests should be very fast under most conditions. Specifically, unless a new page is being accessed (in which case a real API request will have to be made) the every request and navigation never leaves the local loopback. With our current high timeout of 1 minute, we're probably losing some insight into when parts of the application slow down, but we're also making debugging new tests annoying. If a selector you write doesn't work, then you have to wait a full minute for it to fail unless you remembered specifically to pass a shorter timeout to the selector function.

Curious what @WordPress/openverse-frontend folks think about this and what the lowered timeout should be if we did do it. Most of the time I find that 100 works fine, but it might be too low for certain things like page navigation. In those cases, should we expect the test writer to manually raise the timeout only for those cases when it is necessary? Or should we choose a middle ground like 1 or 2 seconds? Those still seem very high if we expect our app to be performant (especially considering we're taking network latency out of the equation except when running update-tapes).

If we do lower it, we'd also want to default to a higher value when updating tapes, so that we do account for the network latency.

Implementation

  • 🙋 I would be interested in implementing this feature.
@sarayourfriend sarayourfriend added 🟩 priority: low Low priority and doesn't need to be rushed 🤖 aspect: dx Concerns developers' experience with the codebase 💬 talk: discussion Open for discussions and feedback 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Apr 11, 2022
@obulat
Copy link
Contributor

obulat commented Apr 13, 2022

Having fast Playwright tests would be awesome. I am, however, worried that it might also make them flaky in CI if for whatever reason some tests become slower.

@obulat obulat transferred this issue from WordPress/openverse-frontend Feb 22, 2023
@obulat obulat added the 🧱 stack: frontend Related to the Nuxt frontend label Feb 22, 2023
@zackkrida zackkrida removed the 💬 talk: discussion Open for discussions and feedback label Sep 4, 2024
@zackkrida
Copy link
Member

zackkrida commented Sep 4, 2024

@WordPress/openverse-frontend and @sarayourfriend I looked into this a bit today. Lowering the timeout to 1s led to only two repeatable and consistent test failures:

  • e2e/report-media.spec.ts:138:9
  • e2e/load-more.spec.ts:75:11

Some other observations:

  • Playwright's default test timeout is 30s, so 50% less than ours.
  • This did not meaningfully change the time to run ov just frontend/run test:playwright locally it still took ~3 minutes regardless of the timeout. This is because the vast majority of tests run quite quickly and the ones that do exceed short timeouts are running concurrently with others, and still finish before the entire suite does.
  • The timing out tests may have configuration issues, or improperly aren't awaiting certain actions, or other issues causing the slowdown.

Conclusions

I think we can pretty easily reduce this number to 1s and add specific increased timeouts to the relevant tests. Or, even better, fix the likely issues with those tests.

@zackkrida zackkrida self-assigned this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend
Projects
Status: 📅 To Do
Development

No branches or pull requests

3 participants