Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix invalid connect options, when using browserURL #212

Merged
merged 3 commits into from
Mar 13, 2019
Merged

fix invalid connect options, when using browserURL #212

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1296edd
fix invalid connect options, when using browserURL
backbone87 Mar 12, 2019
56bc9a8
harden teardown
backbone87 Mar 12, 2019
8bf3cb2
fix linting
backbone87 Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions packages/jest-environment-puppeteer/src/PuppeteerEnvironment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class PuppeteerEnvironment extends NodeEnvironment {
this.global.browser = await puppeteer.connect({
...config.connect,
...config.launch,
browserURL: undefined,
browserWSEndpoint: wsEndpoint,
})

Expand Down Expand Up @@ -114,16 +115,25 @@ class PuppeteerEnvironment extends NodeEnvironment {
}

async teardown() {
const config = await readConfig()
this.global.page.removeListener('pageerror', handleError)
const { page, context, browser, puppeteerConfig } = this.global

if (config.browserContext === 'incognito') {
await this.global.context.close()
if (page) {
page.removeListener('pageerror', handleError)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@backbone87 backbone87 Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (puppeteerConfig.browserContext === 'incognito') {
if (context) {
await context.close()
}
} else {
await this.global.page.close()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
}

Copy link
Contributor Author

@backbone87 backbone87 Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@backbone87 backbone87 Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (page) {
await page.close()
}
}

await this.global.browser.disconnect()
if (browser) {
await browser.disconnect()
}
}
}

Expand Down