fix invalid connect options, when using browserURL

[backbone87]

Summary

Specifying both options browserURL and browserWSEndpoint is invalid when calling connect. Since PuppeteerEnvironment always gets a WS endpoint prepared from global setup, we must unset the browserURL option.

Edit: Since this problem also exists in v3, it would be nice to backport this, since v4 does not support jest v23 anymore

Edit2: This most likely fixes #200, at least my occurence of it. The real problem is that setup exceptions get swallowed, if teardown also throws. Guessing that the jest code looks something like this:

try {
  env.setup();
  test();
} catch(e) {
  env.teardown();
  throw e;
}

So the actual problem (setup failing) gets shadowed by the teardown fragility not being able to do its work on a dirty env.

[backbone87]

i have added some hardening into teardown, so it can handle most cases of dirty env

[MacZel]

Anyway I'm not sure if it' is an exact solution to the issue.

[MacZel]

You are checking if (page) on line L120 and L129. If it's supposed to be run only if page is true you can combine those two if blocks into one. Logic would stay the same.

[backbone87]

This should be done always, if page is set. Independant of what we want to close.

[MacZel]

tbh logic would stay the same if you will change lines L120-L132 to:

if (puppeteerConfig.browserContext === 'incognito' && context) {
  await context.close()
} else if (page) {
  page.removeListener('pageerror', handleError)
  await page.close()
}

[backbone87]

This code is different: If browserContent === 'incognito' && context, then page.removeListener, wont be called, but that call should only depend on existance of page.
So we must move the page.removeListener call into its own block, leaving us with this:

if (puppeteerConfig.browserContext === 'incognito' && context) {
  await context.close()
} else if (page) {
  await page.close()
}

Normally this also changes the logic of the original block i posted, but since page and context depend on each other, so when browserContent === 'incognito' && context === undefined (because error in setup), it must be page === undefined, because there cant be a page without context.

[backbone87]

To expand on this: You must have knowledge about the dependency between context & page, to correctly understand the minified if/else you propose, which makes it harder to reason about what happens.

[backbone87]

i have fixed the linting error, but imho in this special case, it decreases the readability of the code, because the inner if is independant to the outer if/else.

[gregberge]

Hello @backbone87, thanks for the fix, it looks better than before and it should fix the bug. I will merge it and release a new version.

[backbone87]

@neoziro can we have a backport to v3 for this?

[gregberge]

@backbone87 yes we can

[gregberge]

Published!