-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
fix: Add describeChromeOnly
helper and use to exclude multichain API e2e tests from firefox e2e test runs
#30937
base: main
Are you sure you want to change the base?
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. |
… tests from firefox e2e test runs
e410908
to
330aa28
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.
Pull Request Overview
This PR adds a helper function, describeChromeOnly, to conditionally run or skip multichain API end-to-end tests based on the SELENIUM_BROWSER environment variable. Key changes include:
- Adding and exporting the describeChromeOnly helper in testHelpers.ts with appropriate JSDoc documentation.
- Updating multiple multichain API test files to replace describe with describeChromeOnly so that tests are executed only in Chrome.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/e2e/flask/multichain-api/testHelpers.ts | Added and documented the describeChromeOnly helper function |
test/e2e/flask/multichain-api/wallet_revokeSession.spec.ts | Replaced describe with describeChromeOnly for conditional test execution |
test/e2e/flask/multichain-api/wallet_notify.spec.ts | Replaced describe with describeChromeOnly for conditional test execution |
test/e2e/flask/multichain-api/wallet_sessionChanged.spec.ts | Replaced describe with describeChromeOnly for conditional test execution |
test/e2e/flask/multichain-api/wallet_createSession.spec.ts | Replaced describe with describeChromeOnly and updated test descriptions |
test/e2e/flask/multichain-api/wallet_invokeMethod.spec.ts | Replaced describe with describeChromeOnly for conditional test execution |
test/e2e/flask/multichain-api/wallet_getSession.spec.ts | Replaced describe with describeChromeOnly for conditional test execution |
return process.env.SELENIUM_BROWSER === 'chrome' | ||
? describe(description, callback) | ||
: describe.skip(description, callback); | ||
}; |
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 need a Firefox only version of this at some point. Would it make sense to make this helper accept a browser: Browser
arg instead of making it chrome specific here? Then it could be used something like describeOnlyBrowser(Browser.CHROME)
describeOnlyBrowser(Browser.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.
Wouldn't we just stop using this describeOnly
helper altogether once Multichain is supported on 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.
Ahh good point. Firefox will have a different entrypoint into the Multichain API, but that'll be handled at the test dapp layer, not the wallet which is what I was originally thinking.
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.
Its possible we'll use different tests for firefox? Probably not though we'd probably switch the transport in the test based on the environment...
I'll just make this change for now since I'm in there fixing the lint issue and will need re-review anyways
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.
done here: 101d29f
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.
LGTM 🚀
describeChromeOnly
helper and use to exclude multichain API e2e tests from firefox e2e test runsdescribeChromeOnly
helper and use to exclude multichain API e2e tests from firefox e2e test runs
All of the multichain API tests are currently failing on firefox in CI. This is expected because the Multichain API is not currently supported on firefox. This PR adds a
describeChromeOnly
helper function to conditionally skip these tests in non-chrome environmentsPre-merge reviewer checklist