diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 0821047db..2ea37424a 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -127,17 +127,15 @@ export default class Percy { let meta = { build: { id: build.id } }; log.info('Stopping percy...', meta); - // clear queued page captures and wait for any pending - if (this.#captures.clear()) { - log.info(`Waiting for ${this.#captures.length} page(s) to complete`, meta); - await this.#captures.idle(); + // log about queued captures or uploads + if (this.#captures.length) { + log.info(`Waiting for ${this.#captures.length} page(s) to finish snapshotting`, meta); + } else if (this.#snapshots.length) { + log.info(`Waiting for ${this.#snapshots.length} snapshot(s) to finish uploading`, meta); } - // clear queued snapshots and wait for any pending - if (this.#snapshots.clear()) { - log.info(`Waiting for ${this.#snapshots.length} snapshot(s) to complete`, meta); - await this.#snapshots.idle(); - } + // wait for any queued captures or snapshots + await this.idle(); // close the server and browser this.server?.close(); @@ -161,8 +159,9 @@ export default class Percy { } // Handles asset discovery for the URL and DOM snapshot at each requested - // width with the provided options. Resolves when the snapshot is complete, - // although shouldn't be awaited on as snapshots are handled concurrently. + // width with the provided options. Resolves when the snapshot has been taken + // and asset discovery is finished, but does not gaurantee that the snapshot + // will be succesfully uploaded. snapshot({ url, name, @@ -197,68 +196,70 @@ export default class Percy { // fallback to instance enable JS flag enableJavaScript = enableJavaScript ?? this.config.snapshot.enableJavaScript ?? false; - // add this snapshot task to the snapshot queue - return this.#snapshots.push(async () => { - let meta = { - snapshot: { name }, - build: { id: this.client.build.id } - }; - - log.debug('---------'); - log.debug('Handling snapshot:', meta); - log.debug(`-> name: ${name}`, meta); - log.debug(`-> url: ${url}`, meta); - log.debug(`-> widths: ${widths.join('px, ')}px`, meta); - log.debug(`-> clientInfo: ${clientInfo}`, meta); - log.debug(`-> environmentInfo: ${environmentInfo}`, meta); - log.debug(`-> requestHeaders: ${JSON.stringify(requestHeaders)}`, meta); - log.debug(`-> domSnapshot:\n${( - domSnapshot.length <= 1024 ? domSnapshot - : (domSnapshot.substr(0, 1024) + '... [truncated]') - )}`, meta); - - try { - // inject Percy CSS - let [percyDOM, percyCSSResource] = injectPercyCSS(url, domSnapshot, percyCSS, meta); - // use a map so resources remain unique by url - let resources = new Map([[url, createRootResource(url, percyDOM)]]); - - // gather resources at each width concurrently - await Promise.all(widths.map(width => ( - this.discoverer.gatherResources(resources, { - rootUrl: url, - rootDom: domSnapshot, - enableJavaScript, - requestHeaders, - width, - meta - }) - ))); - - // convert resource map to array - resources = Array.from(resources.values()); - // include the Percy CSS resource if there was one - if (percyCSSResource) resources.push(percyCSSResource); - // include a log resource for debugging - let logs = await log.query({ filter: ({ snapshot: s }) => s?.name === name }); - resources.push(createLogResource(logs)); - - // create, upload, and finalize the snapshot - await this.client.sendSnapshot({ - name, - widths, - minimumHeight, + // useful meta info for the logfile + let meta = { + snapshot: { name }, + build: { id: this.client.build.id } + }; + + log.debug('---------'); + log.debug('Handling snapshot:', meta); + log.debug(`-> name: ${name}`, meta); + log.debug(`-> url: ${url}`, meta); + log.debug(`-> widths: ${widths.join('px, ')}px`, meta); + log.debug(`-> clientInfo: ${clientInfo}`, meta); + log.debug(`-> environmentInfo: ${environmentInfo}`, meta); + log.debug(`-> requestHeaders: ${JSON.stringify(requestHeaders)}`, meta); + log.debug(`-> domSnapshot:\n${( + domSnapshot.length <= 1024 ? domSnapshot + : (domSnapshot.substr(0, 1024) + '... [truncated]') + )}`, meta); + + // use a promise as a try-catch so we can do the remaining work + // asynchronously, but perform the above synchronously + return Promise.resolve().then(async () => { + // inject Percy CSS + let [percyDOM, percyCSSResource] = injectPercyCSS(url, domSnapshot, percyCSS, meta); + // use a map so resources remain unique by url + let resources = new Map([[url, createRootResource(url, percyDOM)]]); + // include the Percy CSS resource if there was one + if (percyCSSResource) resources.set('percy-css', percyCSSResource); + + // gather resources at each width concurrently + await Promise.all(widths.map(width => ( + this.discoverer.gatherResources(resources, { + rootUrl: url, + rootDom: domSnapshot, enableJavaScript, - clientInfo, - environmentInfo, - resources - }); - - log.info(`Snapshot taken: ${name}`, meta); - } catch (error) { - log.error(`Encountered an error for snapshot: ${name}`, meta); + requestHeaders, + width, + meta + }) + ))); + + // include a log resource for debugging + let logs = await log.query({ filter: ({ snapshot: s }) => s?.name === name }); + resources.set('percy-logs', createLogResource(logs)); + + // log that the snapshot has been taken before uploading it + log.info(`Snapshot taken: ${name}`, meta); + + // upload within the async snapshot queue + this.#snapshots.push(() => this.client.sendSnapshot({ + name, + widths, + minimumHeight, + enableJavaScript, + clientInfo, + environmentInfo, + resources: Array.from(resources.values()) + }).catch(error => { + log.error(`Encountered an error uploading snapshot: ${name}`, meta); log.error(error); - } + })); + }).catch(error => { + log.error(`Encountered an error taking snapshot: ${name}`, meta); + log.error(error); }); } @@ -276,6 +277,7 @@ export default class Percy { snapshots = name ? [{ name, execute }].concat(snapshots) : snapshots; assert(snapshots.length && snapshots.every(s => s.name), `Missing name for ${url}`); + // the entire capture process happens within the async capture queue return this.#captures.push(async () => { let results = []; let page; @@ -315,7 +317,7 @@ export default class Percy { log.error(`Encountered an error for page: ${url}`); log.error(error); } finally { - // awaiting on resulting snapshots syncs this task with those snapshot tasks + // await on any resulting snapshots await Promise.all(results); await page?.close(); } diff --git a/packages/core/src/server.js b/packages/core/src/server.js index 5346822f9..ef8d46c08 100644 --- a/packages/core/src/server.js +++ b/packages/core/src/server.js @@ -33,10 +33,9 @@ export function createServerApp(percy) { .get('/percy/dom.js', (_, res) => { res.sendFile(require.resolve('@percy/dom')); }) - // snapshot requests are concurrent by default + // forward snapshot requests .post('/percy/snapshot', asyncRoute(async (req, res) => { - let snapshot = percy.snapshot(req.body); - if (req.body.concurrent === false) await snapshot; + await percy.snapshot(req.body); res.json({ success: true }); })) // stops the instance diff --git a/packages/core/test/asset-discovery.test.js b/packages/core/test/asset-discovery.test.js index f4cb0b1f9..744e52549 100644 --- a/packages/core/test/asset-discovery.test.js +++ b/packages/core/test/asset-discovery.test.js @@ -74,6 +74,7 @@ describe('Asset Discovery', () => { domSnapshot: testDOM }); + await percy.idle(); let paths = server.requests.map(r => r.path); // does not request the root url (serves domSnapshot instead) expect(paths).not.toContain('/'); @@ -116,6 +117,7 @@ describe('Asset Discovery', () => { domSnapshot: prefetchDOM }); + await percy.idle(); let paths = server.requests.map(r => r.path); expect(paths).toContain('/style.css'); @@ -150,6 +152,7 @@ describe('Asset Discovery', () => { domSnapshot: dataUrlDOM }); + await percy.idle(); expect(captured[0]).not.toEqual(expect.arrayContaining([ expect.objectContaining({ attributes: expect.objectContaining({ @@ -170,6 +173,7 @@ describe('Asset Discovery', () => { domSnapshot: testDOM.replace('style.css', 'stylesheet.css') }); + await percy.idle(); let paths = server.requests.map(r => r.path); expect(paths).toContain('/stylesheet.css'); expect(paths).toContain('/style.css'); @@ -199,6 +203,7 @@ describe('Asset Discovery', () => { }) )); + await percy.idle(); expect(captured[0]).toEqual([ expect.objectContaining({ attributes: expect.objectContaining({ @@ -299,6 +304,8 @@ describe('Asset Discovery', () => { url: 'http://localhost:8000', domSnapshot: testDOM }); + + await percy.idle(); }; it('caches resource requests', async () => { @@ -407,6 +414,7 @@ describe('Asset Discovery', () => { domSnapshot: testExternalDOM }); + await percy.idle(); let paths = server.requests.map(r => r.path); expect(paths).toContain('/style.css'); expect(paths).not.toContain('/img.gif'); @@ -450,6 +458,7 @@ describe('Asset Discovery', () => { widths: [1000] }); + await percy.idle(); expect(captured[0][3]).toEqual( expect.objectContaining({ attributes: expect.objectContaining({ @@ -477,6 +486,7 @@ describe('Asset Discovery', () => { widths: [1000] }); + await percy.idle(); expect(captured[0][3]).toEqual( expect.objectContaining({ attributes: expect.objectContaining({ @@ -504,6 +514,7 @@ describe('Asset Discovery', () => { widths: [1000] }); + await percy.idle(); expect(captured[0]).toEqual([ expect.objectContaining({ attributes: expect.objectContaining({ diff --git a/packages/core/test/percy-css.test.js b/packages/core/test/percy-css.test.js index 805635bca..300ac964f 100644 --- a/packages/core/test/percy-css.test.js +++ b/packages/core/test/percy-css.test.js @@ -42,6 +42,7 @@ describe('Percy CSS', () => { domSnapshot: testDOM }); + await percy.idle(); let resources = mockAPI.requests['/builds/123/snapshots'][0] .body.data.relationships.resources.data; @@ -58,6 +59,7 @@ describe('Percy CSS', () => { percyCSS: 'body { color: purple; }' }); + await percy.idle(); let resources = mockAPI.requests['/builds/123/snapshots'][0] .body.data.relationships.resources.data; @@ -73,6 +75,7 @@ describe('Percy CSS', () => { percyCSS: 'p { font-size: 2rem; }' }); + await percy.idle(); let resources = mockAPI.requests['/builds/123/snapshots'][0] .body.data.relationships.resources.data; diff --git a/packages/core/test/percy.test.js b/packages/core/test/percy.test.js index 44219bd92..8cd936912 100644 --- a/packages/core/test/percy.test.js +++ b/packages/core/test/percy.test.js @@ -208,8 +208,7 @@ describe('Percy', () => { }); it('logs when stopping with pending snapshots', async () => { - // not awaited on so it becomes pending - percy.snapshot({ + await percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', domSnapshot: '', @@ -222,8 +221,7 @@ describe('Percy', () => { expect(stdio[2]).toHaveLength(0); expect(stdio[1]).toEqual([ '[percy] Stopping percy...\n', - '[percy] Waiting for 1 snapshot(s) to complete\n', - '[percy] Snapshot taken: test snapshot\n', + '[percy] Waiting for 1 snapshot(s) to finish uploading\n', '[percy] Finalized build #1: https://percy.io/test/test/123\n', '[percy] Done!\n' ]); @@ -244,7 +242,7 @@ describe('Percy', () => { expect(stdio[2]).toHaveLength(0); expect(stdio[1]).toEqual([ '[percy] Stopping percy...\n', - '[percy] Waiting for 1 page(s) to complete\n', + '[percy] Waiting for 1 page(s) to finish snapshotting\n', '[percy] Snapshot taken: test snapshot\n', '[percy] Finalized build #1: https://percy.io/test/test/123\n', '[percy] Done!\n' @@ -287,21 +285,18 @@ describe('Percy', () => { }); it('resolves after snapshots idle', async () => { - // not awaited on so it becomes pending - percy.snapshot({ + await percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', domSnapshot: '', widths: [1000] }); - percy.loglevel('info'); - await stdio.capture(() => percy.idle()); + expect(mockAPI.requests['/builds/123/snapshots']).toBeUndefined(); - expect(stdio[2]).toHaveLength(0); - expect(stdio[1]).toEqual([ - '[percy] Snapshot taken: test snapshot\n' - ]); + await percy.idle(); + + expect(mockAPI.requests['/builds/123/snapshots']).toHaveLength(1); }); }); @@ -338,12 +333,13 @@ describe('Percy', () => { }); it('creates a new snapshot for the build', async () => { - await expect(percy.snapshot({ + await percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', domSnapshot: testDOM - })).resolves.toBeUndefined(); + }); + await percy.idle(); expect(mockAPI.requests['/builds/123/snapshots'][0].body).toEqual({ data: { type: 'snapshots', @@ -395,12 +391,13 @@ describe('Percy', () => { }); it('uploads missing resources for the snapshot', async () => { - await expect(percy.snapshot({ + await percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', domSnapshot: testDOM - })).resolves.toBeUndefined(); + }); + await percy.idle(); expect(mockAPI.requests['/builds/123/resources']).toHaveLength(4); expect(mockAPI.requests['/builds/123/resources'].map(r => r.body)).toEqual( expect.arrayContaining([{ @@ -441,12 +438,13 @@ describe('Percy', () => { }); it('finalizes the snapshot', async () => { - await expect(percy.snapshot({ + await percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', domSnapshot: testDOM - })).resolves.toBeUndefined(); + }); + await percy.idle(); expect(mockAPI.requests['/snapshots/4567/finalize']).toBeDefined(); }); @@ -480,25 +478,47 @@ describe('Percy', () => { ]); }); - it('logs any encountered errors', async () => { - mockAPI.reply('/builds/123/snapshots', () => [401, { - errors: [{ detail: 'snapshot error' }] - }]); - + it('logs any encountered errors when snapshotting', async () => { await stdio.capture(() => ( percy.snapshot({ name: 'test snapshot', url: 'http://localhost:8000', - domSnapshot: testDOM + domSnapshot: testDOM, + // sabatoge an array to cause an unexpected error + widths: Object.assign([1000], { + map: () => { throw new Error('snapshot error'); } + }) }) )); expect(stdio[1]).toHaveLength(0); expect(stdio[2]).toEqual([ - '[percy] Encountered an error for snapshot: test snapshot\n', + '[percy] Encountered an error taking snapshot: test snapshot\n', '[percy] Error: snapshot error\n' ]); }); + + it('logs any encountered errors when uploading', async () => { + mockAPI.reply('/builds/123/snapshots', () => [401, { + errors: [{ detail: 'snapshot upload error' }] + }]); + + await percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM + }); + + await stdio.capture(() => ( + percy.idle() + )); + + expect(stdio[1]).toHaveLength(0); + expect(stdio[2]).toEqual([ + '[percy] Encountered an error uploading snapshot: test snapshot\n', + '[percy] Error: snapshot upload error\n' + ]); + }); }); describe('#capture()', () => { @@ -535,6 +555,7 @@ describe('Percy', () => { url: 'http://localhost:8000' })); + await percy.idle(); expect(Buffer.from(( mockAPI.requests['/builds/123/resources'][0] .body.data.attributes['base64-content'] @@ -552,6 +573,7 @@ describe('Percy', () => { waitFor: '#test' })); + await percy.idle(); expect(Buffer.from(( mockAPI.requests['/builds/123/resources'][0] .body.data.attributes['base64-content'] @@ -568,6 +590,7 @@ describe('Percy', () => { } })); + await percy.idle(); expect(Buffer.from(( mockAPI.requests['/builds/123/resources'][0] .body.data.attributes['base64-content'] diff --git a/packages/core/test/server.test.js b/packages/core/test/server.test.js index 84505a4a5..450759997 100644 --- a/packages/core/test/server.test.js +++ b/packages/core/test/server.test.js @@ -73,14 +73,9 @@ describe('Snapshot Server', () => { expect(percy.stop.calls).toHaveLength(1); }); - it('has a /snapshot endpoint that calls #snapshot() concurrently', async () => { - let done = false; - + it('has a /snapshot endpoint that calls #snapshot()', async () => { await percy.start(); - percy.snapshot = data => { - (percy.snapshot.calls = percy.snapshot.calls || []).push(data); - return new Promise(r => setTimeout(r, 10)).then(() => (done = true)); - }; + percy.snapshot = data => (percy.snapshot.calls = percy.snapshot.calls || []).push(data); let response = await fetch('http://localhost:1337/percy/snapshot', { method: 'post', @@ -91,28 +86,6 @@ describe('Snapshot Server', () => { await expect(response.json()).resolves.toEqual({ success: true }); expect(percy.snapshot.calls).toHaveLength(1); expect(percy.snapshot.calls[0]).toEqual({ test: true }); - expect(done).toBe(false); - }); - - it('waits for /snapshot completion when `concurrent` is false', async () => { - let done = false; - - await percy.start(); - percy.snapshot = data => { - (percy.snapshot.calls = percy.snapshot.calls || []).push(data); - return new Promise(r => setTimeout(r, 10)).then(() => (done = true)); - }; - - let response = await fetch('http://localhost:1337/percy/snapshot', { - method: 'post', - headers: { 'Content-Type': 'application/json' }, - body: '{ "concurrent": false }' - }); - - await expect(response.json()).resolves.toEqual({ success: true }); - expect(percy.snapshot.calls).toHaveLength(1); - expect(percy.snapshot.calls[0]).toEqual({ concurrent: false }); - expect(done).toBe(true); }); it('returns a 500 error when an endpoint throws', async () => {