From 810e0cd74ecef353cfa43746c18bd5f580a3233d Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Fri, 9 Dec 2022 12:36:39 +0100 Subject: [PATCH] fix: remove oopif expectations and fix oopif flakiness (#9375) With M109 the flakiness should be reduced. Any present flakiness should be investigated. Drive-by: a new debugging helper to debug on CI. --- packages/puppeteer-core/src/common/Debug.ts | 27 +++ .../puppeteer-core/src/common/FrameManager.ts | 27 +-- packages/puppeteer-core/src/common/Page.ts | 3 - test/TestExpectations.json | 154 ++++++++---------- test/src/mocha-utils.ts | 32 ++++ test/src/oopif.spec.ts | 4 +- 6 files changed, 144 insertions(+), 103 deletions(-) diff --git a/packages/puppeteer-core/src/common/Debug.ts b/packages/puppeteer-core/src/common/Debug.ts index 425b7c30..ae9ddab9 100644 --- a/packages/puppeteer-core/src/common/Debug.ts +++ b/packages/puppeteer-core/src/common/Debug.ts @@ -76,6 +76,9 @@ export async function importDebug(): Promise { export const debug = (prefix: string): ((...args: unknown[]) => void) => { if (isNode) { return async (...logArgs: unknown[]) => { + if (captureLogs) { + capturedLogs.push(prefix + logArgs); + } (await importDebug())(prefix)(logArgs); }; } @@ -107,3 +110,27 @@ export const debug = (prefix: string): ((...args: unknown[]) => void) => { console.log(`${prefix}:`, ...logArgs); }; }; + +/** + * @internal + */ +let capturedLogs: string[] = []; +/** + * @internal + */ +let captureLogs = false; + +/** + * @internal + */ +export function setLogCapture(value: boolean): void { + capturedLogs = []; + captureLogs = value; +} + +/** + * @internal + */ +export function getCapturedLogs(): string[] { + return capturedLogs; +} diff --git a/packages/puppeteer-core/src/common/FrameManager.ts b/packages/puppeteer-core/src/common/FrameManager.ts index b0c22402..230fe8ba 100644 --- a/packages/puppeteer-core/src/common/FrameManager.ts +++ b/packages/puppeteer-core/src/common/FrameManager.ts @@ -68,6 +68,13 @@ export class FrameManager extends EventEmitter { */ _frameTree = new FrameTree(); + /** + * Set of frame IDs stored to indicate if a frame has received a + * frameNavigated event so that frame tree responses could be ignored as the + * frameNavigated event usually contains the latest information. + */ + #frameNavigatedReceived = new Set(); + get timeoutSettings(): TimeoutSettings { return this.#timeoutSettings; } @@ -99,6 +106,7 @@ export class FrameManager extends EventEmitter { this.#onFrameAttached(session, event.frameId, event.parentFrameId); }); session.on('Page.frameNavigated', event => { + this.#frameNavigatedReceived.add(event.frame.id); this.#onFrameNavigated(event.frame); }); session.on('Page.navigatedWithinDocument', event => { @@ -203,15 +211,6 @@ export class FrameManager extends EventEmitter { this.initialize(target._session()); } - onDetachedFromTarget(target: Target): void { - const frame = this.frame(target._targetId); - if (frame && frame.isOOPFrame()) { - // When an OOP iframe is removed from the page, it - // will only get a Target.detachedFromTarget event. - this.#removeFramesRecursively(frame); - } - } - #onLifecycleEvent(event: Protocol.Page.LifecycleEventEvent): void { const frame = this.frame(event.frameId); if (!frame) { @@ -249,7 +248,12 @@ export class FrameManager extends EventEmitter { frameTree.frame.parentId ); } - this.#onFrameNavigated(frameTree.frame); + if (!this.#frameNavigatedReceived.has(frameTree.frame.id)) { + this.#onFrameNavigated(frameTree.frame); + } else { + this.#frameNavigatedReceived.delete(frameTree.frame.id); + } + if (!frameTree.childFrames) { return; } @@ -384,8 +388,7 @@ export class FrameManager extends EventEmitter { if (frame._client() !== session) { return; } - - if (contextPayload.auxData && !!contextPayload.auxData['isDefault']) { + if (contextPayload.auxData && contextPayload.auxData['isDefault']) { world = frame.worlds[MAIN_WORLD]; } else if ( contextPayload.name === UTILITY_WORLD_NAME && diff --git a/packages/puppeteer-core/src/common/Page.ts b/packages/puppeteer-core/src/common/Page.ts index b28fc475..950b22c7 100644 --- a/packages/puppeteer-core/src/common/Page.ts +++ b/packages/puppeteer-core/src/common/Page.ts @@ -262,9 +262,6 @@ export class CDPPage extends Page { #onDetachedFromTarget = (target: Target) => { const sessionId = target._session()?.id(); - - this.#frameManager.onDetachedFromTarget(target); - const worker = this.#workers.get(sessionId!); if (!worker) { return; diff --git a/test/TestExpectations.json b/test/TestExpectations.json index 6b80ec69..427b06b4 100644 --- a/test/TestExpectations.json +++ b/test/TestExpectations.json @@ -3,7 +3,7 @@ "testIdPattern": "[accessibility.spec]", "platforms": ["darwin", "linux", "win32"], "parameters": ["firefox"], - "expectations": ["SKIP"] + "expectations": ["SKIP", "TIMEOUT"] }, { "testIdPattern": "[ariaqueryhandler.spec]", @@ -1175,30 +1175,6 @@ "parameters": ["firefox"], "expectations": ["SKIP"] }, - { - "testIdPattern": "[oopif.spec] OOPIF should support OOP iframes becoming normal iframes again", - "platforms": ["darwin"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL"] - }, - { - "testIdPattern": "[oopif.spec] OOPIF should keep track of a frames OOP state", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL"] - }, - { - "testIdPattern": "[oopif.spec] OOPIF should wait for inner OOPIFs", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL", "TIMEOUT"] - }, - { - "testIdPattern": "[oopif.spec] OOPIF should track navigations within OOP iframes", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL"] - }, { "testIdPattern": "[launcher.spec] Launcher specs Puppeteer Puppeteer.connect should be able to connect to a browser with no page targets", "platforms": ["linux", "darwin", "win32"], @@ -1967,54 +1943,6 @@ "parameters": ["firefox"], "expectations": ["SKIP"] }, - { - "testIdPattern": "", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["SKIP"] - }, - { - "testIdPattern": "[launcher.spec] Launcher specs Puppeteer Puppeteer.launch can launch and close the browser", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["PASS"] - }, - { - "testIdPattern": "[Connection.spec] WebDriver BiDi", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["PASS"] - }, - { - "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["PASS"] - }, - { - "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work with function shorthands", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["SKIP"] - }, - { - "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work with unicode chars", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["SKIP"] - }, - { - "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work right after framenavigated", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["SKIP"] - }, - { - "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work from-inside an exposed function", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["firefox", "webDriverBiDi"], - "expectations": ["SKIP"] - }, { "testIdPattern": "[launcher.spec] Launcher specs Puppeteer Puppeteer.launch should be able to launch Firefox", "platforms": ["darwin", "linux", "win32"], @@ -2029,7 +1957,7 @@ }, { "testIdPattern": "[headful.spec] headful tests HEADFUL target.page() should return a background_page", - "platforms": ["win32"], + "platforms": ["win32", "darwin"], "parameters": ["chrome"], "expectations": ["PASS", "FAIL"] }, @@ -2075,12 +2003,6 @@ "parameters": ["chrome"], "expectations": ["PASS", "FAIL"] }, - { - "testIdPattern": "[oopif.spec]", - "platforms": ["win32"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL"] - }, { "testIdPattern": "[network.spec] network \"after each\" hook for \"Same-origin set-cookie subresource\"", "platforms": ["win32"], @@ -2105,12 +2027,6 @@ "parameters": ["chrome", "chrome-headless"], "expectations": ["PASS", "FAIL"] }, - { - "testIdPattern": "[headful.spec] headful tests HEADFUL OOPIF: should expose events within OOPIFs", - "platforms": ["linux"], - "parameters": ["chrome"], - "expectations": ["PASS", "FAIL"] - }, { "testIdPattern": "[waittask.spec] waittask specs Frame.waitForFunction should survive cross-process navigation", "platforms": ["darwin", "linux", "win32"], @@ -2128,5 +2044,71 @@ "platforms": ["darwin", "linux", "win32"], "parameters": ["firefox"], "expectations": ["PASS", "FAIL"] + }, + { + "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should work", + "platforms": ["linux"], + "parameters": ["chrome", "chrome-headless"], + "expectations": ["PASS", "FAIL"] + }, + { + "testIdPattern": "[tracing.spec] Tracing \"after each\" hook for \"should output a trace\"", + "platforms": ["win32"], + "parameters": ["chrome", "headless"], + "expectations": ["PASS", "FAIL"] + }, + { + "testIdPattern": "[tracing.spec] Tracing \"after each\" hook for \"should output a trace\"", + "platforms": ["win32"], + "parameters": ["chrome", "headless"], + "expectations": ["PASS", "FAIL"] + }, + { + "testIdPattern": "", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["SKIP", "TIMEOUT"] + }, + { + "testIdPattern": "[launcher.spec] Launcher specs Puppeteer Puppeteer.launch can launch and close the browser", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["PASS"] + }, + { + "testIdPattern": "[Connection.spec] WebDriver BiDi", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["PASS"] + }, + { + "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["PASS"] + }, + { + "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work with function shorthands", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["SKIP"] + }, + { + "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work with unicode chars", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["SKIP"] + }, + { + "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work right after framenavigated", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["SKIP"] + }, + { + "testIdPattern": "[evaluation.spec] Evaluation specs Page.evaluate should work from-inside an exposed function", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["SKIP"] } ] diff --git a/test/src/mocha-utils.ts b/test/src/mocha-utils.ts index c0b3153f..b4e66721 100644 --- a/test/src/mocha-utils.ts +++ b/test/src/mocha-utils.ts @@ -32,6 +32,10 @@ import puppeteer from 'puppeteer/lib/cjs/puppeteer/puppeteer.js'; import {TestServer} from '@pptr/testserver'; import {extendExpectWithToBeGolden} from './utils.js'; import * as Mocha from 'mocha'; +import { + setLogCapture, + getCapturedLogs, +} from 'puppeteer-core/internal/common/Debug.js'; const setupServer = async () => { const assetsPath = path.join(__dirname, '../assets'); @@ -278,6 +282,34 @@ export const expectCookieEquals = ( } }; +/** + * Use it if you want to capture debug logs for a specitic test suite in CI. + * This describe function enables capturing of debug logs and would print them + * only if a test fails to reduce the amount of output. + */ +export const describeWithDebugLogs = ( + description: string, + body: (this: Mocha.Suite) => void +): Mocha.Suite | void => { + describe(description + '-debug', () => { + beforeEach(() => { + setLogCapture(true); + }); + + afterEach(function () { + if (this.currentTest?.state === 'failed') { + console.log( + `\n"${this.currentTest.fullTitle()}" failed. Here is a debug log:` + ); + console.log(getCapturedLogs().join('\n') + '\n'); + } + setLogCapture(false); + }); + + describe(description, body); + }); +}; + export const shortWaitForArrayToHaveAtLeastNElements = async ( data: unknown[], minLength: number, diff --git a/test/src/oopif.spec.ts b/test/src/oopif.spec.ts index d27c3173..eb2eb5e0 100644 --- a/test/src/oopif.spec.ts +++ b/test/src/oopif.spec.ts @@ -16,12 +16,12 @@ import utils from './utils.js'; import expect from 'expect'; -import {getTestState} from './mocha-utils.js'; +import {describeWithDebugLogs, getTestState} from './mocha-utils.js'; import {Browser} from 'puppeteer-core/internal/api/Browser.js'; import {BrowserContext} from 'puppeteer-core/internal/api/BrowserContext.js'; import {Page} from 'puppeteer-core/internal/api/Page.js'; -describe('OOPIF', function () { +describeWithDebugLogs('OOPIF', function () { /* We use a special browser for this test as we need the --site-per-process flag */ let browser: Browser; let context: BrowserContext;