🐛 Wait for isolated networks to idle (#491)

* 🐛 Wait for isolated networks to idle * 🔇 Create common handler to silence expected page closed errors * ✅ Remove lower network idle timeout value in test This value is too low for CI and some assets are missed
percy · Aug 12, 2021 · d47f6b1 · d47f6b1
1 parent 120c61e
commit d47f6b1
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 21 deletions.
diff --git a/packages/core/src/network.js b/packages/core/src/network.js
@@ -29,10 +29,7 @@ export default class Network {
     this.page.on('Network.loadingFinished', this._handleLoadingFinished);
     this.page.on('Network.loadingFailed', this._handleLoadingFailed);
     this.page.on('Page.frameDetached', this._handleFrameDetached);
-
-    /* istanbul ignore next: race condition */
-    this.page.send('Network.enable')
-      .catch(e => this.log.debug(e, this.page.meta));
+    this.page._handleCloseRace(this.page.send('Network.enable'));
   }
 
   // Enable request interception
@@ -50,37 +47,49 @@ export default class Network {
   }
 
   // Resolves after the timeout when there are no more in-flight requests.
-  async idle(filter = r => r, timeout = this.timeout || 100) {
-    let getRequests = () => Array.from(this.#requests.values())
-      .reduce((a, r) => filter(r) ? a.concat(r.url) : a, []);
+  async idle(filter, timeout = this.timeout || 100) {
+    let requests = [];
 
     this.log.debug(`Wait for ${timeout}ms idle`, this.page.meta);
 
-    try {
-      await waitFor(() => {
-        if (this.page.closedReason) {
-          throw new Error(`Network error: ${this.page.closedReason}`);
-        }
+    await waitFor(() => {
+      if (this.page.closedReason) {
+        throw new Error(`Network error: ${this.page.closedReason}`);
+      }
 
-        return getRequests().length === 0;
-      }, {
-        timeout: NETWORK_TIMEOUT,
-        idle: timeout
-      });
-    } catch (error) {
+      requests = this.getRequests(filter);
+      return requests.length === 0;
+    }, {
+      timeout: NETWORK_TIMEOUT,
+      idle: timeout
+    }).catch(error => {
       // throw a better timeout error
       if (error.message.startsWith('Timeout')) {
         let msg = 'Timed out waiting for network requests to idle.';
 
         if (this.log.shouldLog('debug')) {
-          msg += `\n\n  ${['Active requests:', ...getRequests()].join('\n  -> ')}\n`;
+          msg += `\n\n  ${[
+            'Active requests:',
+            ...requests.map(r => r.url)
+          ].join('\n  -> ')}\n`;
         }
 
         throw new Error(msg);
       } else {
         throw error;
       }
+    });
+  }
+
+  // Used to recursively collect requests from this frame and subframes
+  getRequests(filter = () => true) {
+    let requests = Array.from(this.#requests.values()).filter(filter);
+
+    for (let frame of this.page.frames) {
+      requests.push(...frame.network.getRequests(filter));
     }
+
+    return requests;
   }
 
   // Called when a request should be removed from various trackers

diff --git a/packages/core/src/page.js b/packages/core/src/page.js
@@ -16,6 +16,7 @@ export default class Page extends EventEmitter {
   frameId = null;
   contextId = null;
   closedReason = null;
+  frames = new Set();
   log = logger('core:page');
 
   constructor(browser, { params, sessionId: parentId }) {
@@ -34,7 +35,11 @@ export default class Page extends EventEmitter {
 
     // if there is a parent session, automatically init this session
     this.parent = browser.pages.get(parentId);
-    if (this.parent) this.init(this.parent.options);
+
+    if (this.parent) {
+      this.parent.frames.add(this);
+      this._handleCloseRace(this.init(this.parent.options));
+    }
   }
 
   // initial page options asynchronously
@@ -96,6 +101,8 @@ export default class Page extends EventEmitter {
   async close() {
     if (!this.browser) return;
 
+    this.log.debug('Page closing', this.meta);
+
     /* istanbul ignore next: errors race here when the browser closes */
     await this.browser.send('Target.closeTarget', { targetId: this.targetId })
       .catch(error => this.log.debug(error, this.meta));
@@ -307,7 +314,6 @@ export default class Page extends EventEmitter {
   }
 
   _handleClose() {
-    this.log.debug('Page closing', this.meta);
     this.closedReason ||= 'Page closed.';
 
     // reject any pending callbacks
@@ -318,9 +324,19 @@ export default class Page extends EventEmitter {
     }
 
     this.#callbacks.clear();
+    this.parent?.frames.delete(this);
     this.browser = null;
   }
 
+  _handleCloseRace(promise) {
+    /* istanbul ignore next: race conditions, amirite? */
+    return promise.catch(error => {
+      if (!error.message.endsWith(this.closedReason)) {
+        this.log.debug(error, this.meta);
+      }
+    });
+  }
+
   _handleExecutionContextCreated = event => {
     if (this.frameId === event.context.auxData.frameId) {
       this.contextId = event.context.id;

diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js
@@ -1001,6 +1001,40 @@ describe('Discovery', () => {
 
       await expectAsync(percy.idle()).toBeResolved();
     });
+
+    it('waits to capture resources from isolated pages', async () => {
+      server.reply('/', () => [200, {
+        'Content-Type': 'text/html',
+        'Origin-Agent-Cluster': '?1' // force page isolation
+      }, testDOM]);
+
+      server.reply('/img.gif', () => new Promise(resolve => {
+        // wait a tad longer than network idle would
+        setTimeout(resolve, 200, [200, 'image/gif', pixel]);
+      }));
+
+      server2.reply('/', () => [200, 'text/html', [
+        '<iframe src="http://embed.localhost:8000"></iframe>'
+      ].join('\n')]);
+
+      await percy.snapshot({
+        name: 'test cors',
+        url: 'http://test.localhost:8001',
+        discovery: {
+          allowedHostnames: ['embed.localhost']
+        }
+      });
+
+      await percy.idle();
+
+      expect(captured[0]).toContain(
+        jasmine.objectContaining({
+          attributes: jasmine.objectContaining({
+            'resource-url': 'http://embed.localhost:8000/img.gif'
+          })
+        })
+      );
+    });
   });
 
   describe('with launch options', () => {