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

E2E sharding part 2 #90

Closed
wants to merge 105 commits into from
Closed

E2E sharding part 2 #90

wants to merge 105 commits into from

Conversation

rylew1
Copy link

@rylew1 rylew1 commented Nov 8, 2024

Ticket

Resolves navapbc/template-infra#720

Changes

  • wrap playwright merge cmd in make target e2e-merge-reports
  • set matrix fail-fast to false so reports can be generated if one of the shards fails
  • update docs for sharding

Context for reviewers

Testing

Local sharding and report demo:

make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=1 CI=true && \
make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=2 CI=true && \
make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=3 CI=true

make e2e-merge-reports REPORT_PATH=blob-report
make e2e-show-report
make e2e-clean-report

the above commands create a combined report locally:
image

Demo of one shard failing (fail-fast demo) - html report is still created:

output.mov

Demo of nodejs caching - skips make e2e-setup-ci step on cache hit:
image

Preview environment

matrix:
shard: [1, 2, 3]
total_shards: [3] # Github Actions doesn't have a built-in method to get the length of an array
fail-fast: false
Copy link
Author

@rylew1 rylew1 Nov 13, 2024

Choose a reason for hiding this comment

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

add a fail-fast to false on the matrix strategy here so even if one shard fails, we still can generate a report out of othershard runs that have succeeded. See PR description for a demo of this working

@lorenyu
Copy link
Collaborator

lorenyu commented Nov 14, 2024

is this PR ready for review? i'm confused it looks like there are some changes in here that were already in the previous PR related to e2e tests. did you merge main into this branch

@rylew1
Copy link
Author

rylew1 commented Nov 14, 2024

I'm confused it looks like there are some changes in here that were already in the previous PR related to e2e tests. did you merge main into this branch

@lorenyu There was some shuffle due to the template deploy workflow not working for a couple days - I ended up branching this PR of the sharding part 1 PR just to get started before the template-deploy was fixed - but I'm not sure what parts you're thinking of that are older.

  • I think the download blob reports step may not have been updated to use ./e2e/blob-report on the PR merge to template-infra - so updating them in this PR

  • I had to update the package-lock.json to fix native run of e2e tests

I've updated more today, including removing the docker setup cache because it was taking too long - I think it's ready for review

@@ -4,231 +4,231 @@
"lockfileVersion": 2,
"requires": true,
"packages": {
"": {
Copy link
Author

@rylew1 rylew1 Nov 15, 2024

Choose a reason for hiding this comment

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

Had to update the lock file to run tests natively/locally

@rylew1 rylew1 marked this pull request as ready for review November 15, 2024 01:40
Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Looking good but let's resolve the issue with some previous changes not appearing on main first

BASE_URL=${{ inputs.service_endpoint }} \
CURRENT_SHARD=${{ matrix.shard }} \
TOTAL_SHARDS=${{ matrix.total_shards }}
make e2e-test \
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the extra indent

id: cache-node
uses: actions/cache@v4
with:
path: e2e/node_modules
Copy link
Collaborator

Choose a reason for hiding this comment

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

indenting too much here let's be consistent. do you need to check editor settings? i would have imagined the editor would autoformat on save

Copy link
Author

Choose a reason for hiding this comment

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

Are we using actionlint for this? What formatter are you using locally for these yml files

Makefile Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Show resolved Hide resolved
.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docs/e2e/e2e-checks.md Show resolved Hide resolved
docs/e2e/e2e-checks.md Outdated Show resolved Hide resolved
@rylew1
Copy link
Author

rylew1 commented Nov 18, 2024

Merged on template-infra navapbc/template-infra#782

@rylew1 rylew1 closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sharding capabilities to e2e tests
3 participants