-
Notifications
You must be signed in to change notification settings - Fork 289
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: support "browser" config option #220
Conversation
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.
Thanks @aslushnikov! It is great, but we have to remove peer dependencies.
@@ -28,6 +27,7 @@ class PuppeteerEnvironment extends NodeEnvironment { | |||
|
|||
async setup() { | |||
const config = await readConfig() | |||
const puppeteer = await getPuppeteer(config) |
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.
No need of await
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.
|
||
let browser | ||
|
||
export async function setup(jestConfig = {}) { | ||
const config = await readConfig() | ||
const puppeteer = await getPuppeteer(config) |
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.
No need of await.
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.
31dd564
to
c5ea20b
Compare
@neoziro thanks for reviewing! I rebaselined atop of master and dropped the peer dependences. Is there anything else I should do? Looking forward for this to be merged! :-) |
Just to clarify, this would mean the end user will manually need to install |
@gidztech this is already the case for Puppeteer. I added a small note to the |
return require('puppeteer-firefox') | ||
default: | ||
throw new Error( | ||
`!!env variable JEST_PUPPETEER_BROWSER is given unsupported value: ${browser}`, |
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 this message is not good. I don't see any JEST_PUPPETEER_BROWSER
variable elsewhere.
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.
Fixed.
@xiaoyuhen if you have time, I would be glad if you could test it. If it is OK for you, could you please tell me then I will merge and release a new version (also included your latest fix). |
yeah, Will check before this Saturday. |
Because of the Great Firewall of 🇨🇳, I can't download Firefox at home. Chromium is ok because I can use the mirrors of Chromium. Firefox dont have any mirror on 🇨🇳 Maybe I need to update my VPN, the check may be delayed until this Sunday. 😢 |
@neoziro I create a firefox example, it works great. I will take a PR after the new version released. @aslushnikov Thanks for your great job! |
Thanks @xiaoyuhen and @neoziro for your time! |
I don't see any mention of Firefox or # for jest 22~23
npm install --save-dev jest-puppeteer@3.9.0 puppeteer jest
# for jest 24+
npm install --save-dev jest-puppeteer puppeteer jest It would be great to clarify that it's also possible to do npm install --save-dev jest-puppeteer puppeteer-firefox jest if you opt in to use Firefox. I'm wondering whether this is something that could be handled in |
Could someone submit a PR to document it? |
This is great news guys!~ |
This patch adds a new option to
jest-puppeteer.config.js
-browser
. The option might be either'chromium'
or'firefox'
(case-insensetive) and defaults to'chromium'
. Depending on this option, eitherpuppeteer
orpuppeteer-firefox
will be used to drive test execution.Test plan
There's a unit test in the PR. Let me know if/how I should add an integration test.
Fixes #171