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

@@ -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]>

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<!Puppeteer.JSHandle>} 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<!Puppeteer.JSHandle>} args
    * @param {ConsoleMessage.Location} location
    */
-  constructor(type, text, args = [], location = {}) {
+  constructor(type, text, args, location = {}) {
     this._type = type;
     this._text = text;
     this._args = args;

lib/Worker.js

@@ -21,7 +21,7 @@ class Worker extends EventEmitter {
   /**
    * @param {Puppeteer.CDPSession} client
    * @param {string} url
-   * @param {function(!string, !Array<!JSHandle>)} consoleAPICalled
+   * @param {function(!string, !Array<!JSHandle>, 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));
   }
 

test/assets/consolelog.html

@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <title>console.log test</title>
+  </head>
+  <body>
+    <script>
+      console.log('yellow')
+    </script>
+  </body>
+</html>

test/page.spec.js

@@ -322,39 +322,31 @@ 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(`<script>fetch('http://wat');</script>`),
       ]);
-      await new Promise(resolve => setTimeout(resolve, 5000));
       expect(message.text()).toContain(`ERR_NAME_NOT_RESOLVED`);
       expect(message.type()).toEqual('error');
       expect(message.location()).toEqual({
         url: 'http://wat/',
         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,
       });
     });
   });

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));