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

[Issue ##2798] Break up e2e tests #2599

Merged
merged 1 commit into from
Nov 12, 2024
Merged

[Issue ##2798] Break up e2e tests #2599

merged 1 commit into from
Nov 12, 2024

Conversation

acouch
Copy link
Collaborator

@acouch acouch commented Oct 28, 2024

Summary

Fixes #2798

Time to review: 5 mins

Changes proposed

Update e2e tests so they are sharded across 3 processes that run in parallel. Time per test reduced by a couple of mins. More importantly the tests seem less flaky. This not as big a deal now but the tests should scale as we grow.

@rylew1
Copy link
Contributor

rylew1 commented Oct 30, 2024

If you want to shave some additional time on the docker builds, have you tried to use npm commands to run tests in the Dockerfile? - can remove the make install and copying of the Makefile over .

@acouch acouch force-pushed the acouch/shard-e2e-tests branch 2 times, most recently from ab1e31c to 7c33688 Compare November 4, 2024 19:27
@acouch acouch changed the title Update ci-frontend-e2e.yml to shard tests [Issue ##2798] Break up e2e tests Nov 8, 2024
@acouch acouch marked this pull request as ready for review November 8, 2024 23:21
Copy link
Collaborator

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

Cool! Haven't seen the shards thing before

- name: Run e2e tests (Shard ${{ matrix.shard }}/${{ matrix.total_shards }})
env:
CI: true
TOTAL_SHARDS: ${{ matrix.total_shards }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious about the decision to use environment variables, which I can't find a reference to in the docs, versus the --shards flag that seems to be recommended https://playwright.dev/docs/test-sharding#sharding-tests-between-multiple-machines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This strategy was inherited from the platform, @rylew1 ?

cat .env.development >> .env.local
npm run test:e2e
npm run build
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like since the sharded test runs are all going in parallel that the npm and playwright setup steps are not able to be cached, so 5 mins of processing time is being spent on each run on something that it seems like could be shared. I'm a novice with github actions, but is this a place where a reusable workflow could help? https://docs.github.com/en/actions/sharing-automations/reusing-workflows#creating-a-reusable-workflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this would be a good follow up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created ticket #2813

@acouch acouch merged commit 954d463 into main Nov 12, 2024
14 checks passed
@acouch acouch deleted the acouch/shard-e2e-tests branch November 12, 2024 16:25
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.

Shard e2e tests to make them faster and more reliable
4 participants