Set higher default and allow configuring puppeteer protocol timeout #58
Conversation
|
@nachocodoner @jankapunkt @StorytellerCZ Will this require a backport to work in Meteor 2 or is the latest version compatible with both Meteor 2 and 3? |
|
@make-github-pseudonymous-again it's intended to work with both but please let me know if you encounter issues. |
|
So, did you stop to see the error after tweaking this timeout? If that is ok, it feels good to me to merge, just tweaking a timeout doesn't seem problematic. |
|
@nachocodoner If that question is for me, I did not test anything. Just assumed it would work. |
|
@nachocodoner How could I test this before merging it? I can reproduce this bug locally. |
|
@make-github-pseudonymous-again create a |
|
@make-github-pseudonymous-again cab there be a situation where users might want to set |
|
The It seems the puppeteer driver implemented by Meteor is making puppeteer wait for a message response for the entire duration of the scheduled tests, which can easily reach in the minutes, especially for client app tests. @jankapunkt Default is 3 minutes. See: |
browser/puppeteer.js
Outdated
| @@ -31,6 +33,7 @@ export default function startPuppeteer({ | |||
| const browser = await puppeteer.launch({ | |||
| args: ['--no-sandbox', '--disable-setuid-sandbox'], | |||
| headless: 'new', | |||
| protocolTimeout: Number.parseInt(process.env.PUPPETEER_PROTOCOL_TIMEOUT, 10) || TWENTY_DAYS, | |||
There was a problem hiding this comment.
@make-github-pseudonymous-again
protocolTimeout = 0 would be interpreted as "no timeout" AFAICT. See
In that case, your current code would never allow such, because 0 would be evaluated as falsy and thus always fall back to TWENTY_DAYS.
To also support 0 as value, it should be
protocolTimeout: process.env.PUPPETEER_PROTOCOL_TIMEOUT
? Number.parseInt(process.env.PUPPETEER_PROTOCOL_TIMEOUT, 10)
: TWENTY_DAYS,
There was a problem hiding this comment.
Makes sense.
There was a problem hiding this comment.
On the other hand, the intention behind twenty days here (copied from Nightmare's default) is perhaps to disable the timeout entirely. So should we instead have the default be 0 instead of TWENTY_DAYS?
There was a problem hiding this comment.
@StorytellerCZ what do you think here?
There was a problem hiding this comment.
I think 0 should disable timeout, I think that is the intention with this. If nothing is passed then use default.
There was a problem hiding this comment.
OK. I'll update this PR to set "no timeout" as the default, which means passing protocolTimeout = 0 to puppeteer.
There was a problem hiding this comment.
That is not what I meant. From my understanding the default right now is TWENTY_DAYS, keep that if there is no input. If the input is something different, including 0, then set it to that. So if the input is 0, then timeout is zero = disabled.
There was a problem hiding this comment.
OK. Had misunderstood. Will apply @jankapunkt's suggestion instead then.
There was a problem hiding this comment.
Suggestion applied.
There was a problem hiding this comment.
@StorytellerCZ Waiting for your input.
@StorytellerCZ Just tested this. Works: setting a small timeout (5000ms) crashes mid-tests, setting a large timeout lets all tests run to completion. Setting a really small timeout (5ms) crashes during puppeteer setup. Just out of curiosity, does it matter what name I give to the package under Next step is to act on #58 (comment), and then I think it will be ready to merge. |
|
The folder name is irrelevant as long as it contains the package.js file with proper package config |
make-github-pseudonymous-again
force-pushed
the
feat/allow-configuring-puppeteer-protocol-timeout
branch
from
7229082 to
093e240
Compare
make-github-pseudonymous-again
force-pushed
the
feat/allow-configuring-puppeteer-protocol-timeout
branch
from
093e240 to
88b1795
Compare
I believe this should fix Meteor-Community-Packages/meteor-mocha#104.