-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(ct): recreate context on option change #14243
Conversation
const hash = { | ||
acceptDownloads: context.acceptDownloads, | ||
bypassCSP: context.bypassCSP, | ||
colorScheme: context.colorScheme, | ||
extraHTTPHeaders: context.extraHTTPHeaders, | ||
forcedColors: context.forcedColors, | ||
geolocation: context.geolocation, | ||
hasTouch: context.hasTouch, | ||
httpCredentials: context.httpCredentials, | ||
ignoreHTTPSErrors: context.ignoreHTTPSErrors, | ||
isMobile: context.isMobile, | ||
javaScriptEnabled: context.javaScriptEnabled, | ||
locale: context.locale, | ||
offline: context.offline, | ||
permissions: context.permissions, | ||
proxy: context.proxy, | ||
storageState: context.storageState, | ||
timezoneId: context.timezoneId, | ||
userAgent: context.userAgent, | ||
deviceScaleFactor: context.deviceScaleFactor, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry we will add a context option and never remember to add here making for a gnarly debug. (Especially given most of these are upstream browser options, they will rarely change and we will forget to edit here.)
Can we change this to be defensive so by default new context options will create a cache miss.
Something like:
const EXCLUDE_FROM_HASH = […];
const entriesToHash = Object.entries(context).filter(([key]) => !EXCLUDE_FROM_HASH.includes(key));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo some inline comments w.r.t. readability.
However, we should consider making optimization/reuse of the page opt-in (like we do for fullyParallel). For smaller suites, newPage is likely plenty fast.
I ran:
const start = hrtime.bigint();
for (let i = 0; i < 150; i++) {
const page = await context.newPage();
await page.close();
}
const end = hrtime.bigint();
console.log('END', browserName, `${end - start} nanoseconds`);
for each of the browsers, and the results were:
browser | time per iteration (ms) |
---|---|
chromium | ~40 |
webkit | ~100 |
firefox | ~150 |
if (_ctPage.page) | ||
await _ctPage.page.close(); | ||
_ctPage.page = await (browser as any)._wrapApiCall(async () => { | ||
const page = await browser.newPage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const page = await browser.newPage(); | |
const context = await browser.newContext(defaultContextOptions); | |
const page = await browser.newPage(); |
This code's complexity is due to the fact that we need a new context, yet it's hidden without the above edit.
await (page.context() as any)._resetForReuse(); | ||
await page.setViewportSize(viewport || { width: 1280, height: 800 }); | ||
page: async ({ _ctPage, browser, viewport, playwright }, use) => { | ||
const defaultContextOptions = (playwright.chromium as any)._defaultContextOptions as BrowserContextOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will magically have the proper context options from test.use({ colorScheme: 'dark' })
? That's not obvious; let's add a comment.
Co-authored-by: Pavel Feldman <pavel.feldman@gmail.com>
No description provided.