Proper WS-Endpoint via pid management #115
Conversation
Allows multiple Jest processes to connect to their own chromium instances
pid here should be the jest process itself
This will work for child process spawned by jest and multiple jest instances
ghost
mentioned this pull request
* Update PuppeteerEnvironment.js Travis will like complain about e not being used in which case I will use a finally loop that checks if Ws endpoint is defined * Update global.js global doesnt know if it has children yet so we can't support pid if ppid isn't avaliable * Update PuppeteerEnvironment.js * Update PuppeteerEnvironment.js ternary is unnecessary in the catch because if ppid does not exist, then wsendpoint for both global and puppeteerEnvironment have defaulted to the old method of accessing the shared ws-endpoint file. This still presents the race, but works with all use cases
|
I'll continue to look into this, because having separate Jest instances rather than letting Jest manage child processes is a valid use case. |
| wsEndpoint = fs.default.readFileSync(WS_ENDPOINT_PATH + ((process.ppid) ? process.pid : ""), 'utf8') | ||
| } catch (e) { | ||
| wsEndpoint = fs.default.readFileSync(WS_ENDPOINT_PATH + process.ppid, 'utf8') | ||
| } |
There was a problem hiding this comment.
I don't understand the logic here. If there is a ppid, then we use pid but if there is an error then we fallback using ppid??
There was a problem hiding this comment.
The correct logic should be to fallback to the using the old name without the pid added if ppid is not defined/supported. Then attempt to read file using pid and if that fails ppid. I see no reason why do it in a specific order, I was trying to use the same pattern as in the global,js.
| @@ -19,7 +19,7 @@ export async function setup() { | |||
| const config = await readConfig() | |||
| browser = await puppeteer.launch(config.launch) | |||
| mkdirp.sync(DIR) | |||
| fs.writeFileSync(WS_ENDPOINT_PATH, browser.wsEndpoint()) | |||
| fs.writeFileSync(WS_ENDPOINT_PATH + ((process.ppid) ? process.pid : ""), browser.wsEndpoint()) | |||
There was a problem hiding this comment.
In global mode we are in ppid? No? I really don't understand the logic.
There was a problem hiding this comment.
We check for ppid in global mode because if it is not defined the user's Node doesn't support it and we are falling back to the old ws-endpoint naming scheme. We only ever use pid in global mode because it is the parent process if we have multiple test files or the same process for a single test file.
|
Also tests are all broken, meaning the logic is not correct. |
|
#138 is easier |
Requires support for process.ppid which is in node v 9.2.0
Global will always assign ws-endpoint via its pid, PuppeteerEnvironment will try to find the ws-endpoint via its pid first (valid in the case of a single test file), and will use its ppid instead of that fails.
PPID of child process PupeteerEnvironment should match PID of parent process Global.