-
Notifications
You must be signed in to change notification settings - Fork 639
Use Vitest for browser tests #3236
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
Conversation
|
👍 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. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
e03fd92 to
5ce17c2
Compare
snaps-execution-environments browser tests8f5175c to
d061a4b
Compare
|
@SocketSecurity ignore npm/istanbul-lib-report@3.0.1 New author is ok. |
| }; | ||
| }, {}] | ||
| }, {}, [1]); | ||
| //# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJyIiwiZSIsIm4iLCJ0IiwibyIsImkiLCJmIiwiYyIsInJlcXVpcmUiLCJ1IiwiYSIsIkVycm9yIiwiY29kZSIsInAiLCJleHBvcnRzIiwiY2FsbCIsImxlbmd0aCIsIm1vZHVsZSIsIm9uUnBjUmVxdWVzdCIsInJlcXVlc3QiLCJjb25zb2xlIiwibG9nIiwibWV0aG9kIiwiaWQiXSwic291cmNlcyI6WyIuLi8uLi9ub2RlX21vZHVsZXMvYnJvd3Nlci1wYWNrL19wcmVsdWRlLmpzIiwiX3N0cmVhbV8wLmpzIl0sInNvdXJjZXNDb250ZW50IjpbIihmdW5jdGlvbigpe2Z1bmN0aW9uIHIoZSxuLHQpe2Z1bmN0aW9uIG8oaSxmKXtpZighbltpXSl7aWYoIWVbaV0pe3ZhciBjPVwiZnVuY3Rpb25cIj09dHlwZW9mIHJlcXVpcmUmJnJlcXVpcmU7aWYoIWYmJmMpcmV0dXJuIGMoaSwhMCk7aWYodSlyZXR1cm4gdShpLCEwKTt2YXIgYT1uZXcgRXJyb3IoXCJDYW5ub3QgZmluZCBtb2R1bGUgJ1wiK2krXCInXCIpO3Rocm93IGEuY29kZT1cIk1PRFVMRV9OT1RfRk9VTkRcIixhfXZhciBwPW5baV09e2V4cG9ydHM6e319O2VbaV1bMF0uY2FsbChwLmV4cG9ydHMsZnVuY3Rpb24ocil7dmFyIG49ZVtpXVsxXVtyXTtyZXR1cm4gbyhufHxyKX0scCxwLmV4cG9ydHMscixlLG4sdCl9cmV0dXJuIG5baV0uZXhwb3J0c31mb3IodmFyIHU9XCJmdW5jdGlvblwiPT10eXBlb2YgcmVxdWlyZSYmcmVxdWlyZSxpPTA7aTx0Lmxlbmd0aDtpKyspbyh0W2ldKTtyZXR1cm4gb31yZXR1cm4gcn0pKCkiLCJcbiAgbW9kdWxlLmV4cG9ydHMub25ScGNSZXF1ZXN0ID0gKHsgcmVxdWVzdCB9KSA9PiB7XG4gICAgY29uc29sZS5sb2coXCJIZWxsbywgd29ybGQhXCIpO1xuXG4gICAgY29uc3QgeyBtZXRob2QsIGlkIH0gPSByZXF1ZXN0O1xuICAgIHJldHVybiBtZXRob2QgKyBpZDtcbiAgfTtcbiJdLCJtYXBwaW5ncyI6IkFBQUE7RUFBQSxTQUFBQSxFQUFBQyxDQUFBLEVBQUFDLENBQUEsRUFBQUMsQ0FBQTtJQUFBLFNBQUFDLEVBQUFDLENBQUEsRUFBQUMsQ0FBQTtNQUFBLEtBQUFKLENBQUEsQ0FBQUcsQ0FBQTtRQUFBLEtBQUFKLENBQUEsQ0FBQUksQ0FBQTtVQUFBLElBQUFFLENBQUEsd0JBQUFDLE9BQUEsSUFBQUEsT0FBQTtVQUFBLEtBQUFGLENBQUEsSUFBQUMsQ0FBQSxTQUFBQSxDQUFBLENBQUFGLENBQUE7VUFBQSxJQUFBSSxDQUFBLFNBQUFBLENBQUEsQ0FBQUosQ0FBQTtVQUFBLElBQUFLLENBQUEsT0FBQUMsS0FBQSwwQkFBQU4sQ0FBQTtVQUFBLE1BQUFLLENBQUEsQ0FBQUUsSUFBQSx1QkFBQUYsQ0FBQTtRQUFBO1FBQUEsSUFBQUcsQ0FBQSxHQUFBWCxDQUFBLENBQUFHLENBQUE7VUFBQVMsT0FBQTtRQUFBO1FBQUFiLENBQUEsQ0FBQUksQ0FBQSxLQUFBVSxJQUFBLENBQUFGLENBQUEsQ0FBQUMsT0FBQSxZQUFBZCxDQUFBO1VBQUEsSUFBQUUsQ0FBQSxHQUFBRCxDQUFBLENBQUFJLENBQUEsS0FBQUwsQ0FBQTtVQUFBLE9BQUFJLENBQUEsQ0FBQUYsQ0FBQSxJQUFBRixDQUFBO1FBQUEsR0FBQWEsQ0FBQSxFQUFBQSxDQUFBLENBQUFDLE9BQUEsRUFBQWQsQ0FBQSxFQUFBQyxDQUFBLEVBQUFDLENBQUEsRUFBQUMsQ0FBQTtNQUFBO01BQUEsT0FBQUQsQ0FBQSxDQUFBRyxDQUFBLEVBQUFTLE9BQUE7SUFBQTtJQUFBLFNBQUFMLENBQUEsd0JBQUFELE9BQUEsSUFBQUEsT0FBQSxFQUFBSCxDQUFBLE1BQUFBLENBQUEsR0FBQUYsQ0FBQSxDQUFBYSxNQUFBLEVBQUFYLENBQUEsSUFBQUQsQ0FBQSxDQUFBRCxDQUFBLENBQUFFLENBQUE7SUFBQSxPQUFBRCxDQUFBO0VBQUE7RUFBQSxPQUFBSixDQUFBO0FBQUE7RUFBQSxjQUFBUSxPQUFBLEVBQUFTLE1BQUEsRUFBQUgsT0FBQTtJQ0NBRyxNQUFBLENBQUFILE9BQUEsQ0FBQUksWUFBQTtNQUFBQztJQUFBO01BQ0FDLE9BQUEsQ0FBQUMsR0FBQTtNQUVBO1FBQUFDLE1BQUE7UUFBQUM7TUFBQSxJQUFBSixPQUFBO01BQ0EsT0FBQUcsTUFBQSxHQUFBQyxFQUFBO0lBQ0EifQ==" |
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.
This changed because of a bump in dependencies responsible for source maps.
| expect.assertions(4); | ||
| expect.assertions(5); |
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.
This number was wrong initially, it seems WDIO just ignores it.
| const snap = top.querySelector('iframe[data-testid="snaps-iframe"]'); | ||
| // Parent is the iframe created by Vitest containing the Snap iframe | ||
| // and this page as a sibling iframe. We try to access the Snap iframe | ||
| // from the parent iframe. | ||
| const snap = parent.document.querySelector( | ||
| 'iframe[data-testid="snaps-iframe"]', | ||
| ); | ||
| if (!snap) { | ||
| window.parent.postMessage('Failed to find snap iframe.', '*'); | ||
| window.parent.postMessage('Failed to find Snap iframe.', '*'); | ||
| return; | ||
| } | ||
|
|
||
| // We try to access the snap iframe document. If this works, it means | ||
| // that the iframe is not sandboxed. Otherwise it will throw an error. | ||
| snap.document; | ||
|
|
||
| window.parent.postMessage( | ||
| 'Same origin frames access worked which might suggest sandboxing failed.', | ||
| '*', | ||
| ); |
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.
This test was wrong in the first place. The iframe was served from a different origin, and trying to access top.querySelector resulted in:
SecurityError: Failed to read a named property 'querySelector' from 'Window': Blocked a frame with origin "http://localhost:4567/" from accessing a cross-origin frame.
So the snap.document was never reached, and top.querySelector isn't even a function even if it was accessible. 😅
I confirmed that the new test properly tests the sandboxing behaviour, as it's now served from the same origin. Removing the sandbox property from the iframe now results in a test failure.
| @@ -0,0 +1 @@ | |||
| export const MOCK_BLOCK_NUMBER = '0xa70e75'; | |||
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.
Moved this to a separate file because Vitest was trying to import Node.js dependencies through ./test-utils/index.ts.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3236 +/- ##
==========================================
- Coverage 94.88% 94.72% -0.16%
==========================================
Files 518 518
Lines 11357 11932 +575
Branches 1752 1831 +79
==========================================
+ Hits 10776 11303 +527
- Misses 581 629 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // @ts-expect-error: Incompatibility between Vite versions. | ||
| NodeModulesPolyfillPlugin(), | ||
|
|
||
| // @ts-expect-error: Incompatibility between Vite versions. | ||
| NodeGlobalsPolyfillPlugin({ | ||
| buffer: true, | ||
| }), |
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 plugins seem to be unmaintained, but swapping them out for vite-plugin-node-polyfills like I did in snaps-utils and snaps-controllers doesn't seem to work with SES (because the process is incompatible?). I haven't looked into it much further, but for now we can keep using these here.
ea808c9 to
ff90098
Compare
| "@metamask/*": ["../*/src"] | ||
| }, | ||
| "resolveJsonModule": true, | ||
| "skipLibCheck": true, |
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.
Vitest seems to import non-existing types from Vite. This seems like the easiest way to avoid it, and actually speeds up building too.
|
@SocketSecurity ignore npm/mute-stream@2.0.0 New author is ok. @SocketSecurity ignore npm/tinypool@1.0.2 Shell access fine. |
| * | ||
| * @returns The summary of the merged coverage. | ||
| */ | ||
| // TODO: Check if Vite's coverage is actually inaccurate. |
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 think it's worth looking into in this PR, especially if we can avoid unnecessary computation.
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.
Seems like either Jest or Vite is inaccurate. I checked the iframe execution service for example: Vite shows it as completely covered, but after merging with Jest, one line is not covered.
The actual computation done here isn't really significant anyway though, and long term we can likely remove this once we migrate all tests to Vitest.
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 turns out that we need this logic, then we should probably make this logic re-usable since we also use it in snaps-controllers
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 use this in snaps-controllers, snaps-execution-environments, and snaps-utils. I agree it could be reusable, but I would prefer to do that in a separate PR.
| import 'ses'; | ||
|
|
||
| describe('BaseSnapExecutor', () => { | ||
| before(() => { |
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.
Why isn't this needed anymore?
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'm not sure why it was necessary in the first place anymore. It works without it now.
packages/snaps-execution-environments/src/common/endowments/endowments.test.browser.ts
Show resolved
Hide resolved
packages/snaps-execution-environments/src/proxy/ProxySnapExecutor.test.browser.ts
Outdated
Show resolved
Hide resolved
...es/snaps-execution-environments/src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts
Outdated
Show resolved
Hide resolved
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.
Maybe we can reuse this config as well?
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.
Long term I want to start migrating more stuff to Vitest, at which point we can use workspace configs. For now I would prefer to keep it like this.
No longer a POC, i think? 😄 |
79723a1 to
45b124c
Compare
|
@SocketSecurity ignore npm/playwright-core@1.49.1 Network access and shell access are fine. |
| uses: MetaMask/action-retry-command@v1 | ||
| with: | ||
| command: yarn install-chrome | ||
| command: yarn playwright install --with-deps --only-shell chromium firefox |
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.
How will this work locally? Do you need to run any commands besides yarn and yarn test to run browser tests locally?
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.
Yes, Vitest will tell you to run yarn playwright install on the first time running yarn test.
| 'src/**/test-utils/**', | ||
| 'src/**/__mocks__/**', | ||
| 'src/**/node-js/**', | ||
| 'src/services/node.ts', |
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.
What's the reasoning for this and the index.ts exclude?
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 caused the coverage merge script to break because Vitest had coverage for these files, but Jest didn't. We call jestMap.fileCoverageFor(file), which throws if the file doesn't exist.
This migrates all browser tests which were previously run with WebdriverIO to Vitest. WebdriverIO had several issues like flakiness, lack of certain APIs since it uses Mocha, and performance in CI. Vitest does not seem to have these issues.
Long term we can start migrating all tests to Vitest instead of Jest. The APIs are largely the same between the two, so the migration should be relatively straightforward, and it simplifies some of our custom logic like merging coverage.