From 89fc2adff57b939b8426e0fe68b72397522d4128 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 10 Jan 2019 18:05:28 -0800 Subject: [PATCH] fix(page): consoleMessage.location() should work with workers (#3752) This patch: - refactors consoleMessage.location() implementation to make it work for workers - re-writes tests to avoid 5 second delay --- docs/api.md | 6 +++--- lib/Page.js | 22 ++++++++++---------- lib/Worker.js | 4 ++-- test/assets/console-message-1.html | 11 ---------- test/assets/console-message-2.html | 11 ---------- test/assets/consolelog.html | 11 ++++++++++ test/page.spec.js | 32 +++++++++++------------------- test/worker.spec.js | 16 +++++++++++---- 8 files changed, 50 insertions(+), 63 deletions(-) delete mode 100644 test/assets/console-message-1.html delete mode 100644 test/assets/console-message-2.html create mode 100644 test/assets/consolelog.html diff --git a/docs/api.md b/docs/api.md index ea67d0b954106..9dde855aa24e1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -2318,9 +2318,9 @@ puppeteer.launch().then(async browser => { #### consoleMessage.location() - returns: <[Object]> - - `url` <[string]> URL of the resource if known - - `lineNumber` <[number]> line number in the resource - - `columnNumber` <[number]> line number in the resource + - `url` <[string]> URL of the resource if known or `undefined` otherwise. + - `lineNumber` <[number]> line number in the resource if known or `undefined` otherwise. + - `columnNumber` <[number]> column number in the resource if known or `undefined` otherwise. #### consoleMessage.text() - returns: <[string]> diff --git a/lib/Page.js b/lib/Page.js index 1e7555a9140e4..01ceff67d2bc6 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -191,7 +191,7 @@ class Page extends EventEmitter { if (args) args.map(arg => helper.releaseObject(this._client, arg)); if (source !== 'worker') - this.emit(Page.Events.Console, new ConsoleMessage(level, text, undefined, { url, lineNumber })); + this.emit(Page.Events.Console, new ConsoleMessage(level, text, [], {url, lineNumber})); } /** @@ -510,14 +510,7 @@ class Page extends EventEmitter { async _onConsoleAPI(event) { const context = this._frameManager.executionContextById(event.executionContextId); const values = event.args.map(arg => createJSHandle(context, arg)); - const location = {}; - if (event.stackTrace && event.stackTrace.callFrames) { - const {url, lineNumber, columnNumber} = event.stackTrace.callFrames[0]; - if (url) location.url = url; - if (lineNumber) location.lineNumber = lineNumber; - if (columnNumber) location.columnNumber = columnNumber; - } - this._addConsoleMessage(event.type, values, location); + this._addConsoleMessage(event.type, values, event.stackTrace); } /** @@ -574,9 +567,9 @@ class Page extends EventEmitter { /** * @param {string} type * @param {!Array} args - * @param {ConsoleMessage.Location} location + * @param {Protocol.Runtime.StackTrace=} stackTrace */ - _addConsoleMessage(type, args, location) { + _addConsoleMessage(type, args, stackTrace) { if (!this.listenerCount(Page.Events.Console)) { args.forEach(arg => arg.dispose()); return; @@ -589,6 +582,11 @@ class Page extends EventEmitter { else textTokens.push(helper.valueFromRemoteObject(remoteObject)); } + const location = stackTrace && stackTrace.callFrames.length ? { + url: stackTrace.callFrames[0].url, + lineNumber: stackTrace.callFrames[0].lineNumber, + columnNumber: stackTrace.callFrames[0].columnNumber, + } : {}; const message = new ConsoleMessage(type, textTokens.join(' '), args, location); this.emit(Page.Events.Console, message); } @@ -1248,7 +1246,7 @@ class ConsoleMessage { * @param {!Array} args * @param {ConsoleMessage.Location} location */ - constructor(type, text, args = [], location = {}) { + constructor(type, text, args, location = {}) { this._type = type; this._text = text; this._args = args; diff --git a/lib/Worker.js b/lib/Worker.js index df41f186fae78..f3d8928d5d617 100644 --- a/lib/Worker.js +++ b/lib/Worker.js @@ -21,7 +21,7 @@ class Worker extends EventEmitter { /** * @param {Puppeteer.CDPSession} client * @param {string} url - * @param {function(!string, !Array)} consoleAPICalled + * @param {function(!string, !Array, Protocol.Runtime.StackTrace=)} consoleAPICalled * @param {function(!Protocol.Runtime.ExceptionDetails)} exceptionThrown */ constructor(client, url, consoleAPICalled, exceptionThrown) { @@ -39,7 +39,7 @@ class Worker extends EventEmitter { // This might fail if the target is closed before we recieve all execution contexts. this._client.send('Runtime.enable', {}).catch(debugError); - this._client.on('Runtime.consoleAPICalled', event => consoleAPICalled(event.type, event.args.map(jsHandleFactory))); + this._client.on('Runtime.consoleAPICalled', event => consoleAPICalled(event.type, event.args.map(jsHandleFactory), event.stackTrace)); this._client.on('Runtime.exceptionThrown', exception => exceptionThrown(exception.exceptionDetails)); } diff --git a/test/assets/console-message-1.html b/test/assets/console-message-1.html deleted file mode 100644 index 3336525736e94..0000000000000 --- a/test/assets/console-message-1.html +++ /dev/null @@ -1,11 +0,0 @@ - - - - Log Entry Test - - - - - diff --git a/test/assets/console-message-2.html b/test/assets/console-message-2.html deleted file mode 100644 index ae576efc99249..0000000000000 --- a/test/assets/console-message-2.html +++ /dev/null @@ -1,11 +0,0 @@ - - - - Log Entry Test - - - - - diff --git a/test/assets/consolelog.html b/test/assets/consolelog.html new file mode 100644 index 0000000000000..7fa1b211a4df0 --- /dev/null +++ b/test/assets/consolelog.html @@ -0,0 +1,11 @@ + + + + console.log test + + + + + diff --git a/test/page.spec.js b/test/page.spec.js index 588a30054f1d4..89d89a9a39897 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -322,16 +322,12 @@ module.exports.addTests = function({testRunner, expect, headless}) { expect(message.text()).toContain('No \'Access-Control-Allow-Origin\''); expect(message.type()).toEqual('error'); }); - it('should show correct additional info when console event emitted for logEntry', async({page, server}) => { - let message = null; - page.on('console', msg => { - message = msg; - }); - await Promise.all([ - page.goto(server.PREFIX + '/console-message-1.html'), + it('should have location when fetch fails', async({page, server}) => { + await page.goto(server.EMPTY_PAGE); + const [message] = await Promise.all([ waitEvent(page, 'console'), + page.setContent(``), ]); - await new Promise(resolve => setTimeout(resolve, 5000)); expect(message.text()).toContain(`ERR_NAME_NOT_RESOLVED`); expect(message.type()).toEqual('error'); expect(message.location()).toEqual({ @@ -339,22 +335,18 @@ module.exports.addTests = function({testRunner, expect, headless}) { lineNumber: undefined }); }); - it('should show correct additional info when console event emitted for consoleAPI', async({page, server}) => { - let message = null; - page.on('console', msg => { - message = msg; - }); - await Promise.all([ - page.goto(server.PREFIX + '/console-message-2.html'), + it('should have location for console API calls', async({page, server}) => { + await page.goto(server.EMPTY_PAGE); + const [message] = await Promise.all([ waitEvent(page, 'console'), + page.goto(server.PREFIX + '/consolelog.html'), ]); - await new Promise(resolve => setTimeout(resolve, 5000)); - expect(message.text()).toContain(`wat`); - expect(message.type()).toEqual('warning'); + expect(message.text()).toBe('yellow'); + expect(message.type()).toBe('log'); expect(message.location()).toEqual({ - url: `http://localhost:${server.PORT}/console-message-2.html`, + url: server.PREFIX + '/consolelog.html', lineNumber: 7, - columnNumber: 16 + columnNumber: 14, }); }); }); diff --git a/test/worker.spec.js b/test/worker.spec.js index 0f95ee70c1879..a4ccb62fb5203 100644 --- a/test/worker.spec.js +++ b/test/worker.spec.js @@ -1,3 +1,5 @@ +const utils = require('./utils'); +const {waitEvent} = utils; module.exports.addTests = function({testRunner, expect}) { const {describe, xdescribe, fdescribe} = testRunner; @@ -29,10 +31,16 @@ module.exports.addTests = function({testRunner, expect}) { expect(error.message).toContain('Most likely the worker has been closed.'); }); it('should report console logs', async function({page}) { - const logPromise = new Promise(x => page.on('console', x)); - await page.evaluate(() => new Worker(`data:text/javascript,console.log(1)`)); - const log = await logPromise; - expect(log.text()).toBe('1'); + const [message] = await Promise.all([ + waitEvent(page, 'console'), + page.evaluate(() => new Worker(`data:text/javascript,console.log(1)`)), + ]); + expect(message.text()).toBe('1'); + expect(message.location()).toEqual({ + url: 'data:text/javascript,console.log(1)', + lineNumber: 0, + columnNumber: 8, + }); }); it('should have JSHandles for console logs', async function({page}) { const logPromise = new Promise(x => page.on('console', x));