From d47f6b1bc319112c99b73e4a696b2b4c5c09afa2 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Thu, 12 Aug 2021 15:36:08 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Wait=20for=20isolated=20networks?= =?UTF-8?q?=20to=20idle=20(#491)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 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 --- packages/core/src/network.js | 47 +++++++++++++++++----------- packages/core/src/page.js | 20 ++++++++++-- packages/core/test/discovery.test.js | 34 ++++++++++++++++++++ 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/packages/core/src/network.js b/packages/core/src/network.js index 50b900314..f47911245 100644 --- 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 index 81d0b8708..9747ce95c 100644 --- 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 index a788ccd4a..c6bc09ae2 100644 --- 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', [ + '' + ].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', () => {