🐛 Improve aborting during bulk snapshots (#1015)

* ✨ Add additional core generator utils

* ♻ Use new generator utils to improve aborting bulk snapshots

* 🚨 Turn off linting rule restricting one var per let

* ✅ Fix flakey cli-snapshot tests

.eslintrc

@@ -3,6 +3,7 @@ extends: standard
 plugins:
   - babel
 rules:
+  one-var: off
   prefer-const: off
   no-extra-parens: off
   no-unused-expressions: off

packages/cli-exec/src/start.js

@@ -14,15 +14,14 @@ export const start = command('start', {
   }
 }, async function*({ percy, exit }) {
   if (!percy) exit(0, 'Percy is disabled');
+  let { yieldFor } = await import('@percy/cli-command/utils');
 
   // start percy
   yield* percy.yield.start();
 
   try {
     // run until stopped or terminated
-    while (percy.readyState < 3) {
-      yield new Promise(r => setImmediate(r));
-    }
+    yield* yieldFor(() => percy.readyState >= 3);
   } catch (error) {
     await percy.stop(true);
     throw error;

packages/cli-snapshot/test/directory.test.js

@@ -45,7 +45,7 @@ describe('percy snapshot <directory>', () => {
       '[percy] Snapshot taken: /test-1.html',
       '[percy] Snapshot taken: /test-2.html',
       '[percy] Snapshot taken: /test-3.html',
-      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots'),
+      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots?'),
       '[percy] Finalized build #1: https://percy.io/test/test/123'
     ]));
   });
@@ -60,7 +60,7 @@ describe('percy snapshot <directory>', () => {
       '[percy] Snapshot taken: /base/test-2.html',
       '[percy] Snapshot taken: /base/test-3.html',
       '[percy] Snapshot taken: /base/test-index/index.html',
-      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots'),
+      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots?'),
       '[percy] Finalized build #1: https://percy.io/test/test/123'
     ]));
   });

packages/cli-snapshot/test/file.test.js

@@ -120,7 +120,7 @@ describe('percy snapshot <file>', () => {
       '[percy] Snapshot taken: JS Snapshot',
       '[percy] Snapshot taken: JS Snapshot 2',
       '[percy] Snapshot taken: Other JS Snapshot',
-      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots'),
+      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots?'),
       '[percy] Finalized build #1: https://percy.io/test/test/123'
     ]);
   });
@@ -146,7 +146,7 @@ describe('percy snapshot <file>', () => {
       '[percy] Snapshot taken: /',
       '[percy] Snapshot taken: /one',
       '[percy] Snapshot taken: /two',
-      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots'),
+      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots?'),
       '[percy] Finalized build #1: https://percy.io/test/test/123'
     ]));
   });
@@ -171,7 +171,7 @@ describe('percy snapshot <file>', () => {
       '[percy] Snapshot taken: Snapshot #42',
       '[percy] Snapshot taken: Snapshot #62',
       '[percy] Snapshot taken: Snapshot #82',
-      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots'),
+      jasmine.stringMatching('\\[percy\\] Uploading \\d snapshots?'),
       '[percy] Finalized build #1: https://percy.io/test/test/123'
     ]));
   });

packages/core/src/percy.js

@@ -14,7 +14,8 @@ import {
   discoverSnapshotResources
 } from './snapshot.js';
 import {
-  generatePromise
+  generatePromise,
+  yieldAll
 } from './utils.js';
 
 // A Percy instance will create a new build when started, handle snapshot
@@ -365,7 +366,7 @@ export class Percy {
     } else if (this.build?.error) {
       throw new Error(this.build.error);
     } else if (Array.isArray(options)) {
-      return Promise.all(options.map(o => this.snapshot(o)));
+      return yieldAll(options.map(o => this.yield.snapshot(o)));
     }
 
     if (typeof options === 'string') {
@@ -391,12 +392,11 @@ export class Percy {
         }
 
         // gather snapshots from options
-        let snapshots = yield gatherSnapshots(this, options);
+        let snapshots = yield* gatherSnapshots(this, options);
 
         try {
-          // yield each task individually to allow canceling
-          let tasks = snapshots.map(s => this._takeSnapshot(s));
-          for (let task of tasks) yield task;
+          // use a try-catch to cancel snapshots that haven't started when the error occurred
+          yield* yieldAll(snapshots.map(s => this._takeSnapshot(s)));
         } catch (error) {
           // cancel queued snapshots that may not have started
           snapshots.map(s => this._cancelSnapshot(s));

packages/core/src/snapshot.js

@@ -10,7 +10,8 @@ import {
   hostnameMatches,
   createRootResource,
   createPercyCSSResource,
-  createLogResource
+  createLogResource,
+  yieldTo
 } from './utils.js';
 
 // Throw a better error message for missing or invalid urls
@@ -103,17 +104,16 @@ export function mapSnapshotOptions(percy, snapshots, config) {
 }
 
 // Returns an array of derived snapshot options
-export async function gatherSnapshots(percy, options) {
+export async function* gatherSnapshots(percy, options) {
   let { baseUrl, snapshots } = options;
 
   if ('url' in options) snapshots = [options];
-  if ('sitemap' in options) snapshots = await getSitemapSnapshots(options);
+  if ('sitemap' in options) snapshots = yield getSitemapSnapshots(options);
 
   // validate evaluated snapshots
   if (typeof snapshots === 'function') {
-    ({ snapshots } = validateSnapshotOptions({
-      baseUrl, snapshots: await snapshots(baseUrl)
-    }));
+    snapshots = yield* yieldTo(snapshots(baseUrl));
+    snapshots = validateSnapshotOptions({ baseUrl, snapshots }).snapshots;
   }
 
   // map snapshots with snapshot options

packages/core/src/utils.js

@@ -46,6 +46,14 @@ export function createLogResource(logs) {
   return createResource(`/percy.${Date.now()}.log`, JSON.stringify(logs), 'text/plain');
 }
 
+// Returns true or false if the provided object is a generator or not
+export function isGenerator(subject) {
+  return typeof subject?.next === 'function' && (
+    typeof subject[Symbol.iterator] === 'function' ||
+    typeof subject[Symbol.asyncIterator] === 'function'
+  );
+}
+
 // Iterates over the provided generator and resolves to the final value when done. With an
 // AbortSignal, the generator will throw with the abort reason when aborted. Also accepts an
 // optional node-style callback, called before the returned promise resolves.
@@ -54,10 +62,9 @@ export async function generatePromise(gen, signal, cb) {
     if (typeof signal === 'function') [cb, signal] = [signal];
     if (typeof gen === 'function') gen = await gen();
 
-    let { done, value } = (typeof gen?.next === 'function' && (
-      typeof gen[Symbol.iterator] === 'function' ||
-      typeof gen[Symbol.asyncIterator] === 'function'
-    )) ? await gen.next() : { done: true, value: await gen };
+    let { done, value } = !isGenerator(gen)
+      ? { done: true, value: await gen }
+      : await gen.next();
 
     while (!done) {
       ({ done, value } = signal?.aborted
@@ -90,6 +97,31 @@ export class AbortError extends Error {
   }
 }
 
+// An async generator that yields after every event loop until the promise settles
+export async function* yieldTo(subject) {
+  let pending = typeof subject?.finally === 'function';
+  if (pending) subject = subject.finally(() => (pending = false));
+  /* eslint-disable-next-line no-unmodified-loop-condition */
+  while (pending) yield new Promise(r => setImmediate(r));
+  return isGenerator(subject) ? yield* subject : await subject;
+}
+
+// An async generator that runs provided generators concurrently
+export async function* yieldAll(all) {
+  let res = new Array(all.length).fill();
+  all = all.map(yieldTo);
+
+  while (true) {
+    res = await Promise.all(all.map((g, i) => (
+      res[i]?.done ? res[i] : g.next(res[i]?.value)
+    )));
+
+    let vals = res.map(r => r?.value);
+    if (res.some(r => !r?.done)) yield vals;
+    else return vals;
+  }
+}
+
 // An async generator that infinitely yields to the predicate function until a truthy value is
 // returned. When a timeout is provided, an error will be thrown during the next iteration after the
 // timeout has been exceeded. If an idle option is provided, the predicate will be yielded to a
@@ -188,6 +220,7 @@ export function serializeFunction(fn) {
     '  waitFor, waitForTimeout, waitForSelector, waitForXPath,',
     '  scrollToBottom',
     '}, ...arguments);',
+    `${isGenerator}`,
     `${generatePromise}`,
     `${yieldFor}`,
     `${waitFor}`,

packages/core/test/unit/utils.test.js

@@ -1,6 +1,8 @@
 import {
   generatePromise,
-  AbortController
+  AbortController,
+  yieldTo,
+  yieldAll
 } from '../../src/utils.js';
 
 describe('Unit / Utils', () => {
@@ -95,4 +97,72 @@ describe('Unit / Utils', () => {
       expect(handler).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe('yieldTo', () => {
+    it('returns a generator that finishes when the promise resolves', async () => {
+      let resolve, promise = new Promise(r => (resolve = r));
+      let gen = yieldTo(promise);
+
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: undefined });
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: undefined });
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: undefined });
+
+      resolve('foo');
+
+      await expectAsync(promise).toBeResolved();
+      await expectAsync(gen.next()).toBeResolvedTo({ done: true, value: 'foo' });
+    });
+
+    it('returns a generator that throws when the promise rejects', async () => {
+      let reject, promise = new Promise((_, r) => (reject = r));
+      let gen = yieldTo(promise);
+
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: undefined });
+
+      reject(new Error('foo'));
+
+      await expectAsync(promise).toBeRejectedWithError('foo');
+      await expectAsync(gen.next()).toBeRejectedWithError('foo');
+    });
+
+    it('returns a generator that yields to a provided generator', async () => {
+      function* test(n) { for (let i = 0; i < n; i++) yield i; return n; }
+      let gen = yieldTo(test(3));
+
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: 0 });
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: 1 });
+      await expectAsync(gen.next()).toBeResolvedTo({ done: false, value: 2 });
+      await expectAsync(gen.next()).toBeResolvedTo({ done: true, value: 3 });
+    });
+
+    it('returns a generator that finishes immediately for other values', async () => {
+      await expectAsync(yieldTo(42).next()).toBeResolvedTo({ done: true, value: 42 });
+    });
+  });
+
+  describe('yieldAll', () => {
+    it('returns a generator that yields to all provided values concurrently', async () => {
+      function* test(n) { for (let i = 0; i < n; i++) yield i; return n; }
+
+      let resolve, promise = new Promise(r => (resolve = r));
+      let gen = yieldAll([test(2), 4, null, test(3), promise]);
+
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: false, value: [0, 4, null, 0, undefined] });
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: false, value: [1, 4, null, 1, undefined] });
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: false, value: [2, 4, null, 2, undefined] });
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: false, value: [2, 4, null, 3, undefined] });
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: false, value: [2, 4, null, 3, undefined] });
+
+      resolve(6);
+
+      await expectAsync(promise).toBeResolved();
+      await expectAsync(gen.next()).toBeResolvedTo(
+        { done: true, value: [2, 4, null, 3, 6] });
+    });
+  });
 });