-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: CircleCI is now storing builds-test and builds-test-flask #20749
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [12608d2]
Page Load Metrics (1554 ± 32 ms)
Bundle size diffs
|
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## develop #20749 +/- ##
========================================
Coverage 68.25% 68.25%
========================================
Files 1006 1006
Lines 40189 40189
Branches 10743 10743
========================================
Hits 27429 27429
Misses 12760 12760 ☔ View full report in Codecov by Sentry. |
Able to run individual e2e locally with Firefox v117 on Mac Ventura 13.5.1 Chrome is producing a version error:
Both links for |
Builds ready [12608d2]
Page Load Metrics (1607 ± 49 ms)
Bundle size diffs
|
Still getting this:
when trying to download test flask builds from #20749 (comment) |
a00dbbe
to
518d4f6
Compare
Builds ready [518d4f6]
Page Load Metrics (1776 ± 77 ms)
Bundle size diffs
|
Builds ready [518d4f6]
Page Load Metrics (1776 ± 77 ms)
Bundle size diffs
|
518d4f6
to
0f6e6e6
Compare
Builds ready [0f6e6e6]
Page Load Metrics (1656 ± 85 ms)
Bundle size diffs
|
Builds ready [0f6e6e6]
Page Load Metrics (1610 ± 61 ms)
Bundle size diffs
|
Looks good, the missing builds are now present in the latest #20749 (comment) |
0f6e6e6
to
11ec327
Compare
11ec327
to
64ac3be
Compare
Builds ready [4b496e5]
Page Load Metrics (1405 ± 26 ms)
Bundle size diffs
|
@HowardBraham I think i'm not understanding the benefit/reason for storing the test builds? is it just to make them available for pulling for testing locally when the PR passes? Sorry if I'm being dense here. |
@brad-decker The benefit is to save a little time for QA and developers, when they have to write, modify, or debug local E2E tests. The However, I think that extra fanciness I did with |
4b496e5
to
b8d9fa4
Compare
Builds ready [b8d9fa4]
Page Load Metrics (1602 ± 39 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets update the PR description details to reflect the updated approach now that save_cache is no longer included.
There are multiple possible solutions to this, but the one I chose was:
- save_cache:
key: builds-test-flask-{{ .Environment.CIRCLE_SHA1 }}
paths:
- builds-test-flask
Explanation
The intent of this PR is very similar to #20783. We want to save a little time for QA and developers, when they have to write, modify, or debug local E2E tests. The
builds-test
and thebuilds-test-flask
can be downloaded from the PR instead of locally generated. It also ensures that your local E2E is running the same code as the CircleCI E2E.The first version of this PR had to do some acrobatics with
save_cache
becauseprep-build-test-flask
was not connected tojobs-publish-prerelease
. But now they are connected, so this PR is now more straightforward.Screenshots/Screencaps
Before
After
Manual Testing Steps
"description": "testing build from git id: xxxx",
and
"name": "MetaMask Main lavamoat snow",
or"name": "MetaMask Flask lavamoat snow",
yarn test:e2e:single
, make sure they run E2E tests properlyThe QA Engineer Mike Berardi was already testing this as I was developing it.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.