✨Cancelable @percy/core methods (#654)

* 🚨 Turn off node callback literal lint rule * 🐛 Tighten browser launch check Also disable renderer backgrounding which can sometimes cause orphan chromium processes to hang around after tests * ✨ Allow configuring discovery concurrency with setConfig Also mirror snapshot concurrency in the upload queue * ✨ Make internal core method useful for other internal packages * ✨ Generalize queue's cancelable generators into a common util Utilize new util to rewrite the waitFor helper to be cancelable * 🐛 Fix page eval percy helpers Also strip coverage statements from injected code * ✨ Allow some core methods to be cancelable * 🐛 Fix implicit promise behavior of cancelable methods in callbacks
percy · Dec 13, 2021 · dc076d8 · dc076d8
1 parent 0275af8
commit dc076d8
Show file tree

Hide file tree

Showing 12 changed files with 463 additions and 177 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -6,6 +6,7 @@ rules:
   prefer-const: off
   no-unused-expressions: off
   babel/no-unused-expressions: warn
+  node/no-callback-literal: off
   promise/param-names: off
   semi: [error, always]
   multiline-ternary: off

diff --git a/packages/cli-exec/src/commands/exec/start.js b/packages/cli-exec/src/commands/exec/start.js
@@ -32,7 +32,10 @@ export class Start extends Command {
     });
 
     // only stop when terminated
-    let stop = () => percy.stop(true);
+    let stop = async () => {
+      await percy.stop(true);
+    };
+
     process.on('SIGHUP', stop);
     process.on('SIGINT', stop);
     process.on('SIGTERM', stop);

diff --git a/packages/cli-exec/test/start.test.js b/packages/cli-exec/test/start.test.js
@@ -1,4 +1,4 @@
-import fetch from 'node-fetch';
+import request from '@percy/client/dist/request';
 import { logger } from './helpers';
 import { Start } from '../src/commands/exec/start';
 import { Stop } from '../src/commands/exec/stop';
@@ -13,21 +13,20 @@ describe('percy exec:start', () => {
   });
 
   it('starts a long-running percy process', async () => {
-    let response = await fetch('http://localhost:5338/percy/healthcheck');
-    await expectAsync(response.json()).toBeResolvedTo(
-      jasmine.objectContaining({ success: true }));
+    let response = await request('http://localhost:5338/percy/healthcheck');
+    expect(response).toHaveProperty('success', true);
   });
 
   it('stops the process when terminated', async () => {
     await expectAsync(
-      fetch('http://localhost:5338/percy/healthcheck')
+      request('http://localhost:5338/percy/healthcheck')
     ).toBeResolved();
 
     process.emit('SIGTERM');
 
     // check a few times rather than wait on a timeout to be deterministic
     await expectAsync(function check(i = 0) {
-      return fetch('http://localhost:5338/percy/healthcheck', { timeout: 10 })
+      return request('http://localhost:5338/percy/healthcheck', { timeout: 10 })
         .then(r => i >= 10 ? r : new Promise((res, rej) => {
           setTimeout(() => check(i++).then(res, rej), 100);
         }));

diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js
@@ -13,6 +13,7 @@ import Page from './page';
 export default class Browser extends EventEmitter {
   log = logger('core:browser');
   sessions = new Map();
+  readyState = null;
   closed = false;
 
   #callbacks = new Map();
@@ -25,6 +26,8 @@ export default class Browser extends EventEmitter {
     '--disable-background-networking',
     // disable task throttling of timer tasks from background pages
     '--disable-background-timer-throttling',
+    // disable backgrounding renderer processes
+    '--disable-renderer-backgrounding',
     // disable backgrounding renderers for occluded windows (reduce nondeterminism)
     '--disable-backgrounding-occluded-windows',
     // disable crash reporting
@@ -88,7 +91,9 @@ export default class Browser extends EventEmitter {
   }
 
   async launch() {
-    if (this.isConnected()) return;
+    // already launching or launched
+    if (this.readyState != null) return;
+    this.readyState = 0;
 
     // check if any provided executable exists
     if (this.executable && !existsSync(this.executable)) {
@@ -101,12 +106,10 @@ export default class Browser extends EventEmitter {
     // create a temporary profile directory
     this.profile = await fs.mkdtemp(path.join(os.tmpdir(), 'percy-browser-'));
 
-    // collect args to pass to the browser process
-    let args = [...this.args, `--user-data-dir=${this.profile}`];
-
+    // spawn the browser process detached in its own group and session
+    let args = this.args.concat(`--user-data-dir=${this.profile}`);
     this.log.debug('Launching browser');
 
-    // spawn the browser process detached in its own group and session
     this.process = spawn(this.executable, args, {
       detached: process.platform !== 'win32'
     });
@@ -122,15 +125,19 @@ export default class Browser extends EventEmitter {
     // get version information
     this.version = await this.send('Browser.getVersion');
 
-    this.log.debug(`Browser connected: ${this.version.product}`);
+    this.log.debug(`Browser connected [${this.process.pid}]: ${this.version.product}`);
+    this.readyState = 1;
   }
 
   isConnected() {
     return this.ws?.readyState === WebSocket.OPEN;
   }
 
   async close() {
+    // not running, already closed, or closing
     if (this._closed) return this._closed;
+    this.readyState = 2;
+
     this.log.debug('Closing browser');
 
     // resolves when the browser has closed
@@ -162,8 +169,9 @@ export default class Browser extends EventEmitter {
           this.log.debug(error);
         });
       }
-
+    }).then(() => {
       this.log.debug('Browser closed');
+      this.readyState = 3;
     });
 
     // reject any pending callbacks
@@ -242,20 +250,16 @@ export default class Browser extends EventEmitter {
         if (match) cleanup(() => resolve(match[1]));
       };
 
-      /* istanbul ignore next: for sanity */
-      let handleExit = () => handleError();
-      let handleClose = () => handleError();
-      let handleError = error => {
-        cleanup(() => reject(new Error(
-          `Failed to launch browser. ${error?.message ?? ''}\n${stderr}'\n\n`
-        )));
-      };
+      let handleExitClose = () => handleError();
+      let handleError = error => cleanup(() => reject(new Error(
+        `Failed to launch browser. ${error?.message ?? ''}\n${stderr}'\n\n`
+      )));
 
       let cleanup = callback => {
         clearTimeout(timeoutId);
         this.process.stderr.off('data', handleData);
-        this.process.stderr.off('close', handleClose);
-        this.process.off('exit', handleExit);
+        this.process.stderr.off('close', handleExitClose);
+        this.process.off('exit', handleExitClose);
         this.process.off('error', handleError);
         callback();
       };
@@ -265,8 +269,8 @@ export default class Browser extends EventEmitter {
       ), timeout);
 
       this.process.stderr.on('data', handleData);
-      this.process.stderr.on('close', handleClose);
-      this.process.on('exit', handleExit);
+      this.process.stderr.on('close', handleExitClose);
+      this.process.on('exit', handleExitClose);
       this.process.on('error', handleError);
     });
 

diff --git a/packages/core/src/page.js b/packages/core/src/page.js
@@ -1,7 +1,11 @@
 import { promises as fs } from 'fs';
 import logger from '@percy/logger';
 import Network from './network';
-import { hostname, waitFor } from './utils';
+import {
+  hostname,
+  generatePromise,
+  waitFor
+} from './utils';
 
 export default class Page {
   static TIMEOUT = 30000;
@@ -113,11 +117,17 @@ export default class Page {
     }
 
     // wrap the function body with percy helpers
-    fnbody = 'function withPercyHelpers() {' + (
-      `return (${fnbody})({` + (
-        `waitFor: ${waitFor}`
-      ) + '}, ...arguments)'
-    ) + '}';
+    fnbody = 'function withPercyHelpers() {\n' + [
+      `return (${fnbody})({ generatePromise, waitFor }, ...arguments);`,
+      `${generatePromise}`,
+      `${waitFor}`
+    ].join('\n\n') + '}';
+
+    /* istanbul ignore else: ironic. */
+    if (fnbody.includes('cov_')) {
+      // remove coverage statements during testing
+      fnbody = fnbody.replace(/cov_.*?(;\n?|,)\s*/g, '');
+    }
 
     // send the call function command
     let { result, exceptionDetails } =
@@ -146,7 +156,7 @@ export default class Page {
 
     for (let script of scripts) {
       if (typeof script === 'string') {
-        script = `async eval({ waitFor }) {\n${script}\n}`;
+        script = `async eval() {\n${script}\n}`;
       }
 
       await this.eval(script);