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.

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

fix invalid connect options, when using browserURL #212

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

backbone87
Copy link
Contributor

@backbone87 backbone87 commented Mar 12, 2019

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
Copy link
Contributor Author

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

Copy link

@MacZel MacZel left a comment

Choose a reason for hiding this comment

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

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

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

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.

} 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

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

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.

@backbone87
Copy link
Contributor Author

backbone87 commented Mar 13, 2019

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
Copy link
Member

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.

@gregberge gregberge merged commit 6e483e6 into argos-ci:master Mar 13, 2019
@backbone87
Copy link
Contributor Author

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

@backbone87 backbone87 deleted the fix-connect-browser-url branch March 13, 2019 17:26
@gregberge
Copy link
Member

@backbone87 yes we can

@gregberge
Copy link
Member

Published!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'removeListener' of undefined
3 participants