-
Notifications
You must be signed in to change notification settings - Fork 5.4k
test: Add new test job for dist tests #35906
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
base: main
Are you sure you want to change the base?
Conversation
0767474 to
c9e44ac
Compare
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [c9e44ac]
UI Startup Metrics (1204 ± 63 ms)
Benchmark value 25 exceeds gate value 23 for chrome browserify home mean firstReactRender Benchmark value 16 exceeds gate value 15 for chrome browserify home mean getState Benchmark value 6 exceeds gate value 1 for chrome browserify home mean initialActions Benchmark value 255 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 16 exceeds gate value 1.2 for chrome browserify home p95 initialActions Benchmark value 40 exceeds gate value 29 for chrome webpack home mean getState Benchmark value 9 exceeds gate value 7 for chrome webpack home mean initialActions Benchmark value 80 exceeds gate value 57 for chrome webpack home p95 domInteractive Benchmark value 277 exceeds gate value 195 for chrome webpack home p95 getState Benchmark value 16 exceeds gate value 7 for chrome webpack home p95 initialActions Benchmark value 248 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 36 exceeds gate value 25 for firefox browserify home mean backgroundConnect Benchmark value 27 exceeds gate value 25 for firefox browserify home mean firstReactRender Benchmark value 4 exceeds gate value 1 for firefox browserify home mean initialActions Benchmark value 13 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 224 exceeds gate value 195 for firefox browserify home p95 domInteractive Benchmark value 104 exceeds gate value 70 for firefox browserify home p95 backgroundConnect Benchmark value 11 exceeds gate value 2 for firefox browserify home p95 initialActions Benchmark value 31 exceeds gate value 27 for firefox browserify home p95 setupStore Benchmark value 107 exceeds gate value 100 for firefox webpack home mean domInteractive Benchmark value 31 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 44 exceeds gate value 38 for firefox webpack home mean firstReactRender Benchmark value 3 exceeds gate value 1 for firefox webpack home mean initialActions Benchmark value 1948 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 300 exceeds gate value 156 for firefox webpack home p95 domInteractive Benchmark value 7 exceeds gate value 2 for firefox webpack home p95 initialActions Sum of mean exceeds: 294ms | Sum of p95 exceeds: 786.8ms Sum of all benchmark exceeds: 1080.8ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [1848e92]
UI Startup Metrics (1250 ± 59 ms)
Benchmark value 1083 exceeds gate value 1070 for chrome browserify home mean load Benchmark value 1074 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 255 exceeds gate value 10 for chrome browserify home mean backgroundConnect Benchmark value 27 exceeds gate value 23 for chrome browserify home mean firstReactRender Benchmark value 17 exceeds gate value 15 for chrome browserify home mean getState Benchmark value 5 exceeds gate value 1 for chrome browserify home mean initialActions Benchmark value 278 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 39 exceeds gate value 33 for chrome browserify home p95 getState Benchmark value 15 exceeds gate value 1.2 for chrome browserify home p95 initialActions Benchmark value 19 exceeds gate value 17 for chrome browserify home p95 setupStore Benchmark value 44 exceeds gate value 29 for chrome webpack home mean getState Benchmark value 2461 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 283 exceeds gate value 195 for chrome webpack home p95 getState Benchmark value 11 exceeds gate value 7 for chrome webpack home p95 initialActions Benchmark value 35 exceeds gate value 25 for firefox browserify home mean backgroundConnect Benchmark value 27 exceeds gate value 25 for firefox browserify home mean firstReactRender Benchmark value 6 exceeds gate value 1 for firefox browserify home mean initialActions Benchmark value 13 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 241 exceeds gate value 195 for firefox browserify home p95 domInteractive Benchmark value 97 exceeds gate value 70 for firefox browserify home p95 backgroundConnect Benchmark value 10 exceeds gate value 2 for firefox browserify home p95 initialActions Benchmark value 56 exceeds gate value 27 for firefox browserify home p95 setupStore Benchmark value 30 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 43 exceeds gate value 38 for firefox webpack home mean firstReactRender Benchmark value 3 exceeds gate value 1 for firefox webpack home mean initialActions Benchmark value 272 exceeds gate value 156 for firefox webpack home p95 domInteractive Benchmark value 5 exceeds gate value 2 for firefox webpack home p95 initialActions Sum of mean exceeds: 344ms | Sum of p95 exceeds: 609.8ms Sum of all benchmark exceeds: 953.8ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This comment was marked as resolved.
This comment was marked as resolved.
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [de04e67]
UI Startup Metrics (1229 ± 82 ms)
Benchmark value 25 exceeds gate value 23 for chrome browserify home mean firstReactRender Benchmark value 6 exceeds gate value 1 for chrome browserify home mean initialActions Benchmark value 256 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 14 exceeds gate value 1.2 for chrome browserify home p95 initialActions Benchmark value 19 exceeds gate value 17 for chrome browserify home p95 setupStore Benchmark value 43 exceeds gate value 40 for chrome webpack home mean backgroundConnect Benchmark value 31 exceeds gate value 29 for chrome webpack home mean getState Benchmark value 303 exceeds gate value 90 for chrome webpack home p95 backgroundConnect Benchmark value 278 exceeds gate value 195 for chrome webpack home p95 getState Benchmark value 16 exceeds gate value 7 for chrome webpack home p95 initialActions Benchmark value 242 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 1411 exceeds gate value 1405 for firefox browserify home mean uiStartup Benchmark value 36 exceeds gate value 25 for firefox browserify home mean backgroundConnect Benchmark value 28 exceeds gate value 25 for firefox browserify home mean firstReactRender Benchmark value 4 exceeds gate value 1 for firefox browserify home mean initialActions Benchmark value 11 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 212 exceeds gate value 195 for firefox browserify home p95 domInteractive Benchmark value 90 exceeds gate value 70 for firefox browserify home p95 backgroundConnect Benchmark value 10 exceeds gate value 2 for firefox browserify home p95 initialActions Benchmark value 102 exceeds gate value 100 for firefox webpack home mean domInteractive Benchmark value 31 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 43 exceeds gate value 38 for firefox webpack home mean firstReactRender Benchmark value 6 exceeds gate value 1 for firefox webpack home mean initialActions Benchmark value 267 exceeds gate value 156 for firefox webpack home p95 domInteractive Benchmark value 53 exceeds gate value 50 for firefox webpack home p95 firstReactRender Benchmark value 7 exceeds gate value 2 for firefox webpack home p95 initialActions Sum of mean exceeds: 290ms | Sum of p95 exceeds: 898.8ms Sum of all benchmark exceeds: 1188.8ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| "test:integration": "yarn webpack-cli build --config ./development/webpack/webpack.integration.tests.config.ts && jest --config jest.integration.config.js", | ||
| "test:integration:coverage": "yarn test:integration --coverage", | ||
| "test:e2e:chrome": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts", | ||
| "test:e2e:chrome:dist": "SELENIUM_BROWSER=chrome tsx test/e2e/run-all.ts --dist", |
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.
I generally argue for "we already have too many scripts in package.json, and if you're adding another command that really isn't intended for general use, but will just be run in one place in GitHub Actions, let's keep it out of package.json"
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.
It can be run locally as well, e.g. when debugging an E2E test failure for this job. It's important to have a simple command that can replicate CI tests locally.
| e2e-firefox: | ||
| needs: | ||
| - needs-e2e | ||
| - build-dist-mv2-browserify |
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.
Adding this happens to be a little unfortunate, because you're adding the longest build process as a needs before the longest test suite. So this definitely adds time to the workflow as a whole. Possible solutions include:
- Wait for webpack to completely replace browserify
- Add more shards to
test-e2e-firefox-browserifyandtest-e2e-firefox-flask - Take
test-e2e-firefox-distout ofe2e-firefox.ymland rework how the YML files call each other and are organized, but I think that's not worth it
Edit: hmmmm and then I noticed that test-e2e-firefox-dist is running zero tests....
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.
Is Firefox substantially slower than Chrome to build and/or run tests? I had thought the difference was fairly small
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.
For the build step, it's the dist build that's slower. build-dist-browserify and build-dist-mv2-browserify take the same amount of time. But this needs did not have a dist build in the list before.
For the E2E tests, the test-e2e-firefox-browserify shards take 12-14 minutes, but the test-e2e-firefox-browserify take 9-11 minutes. I have not done any additional investigation as to why it's slower. If we can't make it faster, we could also increase the number of Firefox shards.
| if (process.env.SELENIUM_BROWSER !== 'chrome') { | ||
| // TODO: Get this working on Firefox | ||
| this.skip(); | ||
| } |
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.
We're going to skip ALL of the tests in the dist folder on Firefox? Are more things coming here soon? Because as it is now, a VM will spin up to run test-e2e-firefox-dist, and then it will run zero tests.
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.
These changes were split from this PR, which has a test that would run on Firefox: #31435
I can certainly omit the Firefox workflow until that PR though, so it doesn't spin up a VM that does nothing. Good point.
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.
If it's coming very soon, it matters a little less.
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.
Removed here: 3e45984
I'll re-add it in the PR that adds the first test
## **Description** The script responsible for determining how to split up tests in a given workflow was failing when I tried to add a new workflow. It didn't seem to handle the case where a workflow had zero past runs. It has been updated to use a naive fallback splitting algorithm in this case. [](https://codespaces.new/MetaMask/metamask-extension/pull/35905?quickstart=1) ## **Changelog** CHANGELOG entry: null ## **Related issues** This fixes a CI bug encountered on #31435 ## **Manual testing steps** See this PR for an example of the failures: #35906 See here for a rebased version of that PR on this branch, which works: #35913 ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Howard Braham <howrad@gmail.com>
This comment was marked as spam.
This comment was marked as spam.
Add a new test workflow for E2E tests using `dist` builds. Currently this workflow has just one test suite in it, the vault decryption test. More will be added in later PRs. This was split out from #31435
de04e67 to
3e45984
Compare
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [3e45984]
UI Startup Metrics (1248 ± 77 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Add a new test workflow for E2E tests using
distbuilds. Currently this workflow has just one test suite in it, the vault decryption test. More will be added in later PRs.Changelog
CHANGELOG entry: null
Related issues
This was split out from #31435
Manual testing steps
N/A
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Introduces a dist E2E test mode and workflow, adds a Chrome dist test suite (vault decryption), supporting scripts, and minor CI/.gitignore updates.
test-e2e-chrome-vault-decryptiontotest-e2e-chrome-dist, updatetest-suite-nameandtest-commandtoyarn test:e2e:chrome:distin.github/workflows/e2e-chrome.yml.test-e2e-chrome-dist.build-dist-mv2-browserifyas a need fore2e-firefoxin.github/workflows/main.yml.--distoption totest/e2e/run-all.ts; when set, run tests fromtest/e2e/dist.test:e2e:chrome:distandtest:e2e:firefox:distinpackage.json.test/e2e/dist/vault-decryption-chrome.spec.ts(adjusted imports, skip on non-Chrome)..gitignoreto ignore rootdistonly (/dist).Written by Cursor Bugbot for commit 3e45984. This will update automatically on new commits. Configure here.