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.
puppeteer · Dec 9, 2022 · 810e0cd · 810e0cd
1 parent 5e120e4
commit 810e0cd
Show file tree

Hide file tree

Showing 6 changed files with 144 additions and 103 deletions.
diff --git 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<typeof import('debug')> {
 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
@@ -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<string>();
+
   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
@@ -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
@@ -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
@@ -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
@@ -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;