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

Group e2e tests #1693

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Group e2e tests #1693

merged 1 commit into from
Nov 30, 2023

Conversation

jotaen4tinypilot
Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot commented Nov 29, 2023

As demonstrated in #1685, we can make use of Playwright’s grouping feature to improve our e2e test structure:

  • Instead of ordinary block scopes, we can use dedicated and isolated test clauses. These blocks are grouped inside an enclosing test.describe.
  • The individual test aspects now have their own description, which improves readability.
  • If an individual test aspects fails, the CLI will also output the name of the test, so it’s easier to debug. The report is also more fine-granular:
  • It seems that the execution time is slightly slower now compared to before, though it’s hard to tell exactly, because there is significant variance between passes.
    • In the reports linked above, you see an execution time of ~12s (after) vs. ~9s (before). The bootstrapping procedure of the CI job (installing dependencies, etc.) is usually 60–90 seconds, however, so I’m not sure we should be worried too much at this point. I also think we should have more leverage now to speed up the tests through parallelizing.
    • test clauses are usually run in parallel by default in Playwright, though it seems we opted out of test parallelism in our Playwright config. I’m not sure why we did that, but we could now consider changing it back. (If at all, we should do that separately, though.)

This PR is a non-functional refactoring. It only introduces the grouping + group descriptions, without any other modifications (except code style).

Tagged @cghague for review, since we just ran into this topic recently, so I thought it was fitting. I’d take care of adjusting the Pro-specific tests once we have merged this over.
Review on CodeApprove

Copy link
Contributor

@cghague cghague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

Approved on CodeApprove
✔️ Approved

LGTM


👀 @jotaen4tinypilot it's your turn please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants