-
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/add mm e2e mmi build #20931
Feat/add mm e2e mmi build #20931
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. |
No top level dependency changes detected. Learn more about Socket for GitHub ↗︎ |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
aa1d782
to
f9f4ae0
Compare
Builds ready [aa1d782]
Page Load Metrics (1877 ± 96 ms)
Bundle size diffs
|
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## develop #20931 +/- ##
========================================
Coverage 68.36% 68.36%
========================================
Files 1006 1006
Lines 40262 40262
Branches 10763 10763
========================================
Hits 27522 27522
Misses 12740 12740 ☔ View full report in Codecov by Sentry. |
Builds ready [f9f4ae0]
Page Load Metrics (1564 ± 35 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.
💪🏼
f9f4ae0
to
b46600b
Compare
Builds ready [b46600b]
Page Load Metrics (1551 ± 78 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.
@PeterYinusa you should probably review this and see if we should include some way of documenting the @no-mmi
flag in our E2E documents. Blocking for review but overall the code looks good for me.
test/e2e/run-e2e-test.js
Outdated
if (mmi) { | ||
extraArgs.push('-g', '@no-mmi', '-i'); | ||
} | ||
|
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.
Could you add inline documentation here for what these flags do (grep/invert) and explain the @no-mmi tag on some of the tests
Thanks for the heads up @brad-decker, this seems in line with the approach discussed with @racitores. |
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.
Looks great, please add documentation for contributors
43fd3db
b46600b
to
43fd3db
Compare
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.
🚀
Builds ready [43fd3db]
Page Load Metrics (1526 ± 51 ms)
Bundle size diffs
|
Builds ready [5255fdf]
Page Load Metrics (1513 ± 46 ms)
Bundle size diffs
|
Explanation
Currently MMI extension is assuming that all general/core MM extension features should work under MMI but this is not been tested in the pipeline.
This PR adds Metamask e2e tests run in the circle ci pipeline for a MMI build generated extension. There are a set failing tests marked as
@no-mmi
so that when running the tests withyarn test:e2e:chrome:mmi
will doSELENIUM_BROWSER=chrome node test/e2e/run-all.js --mmi
passing --mmi as param.This is a first step to improve MMI test coverage and regression. Next steps will be reviewed and updated tests if required
Screenshots/Screencaps
Before
After
Manual Testing Steps
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.