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.
2022-12-09 12:36:39 +01:00 · 2022-12-09 12:36:39 +01:00 · 810e0cd74e
commit 810e0cd74e
parent 5e120e49db
6 changed files with 144 additions and 103 deletions
--- 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;
+}
--- 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 &&
--- 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;
--- 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"]
  }
 ]
--- 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,
--- 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;