diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 7fac93237..5b8310260 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -18,6 +18,7 @@ export function createPercyServer(percy, port) { // return json errors return next().catch(e => res.json(e.status ?? 500, { + build: percy.build, error: e.message, success: false })); diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 7cc5c1309..f98b472ec 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -168,7 +168,8 @@ export class Percy { // handle deferred build errors if (this.deferUploads) { buildTask.catch(err => { - this.log.error('Failed to create build'); + this.build = { error: 'Failed to create build' }; + this.log.error(this.build.error); this.log.error(err); this.close(); }); @@ -281,7 +282,7 @@ export class Percy { if (this.build?.failed) { // do not finalize failed builds this.log.warn(`Build #${this.build.number} failed: ${this.build.url}`, meta); - } else if (this.build) { + } else if (this.build?.id) { // finalize the build await this.client.finalizeBuild(this.build.id); this.log.info(`Finalized build #${this.build.number}: ${this.build.url}`, meta); @@ -311,10 +312,9 @@ export class Percy { snapshot(options) { if (this.readyState !== 1) { throw new Error('Not running'); - } - - // handle multiple snapshots - if (Array.isArray(options)) { + } 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))); } @@ -351,6 +351,10 @@ export class Percy { // Queues a snapshot upload with the provided options _scheduleUpload(name, options) { + if (this.build?.error) { + throw new Error(this.build.error); + } + return this.#uploads.push(`upload/${name}`, async () => { try { /* istanbul ignore if: useful for other internal packages */ @@ -367,6 +371,7 @@ export class Percy { // build failed at some point, stop accepting snapshots if (failed) { + this.build.error = failed.detail; this.build.failed = true; this.close(); } diff --git a/packages/core/src/queue.js b/packages/core/src/queue.js index 25bdf28d3..9551f859b 100644 --- a/packages/core/src/queue.js +++ b/packages/core/src/queue.js @@ -15,9 +15,8 @@ export class Queue { } push(id, callback, priority) { - if (this.closed && !id.startsWith('@@/')) { - throw new Error('Closed'); - } + /* istanbul ignore next: race condition paranoia */ + if (this.closed && !id.startsWith('@@/')) return; this.cancel(id); diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index afd001d09..79518d427 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -155,7 +155,11 @@ describe('API Server', () => { let [data, res] = await request('/percy/snapshot', 'POST', true); expect(res.statusCode).toBe(500); - expect(data).toEqual({ success: false, error: 'test error' }); + expect(data).toEqual({ + build: percy.build, + error: 'test error', + success: false + }); }); it('returns a 404 for any other endpoint', async () => { @@ -163,7 +167,11 @@ describe('API Server', () => { let [data, res] = await request('/foobar', true); expect(res.statusCode).toBe(404); - expect(data).toEqual({ success: false, error: 'Not Found' }); + expect(data).toEqual({ + build: percy.build, + error: 'Not Found', + success: false + }); }); it('facilitates logger websocket connections', async () => { diff --git a/packages/core/test/percy.test.js b/packages/core/test/percy.test.js index 161920458..9c5c9ef1b 100644 --- a/packages/core/test/percy.test.js +++ b/packages/core/test/percy.test.js @@ -363,7 +363,7 @@ describe('Percy', () => { expect(() => percy.snapshot({ name: 'Snapshot 2', url: 'http://localhost:8000' - })).toThrowError('Closed'); + })).toThrowError('Failed to create build'); expect(logger.stdout).toEqual([ '[percy] Percy has started!', @@ -374,6 +374,47 @@ describe('Percy', () => { '[percy] Error: build error' ]); }); + + it('stops accepting snapshots when an in-progress build fails', async () => { + mockAPI.reply('/builds/123/snapshots', () => [422, { + errors: [{ + detail: 'Build has failed', + source: { pointer: '/data/attributes/build' } + }] + }]); + + // create a new instance with default concurrency + percy = new Percy({ token: 'PERCY_TOKEN', snapshot: { widths: [1000] } }); + await percy.start(); + + await Promise.all([ + // upload will eventually fail + percy.snapshot({ + url: 'http://localhost:8000/snapshot-1' + }), + // should not upload + percy.snapshot({ + url: 'http://localhost:8000/snapshot-2', + // delay this snapshot so the first upload can fail + waitForTimeout: 100 + }) + ]); + + await percy.idle(); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] Encountered an error uploading snapshot: /snapshot-1', + '[percy] Error: Build has failed' + ])); + + expect(mockAPI.requests['/builds/123/snapshots'].length).toEqual(1); + + // stops accepting snapshots + expect(() => percy.snapshot({ + name: 'Snapshot 2', + url: 'http://localhost:8000' + })).toThrowError('Build has failed'); + }); }); describe('#stop()', () => { diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index 7e96aea0e..35ca717ee 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -45,18 +45,16 @@ describe('Snapshot', () => { }); it('warns when missing additional snapshot names', async () => { - percy.close(); // close queues so snapshots fail - - expect(() => percy.snapshot({ - url: 'http://foo', + await percy.snapshot({ + url: 'http://localhost:8000', additionalSnapshots: [{ - waitForTimeout: 1000 + waitForTimeout: 10 }, { name: 'nombre', suffix: ' - 1', - waitForTimeout: 1000 + waitForTimeout: 10 }] - })).toThrow(); + }); expect(logger.stderr).toEqual([ '[percy] Invalid snapshot options:', @@ -65,10 +63,8 @@ describe('Snapshot', () => { ]); }); - it('warns when providing conflicting options', () => { - percy.close(); // close queues so snapshots fail - - expect(() => percy.snapshot({ + it('warns when providing conflicting options', async () => { + await percy.snapshot({ url: 'http://a', domSnapshot: 'b', waitForTimeout: 3, @@ -77,7 +73,7 @@ describe('Snapshot', () => { additionalSnapshots: [ { prefix: 'f' } ] - })).toThrow(); + }); expect(logger.stderr).toEqual([ '[percy] Invalid snapshot options:', @@ -88,10 +84,8 @@ describe('Snapshot', () => { ]); }); - it('warns if options are invalid', () => { - percy.close(); // close queues so snapshots fail - - expect(() => percy.snapshot({ + it('warns if options are invalid', async () => { + await percy.snapshot({ name: 'invalid snapshot', url: 'http://localhost:8000', widths: ['not-a-width'], @@ -103,7 +97,7 @@ describe('Snapshot', () => { 'finally.a-real.hostname.org' ] } - })).toThrow(); + }); expect(logger.stderr).toEqual([ '[percy] Invalid snapshot options:', @@ -114,12 +108,12 @@ describe('Snapshot', () => { ]); }); - it('warns on deprecated options', () => { - percy.close(); // close queues so snapshots fail - - expect(() => percy.snapshot({ url: 'http://a', requestHeaders: { foo: 'bar' } })).toThrow(); - expect(() => percy.snapshot({ url: 'http://b', authorization: { username: 'foo' } })).toThrow(); - expect(() => percy.snapshot({ url: 'http://c', snapshots: [{ name: 'foobar' }] })).toThrow(); + it('warns on deprecated options', async () => { + await percy.snapshot([ + { url: 'http://localhost:8000/a', requestHeaders: { foo: 'bar' } }, + { url: 'http://localhost:8000/b', authorization: { username: 'foo' } }, + { url: 'http://localhost:8000/c', snapshots: [{ name: 'foobar' }] } + ]); expect(logger.stderr).toEqual([ '[percy] Warning: The snapshot option `requestHeaders` ' + diff --git a/packages/sdk-utils/src/post-snapshot.js b/packages/sdk-utils/src/post-snapshot.js index f7db54024..cf1ef7734 100644 --- a/packages/sdk-utils/src/post-snapshot.js +++ b/packages/sdk-utils/src/post-snapshot.js @@ -7,7 +7,7 @@ export async function postSnapshot(options, params) { let query = params ? `?${new URLSearchParams(params)}` : ''; await request.post(`/percy/snapshot${query}`, options).catch(err => { - if (err.response && err.message === 'Closed') { + if (err.response?.body?.build?.error) { percy.enabled = false; } else { throw err; diff --git a/packages/sdk-utils/test/helpers.js b/packages/sdk-utils/test/helpers.js index c909bbdba..da2258c6a 100644 --- a/packages/sdk-utils/test/helpers.js +++ b/packages/sdk-utils/test/helpers.js @@ -17,7 +17,7 @@ const helpers = { teardown: () => helpers.call('server.close'), getRequests: () => helpers.call('server.requests'), testReply: (path, reply) => helpers.call('server.reply', path, reply), - testFailure: (path, error) => helpers.call('server.test.failure', path, error), + testFailure: (...args) => helpers.call('server.test.failure', ...args), testError: path => helpers.call('server.test.error', path), testSerialize: fn => !fn ? helpers.call('server.test.serialize') // get diff --git a/packages/sdk-utils/test/index.test.js b/packages/sdk-utils/test/index.test.js index 3fc10d02d..60f4593e2 100644 --- a/packages/sdk-utils/test/index.test.js +++ b/packages/sdk-utils/test/index.test.js @@ -118,7 +118,8 @@ describe('SDK Utils', () => { }); it('returns false if a snapshot is sent when the API is closed', async () => { - await helpers.testFailure('/percy/snapshot', 'Closed'); + let error = 'Build failed'; + await helpers.testFailure('/percy/snapshot', error, { build: { error } }); await expectAsync(isPercyEnabled()).toBeResolvedTo(true); await expectAsync(utils.postSnapshot({})).toBeResolved(); await expectAsync(isPercyEnabled()).toBeResolvedTo(false); @@ -162,8 +163,9 @@ describe('SDK Utils', () => { }); it('disables snapshots when the API is closed', async () => { + let error = 'Build failed'; utils.percy.enabled = true; - await helpers.testFailure('/percy/snapshot', 'Closed'); + await helpers.testFailure('/percy/snapshot', error, { build: { error } }); await expectAsync(postSnapshot({})).toBeResolved(); expect(utils.percy.enabled).toEqual(false); }); diff --git a/packages/sdk-utils/test/server.js b/packages/sdk-utils/test/server.js index 280736f5e..02874e669 100644 --- a/packages/sdk-utils/test/server.js +++ b/packages/sdk-utils/test/server.js @@ -72,8 +72,8 @@ function context() { test: { get serialize() { return serializeDOM; }, set serialize(fn) { return (serializeDOM = fn); }, - failure: (path, error) => ctx.server.reply(path, () => ( - [500, 'application/json', { success: false, error }])), + failure: (path, error, o) => ctx.server.reply(path, () => ( + [500, 'application/json', { success: false, error, ...o }])), error: path => ctx.server.reply(path, r => r.connection.destroy()), remote: () => (allowSocketConnections = true) }