From 464fd4609f401edf044e15c7c78bd11b37751a43 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Fri, 29 Jul 2022 11:54:12 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Improve=20aborting=20during=20bu?= =?UTF-8?q?lk=20snapshots=20(#1015)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ 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 | 1 + packages/cli-exec/src/start.js | 5 +- packages/cli-snapshot/test/directory.test.js | 4 +- packages/cli-snapshot/test/file.test.js | 6 +- packages/core/src/percy.js | 12 ++-- packages/core/src/snapshot.js | 12 ++-- packages/core/src/utils.js | 41 +++++++++-- packages/core/test/unit/utils.test.js | 72 +++++++++++++++++++- 8 files changed, 128 insertions(+), 25 deletions(-) diff --git a/.eslintrc b/.eslintrc index a156d7954..915151229 100644 --- a/.eslintrc +++ b/.eslintrc @@ -3,6 +3,7 @@ extends: standard plugins: - babel rules: + one-var: off prefer-const: off no-extra-parens: off no-unused-expressions: off diff --git a/packages/cli-exec/src/start.js b/packages/cli-exec/src/start.js index 19bc7fef6..704bf4d17 100644 --- a/packages/cli-exec/src/start.js +++ b/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; diff --git a/packages/cli-snapshot/test/directory.test.js b/packages/cli-snapshot/test/directory.test.js index 1d9009ae9..3ca1c6753 100644 --- a/packages/cli-snapshot/test/directory.test.js +++ b/packages/cli-snapshot/test/directory.test.js @@ -45,7 +45,7 @@ describe('percy snapshot ', () => { '[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 ', () => { '[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' ])); }); diff --git a/packages/cli-snapshot/test/file.test.js b/packages/cli-snapshot/test/file.test.js index 706ef6956..df019bc2d 100644 --- a/packages/cli-snapshot/test/file.test.js +++ b/packages/cli-snapshot/test/file.test.js @@ -120,7 +120,7 @@ describe('percy snapshot ', () => { '[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 ', () => { '[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 ', () => { '[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' ])); }); diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 93ae938ca..b84ff105e 100644 --- a/packages/core/src/percy.js +++ b/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)); diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js index 8f87001fe..5c9c4bd3d 100644 --- a/packages/core/src/snapshot.js +++ b/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 diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 9a5c13672..bb7263222 100644 --- a/packages/core/src/utils.js +++ b/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}`, diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index a8a30e9c1..354dc1bb7 100644 --- a/packages/core/test/unit/utils.test.js +++ b/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] }); + }); + }); });