Add sharding to e2e tests #89

rylew1 · 2024-10-28T01:19:28Z

Ticket

Resolves navapbc/template-infra#720

Changes

add sharding to e2e tests

https://playwright.dev/docs/test-sharding

TODO

Will likely be part 1 of 2 PR - additional items in future PR:

make target for merge report command
Supporting partial shard report upload
Docker caching/build improvements
Docs updates

Demo

CI:

382633894-ae91a0bd-c3d7-4920-9432-2897df1d064b.2.mov

Local:

output.mov

Preview environment

♻️ Environment destroyed ♻️

e2e/playwright.config.js

lorenyu

Left some initial comments

.github/workflows/e2e-tests.yml

e2e/playwright.config.js

acouch · 2024-10-29T14:05:09Z

I'm testing this out here: HHS/simpler-grants-gov#2599 It seems to work but unclear if it actually speeds things up. Still working on that PR.

.github/workflows/e2e-tests.yml

acouch · 2024-10-29T14:40:34Z

Would be great if there is a way to cache the steps before the matrix. It looks like the all of the build steps are repeated for each one, which is where most of the time for us is spent.

rylew1 · 2024-10-29T15:59:10Z

Would be great if there is a way to cache the steps before the matrix. It looks like the all of the build steps are repeated for each one, which is where most of the time for us is spent.

@acouch on the other e2e Dockerize PR we optimized it a little bit to create the install as one layer so it would cache better #88 (comment) . (we are close to merging there so may want to take a peek at it for grants)

However I don't know if that caching will translate to multiple shard runs, I think each shard run will need to do the full build each time - will have to research that.

It may be that sharding is only beneficial when the bottleneck is the test run time.

lorenyu

Looks pretty good just had a few questions

.github/workflows/e2e-tests.yml

lorenyu · 2024-11-05T18:33:34Z

.github/workflows/e2e-tests.yml

+          ls -R ./e2e/blob-report || echo "blob-report directory not found"
+
+      - name: Upload Blob Report
+        if: ${{ !cancelled() }}


question: what use case is this for?

An extra safeguard that on manual or timeout-based cancellations, steps within create-report don't run - but I think since create-report has a needs dependency on e2e we don't need ths line - let's remove.

They were originally using it in there merge and upload blob reports on the playwright sharding docs https://playwright.dev/docs/test-sharding#github-actions-example

if you look at their example it's because they set fail-fast to false, but they still want to upload the blob even if the e2e tests failed, and they still want to merge even if some of the shards failed. seems like we'd want to do the same, and add a comment saying that "still upload report even if previous step failed"

.github/workflows/e2e-tests.yml

lorenyu · 2024-11-05T19:33:37Z

.github/workflows/e2e-tests.yml

+      - name: Install dependencies in ./e2e
+        run: make e2e-setup-ci


Curious, how come we're running e2e tests in Docker but running the merge reports natively?

The create-report job has to be in a separate "job" that runs after all the shard runs - merging can't really be done in any one of the shard job containers.

I'm not sure there's any need to increase our CI job run time to spin up a container to do a merge. And merging reports probably only needs to run in CI.

Locally I don't think we'd ever want to shard - so the report there is just html in the playwright-report folder.

It also raises the question - should the e2e job (running of tests) instead be run natively in CI? It may speed things up if we just run tests natively vs a container.

However, I think we should still use docker in CI - the CI run should be consistent to how you run it locally in the container. E2E tests can be flaky and I think it makes sense to eliminate any environment issues at play both locally and in the CI run.

That makes sense thanks for explaining. I still think it'd be valuable to convert the line run: npx playwright merge-reports --reporter html ./all-blob-reports to a make target like run: make e2e-merge-report or something like that so that we can test the sharding locally. I think also that in order to test sharding locally, when we download the blob artifacts we should download them to the same place that they were in originally.

so in other words locally we can do

make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=1 make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=2 make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 CURRENT_SHARD=3 make e2e-merge-reports

and in CI we'd do something like

jobs: e2e: matrix: shard: [1,2,3] total_shards: [3] steps: - run: make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=${ matrix.total_shards } CURRENT_SHARD=${ matrix.shard } - # upload artifact from ./e2e/blob-report/* create-report: steps: - # download artifact to ./e2e/blob-report/ - run: make e2e-merge-reports

@lorenyu you also have to set CI=true right now - but this does work locally - we can try in followup pr:

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 cd e2e npx playwright merge-reports --reporter html ./blob-report cd .. make e2e-show-report make e2e-clean-report

I've opened the PR on template infra to merge part 1 and will try to clean some of this up in part 2 pr

lorenyu · 2024-11-05T19:42:35Z

Makefile

 e2e-test: e2e-build
 	@:$(call check_defined, APP_NAME, You must pass in a specific APP_NAME)
 	@:$(call check_defined, BASE_URL, You must pass in a BASE_URL)
-	docker run --rm \
+	docker run --rm\
 		--name playwright-e2e-container \
 		-e APP_NAME=$(APP_NAME) \
 		-e BASE_URL=$(BASE_URL) \
+		-e CURRENT_SHARD=$(CURRENT_SHARD) \
+		-e TOTAL_SHARDS=$(TOTAL_SHARDS) \
+		-e CI=$(CI) \
 		-v $(PWD)/e2e/playwright-report:/e2e/playwright-report \
+		-v $(PWD)/e2e/blob-report:/e2e/blob-report \
 		playwright-e2e


Question: What happens if you run make e2e-test with CURRENT_SHARD and TOTAL_SHARDS undefined?

I run it locally with those values undefined - it just defaults to 1 per ./e2e/playwright.config.js:

make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 local output: Running 6 tests using 6 workers, shard 1 of 1

make e2e-test runs the container - which invokes the default Dockerfile CMD of make e2e-test-native - which invokes npx playwright test - which runs test with the config ./e2e/playwright.config.js - which defaults the shards to 1

platform-test-nextjs/e2e/playwright.config.js

Lines 36 to 41 in 1a1e63c

shard: {

// Total number of shards

total: parseInt(process.env.TOTAL_SHARDS || '1'),

// Specifies which shard this job should execute

current: parseInt(process.env.CURRENT_SHARD || '1'),

},

hmm if I run with TOTAL_SHARDS set locally it actually just runs 1 shard - I'm not sure if we want to support sharding locally

make e2e-test APP_NAME=app BASE_URL=http://host.docker.internal:3001 TOTAL_SHARDS=3 .... Running 2 tests using 2 workers, shard 1 of 3 (just runs 2 of the 6 total tests)

I don't think we need to support sharding in local

I don't think we need to support sharding in local

That's fine, but for testing purposes you can still run the following right?

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

but for testing purposes you can still run the following right?

see the comment above #89 (comment)

.github/workflows/e2e-tests.yml

Co-authored-by: Loren Yu <[email protected]>

…-test-nextjs into rylew/e2e-shard

rylew1 · 2024-11-06T01:43:31Z

I propose a fast follow PR for docs updates and trying to better cache some of the docker - right now it takes about 7-8 minutes to run update-pr-environment - with about a minute for the make e2e-test which we might be able to improve

.github/workflows/e2e-tests.yml

lorenyu · 2024-11-06T17:28:10Z

I propose a fast follow PR for docs updates and trying to better cache some of the docker - right now it takes about 7-8 minutes to run update-pr-environment - with about a minute for the make e2e-test which we might be able to improve

Sounds good to me

lorenyu

Looks good, feel free to do the change I requested in a followup PR. For reference the change I requested is to:

change the download artifact path from ./all-blob-reports to ./e2e/blob-report
move the line npx playwright merge-reports --reporter html ./e2e/blob-report into a make target like make e2e-merge-reports

this way we can test locally and also run in CI

npx playwright merge-reports --reporter html ./all-blob-reports

Co-authored-by: Loren Yu <[email protected]>

rylew1 added 7 commits October 27, 2024 21:16

try sharding

20eb003

update shard workflow name and report

5fa0f7a

try merge reports

bfb6601

fix output folder

e1a16e6

revert to example

7e44eae

add verify blob dir step

ff755f7

try ci true

91cd9b0

rylew1 requested review from lorenyu and acouch October 28, 2024 03:11

rylew1 commented Oct 28, 2024

View reviewed changes

e2e/playwright.config.js Show resolved Hide resolved

rylew1 marked this pull request as draft October 28, 2024 03:20

rylew1 requested a review from doshitan October 28, 2024 15:55

lorenyu reviewed Oct 28, 2024

View reviewed changes

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved

e2e/playwright.config.js Show resolved Hide resolved

remove env block in favor of passing args

7eafd22

acouch reviewed Oct 29, 2024

View reviewed changes

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved

rylew1 and others added 12 commits October 30, 2024 19:10

Merge branch 'main' into rylew/e2e-shard

7c880b1

remove older makefile updates

884b604

try updated package-lock

b9a6e04

try updated workflow file

5ed2fe8

allow e2e-test to pass in shard config

e662964

print all env vars

77e2cc0

updates

086e85b

update path on blob-report

6d2d48e

map blob-report dir on e2e-test cmd

d32452d

docker cp

381aa81

docker cp in e2e-test

a239337

remove -rm on e2e-test

a03ca1e

rylew1 requested a review from lorenyu November 4, 2024 13:43

rylew1 marked this pull request as ready for review November 4, 2024 13:49

rylew1 requested review from doshitan and removed request for doshitan November 4, 2024 18:23

rylew1 added 6 commits November 4, 2024 13:27

remove creating blob-report dir on host ci runner

f9b03fe

try npm and playwright caching in workflow

64082a9

add needs install step

b630221

remove unnecessary install step

e782d32

try without e2e-setup-ci in create-report

14e0c6d

reintroduce e2e-setup-ci in merge test reports

b6b7e58

lorenyu reviewed Nov 5, 2024

View reviewed changes

rylew1 and others added 7 commits November 5, 2024 18:13

Update .github/workflows/e2e-tests.yml

427d10b

Co-authored-by: Loren Yu <[email protected]>

sentence casing

5fddd48

Merge branch 'rylew/e2e-shard' of https://github.com/navapbc/platform…

1e2afc8

…-test-nextjs into rylew/e2e-shard

rename blob-report-shard-*

4cfe512

more sentence casing

1a1e63c

comment for total_shards

8966647

remove !cancelled() from create-report job

830e1b2

rylew1 added 2 commits November 5, 2024 20:48

try blank e2e job name

04d1fed

reintroduce e2e job name

1f8ec00

rylew1 requested review from lorenyu and acouch November 6, 2024 02:08

lorenyu reviewed Nov 6, 2024

View reviewed changes

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved

lorenyu approved these changes Nov 6, 2024

View reviewed changes

job name change

9db9bc8

Co-authored-by: Loren Yu <[email protected]>

rylew1 mentioned this pull request Nov 8, 2024

Run e2e tests in parallel shards navapbc/template-infra#776

Merged

rylew1 closed this Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sharding to e2e tests #89

Add sharding to e2e tests #89

rylew1 commented Oct 28, 2024 •

edited by github-actions bot

Loading

lorenyu left a comment

acouch commented Oct 29, 2024

acouch commented Oct 29, 2024

rylew1 commented Oct 29, 2024 •

edited

Loading

lorenyu left a comment

lorenyu Nov 5, 2024

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024

lorenyu Nov 6, 2024

lorenyu Nov 5, 2024

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

lorenyu Nov 6, 2024

rylew1 Nov 8, 2024 •

edited

Loading

rylew1 Nov 8, 2024 •

edited

Loading

lorenyu Nov 5, 2024

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

lorenyu Nov 6, 2024 •

edited

Loading

rylew1 Nov 8, 2024 •

edited

Loading

rylew1 commented Nov 6, 2024 •

edited

Loading

lorenyu commented Nov 6, 2024

lorenyu left a comment

	shard: {
	// Total number of shards
	total: parseInt(process.env.TOTAL_SHARDS \|\| '1'),
	// Specifies which shard this job should execute
	current: parseInt(process.env.CURRENT_SHARD \|\| '1'),
	},

Add sharding to e2e tests #89

Add sharding to e2e tests #89

Conversation

rylew1 commented Oct 28, 2024 • edited by github-actions bot Loading

Ticket

Changes

TODO

Demo

Preview environment

lorenyu left a comment

Choose a reason for hiding this comment

acouch commented Oct 29, 2024

acouch commented Oct 29, 2024

rylew1 commented Oct 29, 2024 • edited Loading

lorenyu left a comment

Choose a reason for hiding this comment

lorenyu Nov 5, 2024

Choose a reason for hiding this comment

rylew1 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Nov 6, 2024

Choose a reason for hiding this comment

lorenyu Nov 6, 2024

Choose a reason for hiding this comment

lorenyu Nov 5, 2024

Choose a reason for hiding this comment

rylew1 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

lorenyu Nov 6, 2024

Choose a reason for hiding this comment

rylew1 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

lorenyu Nov 5, 2024

Choose a reason for hiding this comment

rylew1 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

lorenyu Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

rylew1 commented Nov 6, 2024 • edited Loading

lorenyu commented Nov 6, 2024

lorenyu left a comment

Choose a reason for hiding this comment

rylew1 commented Oct 28, 2024 •

edited by github-actions bot

Loading

rylew1 commented Oct 29, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 8, 2024 •

edited

Loading

rylew1 Nov 8, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

rylew1 Nov 6, 2024 •

edited

Loading

lorenyu Nov 6, 2024 •

edited

Loading

rylew1 Nov 8, 2024 •

edited

Loading

rylew1 commented Nov 6, 2024 •

edited

Loading