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

deps: upgrade puppeteer to v21.3.6 #15490

Merged
merged 12 commits into from
Oct 4, 2023
Merged

deps: upgrade puppeteer to v21.3.6 #15490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c397604
deps: upgrade puppeteer to v21.3.3
adamraine Sep 22, 2023
cfc66e6
types tests and stuff
adamraine Sep 22, 2023
fac5c7b
bfcache
adamraine Sep 22, 2023
bec7b61
chrome launcher
adamraine Sep 22, 2023
6f5597f
viewport
adamraine Sep 22, 2023
4b5917e
up
adamraine Sep 25, 2023
da4b374
fix
adamraine Sep 25, 2023
1bb6c01
Merge branch 'main' into pptr_21.3.3
adamraine Sep 25, 2023
30564e9
Merge branch 'main' into pptr_21.3.3
adamraine Sep 26, 2023
2548b11
up
adamraine Sep 26, 2023
ba1ee67
Merge branch 'main' into pptr_21.3.3
adamraine Sep 28, 2023
5950245
up
adamraine Sep 28, 2023
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
1 change: 0 additions & 1 deletion build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) {
// resolved eventually.
plugins.partialLoaders.inlineFs({
verbose: Boolean(process.env.DEBUG),
ignorePaths: [require.resolve('puppeteer-core/lib/esm/puppeteer/common/Page.js')],
Copy link
Member Author

Choose a reason for hiding this comment

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

Page.js has moved but it looks like the comment that contained fs.readFileSync is not longer there so we should be good to remove.

}),
plugins.partialLoaders.rmGetModuleDirectory,
plugins.partialLoaders.replaceText({
Expand Down
12 changes: 0 additions & 12 deletions cli/test/smokehouse/test-definitions/dobetterweb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,18 +494,6 @@ const expectations = {
}],
},
},
{
// Support for this was added in M109
// https://crbug.com/1350944
_maxChromiumVersion: '108',
reason: 'Pages that have requested notifications permissions are not currently eligible for back/forward cache.',
failureType: 'Pending browser support',
subItems: {
items: [{
frameUrl: 'http://localhost:10200/dobetterweb/dbw_tester.html',
}],
},
},
Copy link
Member Author

Choose a reason for hiding this comment

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

drive-by outdated bfcache failure

{
// This issue only appears in the DevTools runner for some reason.
// TODO: Investigate why this doesn't happen on the CLI runner.
Expand Down
6 changes: 3 additions & 3 deletions clients/lightrider/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import {Buffer} from 'buffer';

import log from 'lighthouse-logger';
import {CDPBrowser} from 'puppeteer-core/lib/esm/puppeteer/common/Browser.js';
import {Connection as PptrConnection} from 'puppeteer-core/lib/esm/puppeteer/common/Connection.js';
import {CdpBrowser} from 'puppeteer-core/lib/esm/puppeteer/cdp/Browser.js';
import {Connection as PptrConnection} from 'puppeteer-core/lib/esm/puppeteer/cdp/Connection.js';

import lighthouse, * as api from '../../core/index.js';
import {LighthouseError} from '../../core/lib/lh-error.js';
Expand Down Expand Up @@ -46,7 +46,7 @@ async function getPageFromConnection(connection) {

const pptrConnection = new PptrConnection(mainTargetInfo.url, transport);

const browser = await CDPBrowser._create(
const browser = await CdpBrowser._create(
'chrome',
pptrConnection,
[] /* contextIds */,
Expand Down
2 changes: 2 additions & 0 deletions core/gather/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class ProtocolSession extends CrdpEventEmitter {
this._nextProtocolTimeout = undefined;

this._handleProtocolEvent = this._handleProtocolEvent.bind(this);
// @ts-expect-error Puppeteer expects the handler params to be type `unknown`
this._cdpSession.on('*', this._handleProtocolEvent);
}

Expand Down Expand Up @@ -106,6 +107,7 @@ class ProtocolSession extends CrdpEventEmitter {
* @return {Promise<void>}
*/
async dispose() {
// @ts-expect-error Puppeteer expects the handler params to be type `unknown`
this._cdpSession.off('*', this._handleProtocolEvent);
await this._cdpSession.detach();
}
Expand Down
17 changes: 11 additions & 6 deletions core/scripts/pptr-run-devtools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {fileURLToPath} from 'url';
import * as puppeteer from 'puppeteer-core';
import yargs from 'yargs';
import * as yargsHelpers from 'yargs/helpers';
import {getChromePath} from 'chrome-launcher';
import {launch} from 'chrome-launcher';
import esMain from 'es-main';

import {parseChromeFlags} from '../../cli/run.js';
Expand Down Expand Up @@ -300,12 +300,17 @@ function dismissDialog(dialog) {
* @return {Promise<{lhr: LH.Result, artifacts: LH.Artifacts, logs: string[]}>}
*/
async function testUrlFromDevtools(url, options = {}) {
const {config, chromeFlags} = options;
const {config, chromeFlags = []} = options;

const browser = await puppeteer.launch({
Copy link
Member Author

Choose a reason for hiding this comment

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

Puppeteer will now disable the bfcache if we aren't using tab target which is currently only available through an environment variable PUPPETEER_INTERNAL_TAB_TARGET. The env variable needs to be set before puppeteer is imported so we would need to do some annoying dynamic imports.

This is an alternative approach which just uses chrome-launcher with our desired flags. This should be better for our smoke tests in general because it's closer to how Lighthouse launches Chrome.

executablePath: getChromePath(),
args: chromeFlags,
devtools: true,
const newChromeFlags = [
...chromeFlags,
'--auto-open-devtools-for-tabs',
];

const chrome = await launch({chromeFlags: newChromeFlags});

const browser = await puppeteer.connect({
browserURL: `http://127.0.0.1:${chrome.port}`,
defaultViewport: null,
});

Expand Down
6 changes: 3 additions & 3 deletions core/test/gather/driver/target-manager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import {EventEmitter} from 'events';

import {CDPSessionImpl} from 'puppeteer-core/lib/cjs/puppeteer/common/Connection.js';
import {CdpCDPSession} from 'puppeteer-core/lib/cjs/puppeteer/cdp/CDPSession.js';

import {TargetManager} from '../../../gather/driver/target-manager.js';
import {createMockCdpSession} from '../mock-driver.js';
Expand Down Expand Up @@ -251,9 +251,9 @@ describe('TargetManager', () => {
}

const mockCdpConnection = new MockCdpConnection();
/** @type {LH.Puppeteer.CDPSession} */
/** @type {import('puppeteer-core').CDPSession} */
// @ts-expect-error - close enough to the real thing.
const cdpSession = new CDPSessionImpl(mockCdpConnection, '', sessionId);
const cdpSession = new CdpCDPSession(mockCdpConnection, '', sessionId);
return cdpSession;
}

Expand Down
48 changes: 36 additions & 12 deletions core/test/gather/session-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import {EventEmitter} from 'events';

import {CDPSessionImpl} from 'puppeteer-core/lib/cjs/puppeteer/common/Connection.js';
import {CdpCDPSession} from 'puppeteer-core/lib/cjs/puppeteer/cdp/CDPSession.js';

import {ProtocolSession} from '../../gather/session.js';
import {
Expand All @@ -17,6 +17,27 @@ import {
timers,
} from '../test-utils.js';

/**
* @param {number} id
* @return {LH.Crdp.Page.FrameNavigatedEvent}
*/
function mockFrameNavigated(id) {
return {
frame: {
id: String(id),
loaderId: String(id),
url: `https://example.com/page${id}`,
domainAndRegistry: 'example.com',
securityOrigin: 'https://example.com',
mimeType: 'text/html',
secureContextType: 'Secure',
crossOriginIsolatedContextType: 'NotIsolated',
gatedAPIFeatures: [],
},
type: 'Navigation',
};
}

describe('ProtocolSession', () => {
before(() => timers.useFakeTimers());
after(() => timers.dispose());
Expand All @@ -30,46 +51,49 @@ describe('ProtocolSession', () => {

beforeEach(() => {
// @ts-expect-error - Individual mock functions are applied as necessary.
puppeteerSession = new CDPSessionImpl({_rawSend: fnAny(), send: fnAny()}, '', 'root');
puppeteerSession = new CdpCDPSession({_rawSend: fnAny(), send: fnAny()}, '', 'root');
session = new ProtocolSession(puppeteerSession);
});

describe('responds to events from the underlying CDPSession', () => {
const mockNavigated1 = mockFrameNavigated(1);
const mockNavigated2 = mockFrameNavigated(2);

it('once', async () => {
const callback = fnAny();

session.once('Page.frameNavigated', callback);
puppeteerSession.emit('Page.frameNavigated', {id: 1});
puppeteerSession.emit('Page.frameNavigated', mockNavigated1);
expect(callback).toHaveBeenCalledTimes(1);
expect(callback).toHaveBeenCalledWith({id: 1});
expect(callback).toHaveBeenCalledWith(mockNavigated1);

puppeteerSession.emit('Page.frameNavigated', {id: 2});
puppeteerSession.emit('Page.frameNavigated', mockNavigated2);
expect(callback).toHaveBeenCalledTimes(1);
});

it('on', async () => {
const callback = fnAny();

session.on('Page.frameNavigated', callback);
puppeteerSession.emit('Page.frameNavigated', {id: 1});
puppeteerSession.emit('Page.frameNavigated', mockNavigated1);
expect(callback).toHaveBeenCalledTimes(1);
expect(callback).toHaveBeenCalledWith({id: 1});
expect(callback).toHaveBeenCalledWith(mockNavigated1);

puppeteerSession.emit('Page.frameNavigated', {id: 2});
puppeteerSession.emit('Page.frameNavigated', mockNavigated2);
expect(callback).toHaveBeenCalledTimes(2);
expect(callback).toHaveBeenCalledWith({id: 2});
expect(callback).toHaveBeenCalledWith(mockNavigated2);
});

it('off', async () => {
const callback = fnAny();

session.on('Page.frameNavigated', callback);
puppeteerSession.emit('Page.frameNavigated', {id: 1});
puppeteerSession.emit('Page.frameNavigated', mockNavigated1);
expect(callback).toHaveBeenCalledTimes(1);
expect(callback).toHaveBeenCalledWith({id: 1});
expect(callback).toHaveBeenCalledWith(mockNavigated1);

session.off('Page.frameNavigated', callback);
puppeteerSession.emit('Page.frameNavigated', {id: 2});
puppeteerSession.emit('Page.frameNavigated', mockNavigated2);
expect(callback).toHaveBeenCalledTimes(1);
});
});
Expand Down
6 changes: 1 addition & 5 deletions core/test/scenarios/api-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ describe('Individual modes API', function() {
const run = await api.startTimespan(state.page);
for (const iframe of page.frames()) {
if (iframe.url().includes('/oopif-simple-page.html')) {
iframe.click('button');
await iframe.click('button');
Copy link
Member Author

@adamraine adamraine Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Without this await the button click could happen after we end the timespan

This is a fix for the issue mentioned in #15490 (comment)

}
}
await page.waitForNetworkIdle().catch(() => {});
Expand All @@ -180,10 +180,6 @@ describe('Individual modes API', function() {
.map((r) => ({url: r.url, sessionTargetType: r.sessionTargetType}))
// @ts-expect-error
.sort((a, b) => a.url.localeCompare(b.url));
expect(networkRequests).toHaveLength(10);
expect(networkRequests.filter(r => r.sessionTargetType === 'page')).toHaveLength(2);
expect(networkRequests.filter(r => r.sessionTargetType === 'iframe')).toHaveLength(2);
expect(networkRequests.filter(r => r.sessionTargetType === 'worker')).toHaveLength(6);
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these checks are redundant with the snapshot check just below. The snapshot check provides better details on failure so we should just fall to that if a request is missing.

expect(networkRequests).toMatchInlineSnapshot(`
Array [
Object {
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@
"pako": "^2.0.3",
"preact": "^10.7.2",
"pretty-json-stringify": "^0.0.2",
"puppeteer": "21.1.1",
"puppeteer": "^21.3.6",
"resolve": "^1.22.1",
"rollup": "^2.52.7",
"rollup-plugin-polyfill-node": "^0.12.0",
Expand Down Expand Up @@ -199,7 +199,7 @@
"open": "^8.4.0",
"parse-cache-control": "1.0.1",
"ps-list": "^8.0.0",
"puppeteer-core": "21.1.1",
"puppeteer-core": "^21.3.6",
"robots-parser": "^3.0.1",
"semver": "^5.3.0",
"speedline-core": "^1.4.3",
Expand Down
3 changes: 3 additions & 0 deletions types/internal/rxjs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,8 @@ declare module 'rxjs' {
export const tap: any;
export const throwIfEmpty: any;
export const firstValueFrom: any;
export const delay: any;
export const startWith: any;
export const switchMap: any;
}

Loading
Loading