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

Add incognito context support #83

Closed
wants to merge 20 commits into from
Closed

Add incognito context support #83

wants to merge 20 commits into from

Conversation

Niceplace
Copy link

Related issue : #81

  • Add config parameter to set incognito mode (default: false)
  • In environment setup, read from config and launch the page in incognito mode

Note: In headful mode, this seems to launch a separate chrome instance per tab as discussed here (apparently there's no performance overhead) : puppeteer/puppeteer#85 (comment)

I checked for unit tests if I needed to add any, it doesn't seem like it but please let me know if anything is wrong, I'll be happy to fix it !

Copy link
Member

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

Several problems:

  • Please put the option outside "launch" and rename it to "browserContext" (to activate "incognito" mode we will put browserContext: 'incognito'
  • Please expose the global context even if not in "incognito" context
  • Please update jest-environment-puppeteer readme and main readme about the new global context (don't forget ESLint section), the browserContext option and about a recipe for using "incognito" context and its advantages

this.global.context = await this.global.browser.createIncognitoBrowserContext()
// Create a new page in a pristine context.
this.global.page = await this.global.context.newPage()
} else if(config.launch.browserContext === 'default') {
Copy link
Author

Choose a reason for hiding this comment

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

Are you OK with explicitely checking this or you would rather just have else ? I would like to avoid allowing users to specify anything else than default or incognito

@Niceplace Niceplace changed the title Add incognito context support [WIP] - Add incognito context support Jun 21, 2018
@Niceplace
Copy link
Author

I'm going to need to do a bit more testing before I feel comfortable with this but please let me know if I'm on the right track with respect to your previous comments ! :)

@@ -60,6 +60,22 @@ it('should fill an input', async () => {
})
```

### `global.context`

**Note: Requires puppeteer >= 1.5.0**
Copy link
Contributor

Choose a reason for hiding this comment

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

* https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#browserbrowsercontexts
*/
this.global.context = await this.global.browser.browserContexts()[0]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

else {
    throw new Error(`browserContext should be either 'incognito' or 'default'. Received '${config.browserContext}' ')
}

@Niceplace Niceplace changed the title [WIP] - Add incognito context support Add incognito context support Jun 21, 2018
@Niceplace
Copy link
Author

@neoziro I'm ready for a second pass :D !


It is possible to set the browser context inside the config `jest-puppeteer-config.js` in the root of this project. The context will always be exposed via the global `context` object the same way `page` and `browser` are exposed.

Accepted values:
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right place for it, see below ### jest-puppeteer.config.js

Copy link
Author

Choose a reason for hiding this comment

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

Will fix soon ! Thanks for the feedback :) !

*/
this.global.context = await this.global.browser.browserContexts()[0]
} else {
throw new Error(`browserContext should be either 'incognito' or 'default'. Received '${config.browserContext}'`)
Copy link
Member

Choose a reason for hiding this comment

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

It is actually a breaking change because there is no default value specified and all users will have an error.

Please consider it as default if not specified.

Copy link
Author

Choose a reason for hiding this comment

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

Will fix soon ! Thanks for the feedback :) !

Copy link
Author

Choose a reason for hiding this comment

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

I changed my implementation a bit, it felt easier to manage with a default value. If you think its inefficient please let me know I'll try to find a better solution.

@gregberge
Copy link
Member

@Niceplace Thanks for your work, just a few comments.

@Niceplace
Copy link
Author

Niceplace commented Jun 29, 2018

@neoziro We have a problem :(

There seems to be some conflicts with expect-puppeteer, more specifically when we use expect(page).toMatchElement

I keep getting errors like this, "JSHandles can be evaluated only in the context they were created!"
We narrowed it down to this possible piece of code (packages/expect-puppeteer/src/matchers/toMatchElement.js) but some guidance would be appreciated. I do not know the extent of the impact, it could be in other functions / files as well

I think that when the executionContext is fetched, there should be some logic to check if we are in incognito mode ? I'm not really sure ...

export const getContext = async (instance, pageFunction) => {
  const type = getPuppeteerType(instance)
  switch (type) {
    case 'Page':
      return {
        page: instance,
        handle: await instance.evaluateHandle(pageFunction),
      }
    case 'ElementHandle': {
      const executionContext = await instance.executionContext()
      return {
        page: await executionContext.frame(),
        handle: instance,
      }
    }
    default:
      throw new Error(`${type} is not implemented`)
  }
}

@Niceplace Niceplace closed this Jul 23, 2018
@gregberge
Copy link
Member

@Niceplace why closed?

@Niceplace
Copy link
Author

Niceplace commented Jul 23, 2018

@neoziro Hi Greg,

I'm not sure if this is the right way of doing things but at this point I don't feel I have the necessary abilities needed to finish this pull request, as mentioned in my previous message, I'm pretty much blocked and I'm not sure how to move forward from here.

If you want me to re-open it I can but someone else would need to take over.

Sorry about that. :-/

@gregberge
Copy link
Member

@Niceplace ok thanks!

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.

3 participants