From f0e0b4524e8437d725ab12f99bb3894be1dad3c8 Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 11:44:15 -0600 Subject: [PATCH 1/6] test: Add failing test for snapshots being added after finalize --- test/acceptance/asset-discovery.test.js | 26 +++++++++++++++++++++++++ test/acceptance/dummy/snapshot-error.js | 17 ++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 test/acceptance/asset-discovery.test.js create mode 100644 test/acceptance/dummy/snapshot-error.js diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js new file mode 100644 index 00000000..4cc83c8e --- /dev/null +++ b/test/acceptance/asset-discovery.test.js @@ -0,0 +1,26 @@ +import expect from 'expect' +import { execSync } from 'child_process' +import { when } from 'interactor.js' + +import { + launch, + run, + setupApiProxy, + setupDummyApp +} from './helpers' + +describe('Asset discovery', () => { + let proxy = setupApiProxy() + let dummy = setupDummyApp() + + it('does not create snapshots while stopping a build', async () => { + let [stdout] = await run('percy exec -- node ./test/acceptance/dummy/snapshot-error.js'); + + expect(stdout).toHaveEntries([ + '[percy] percy has started.', + "[percy] snapshot taken: 'Home Page - 0'", + '[percy] done.', + '[percy] finalized build #4: <>' + ]) + }) +}) diff --git a/test/acceptance/dummy/snapshot-error.js b/test/acceptance/dummy/snapshot-error.js new file mode 100644 index 00000000..d3944585 --- /dev/null +++ b/test/acceptance/dummy/snapshot-error.js @@ -0,0 +1,17 @@ +// register babel for the snapshot helper +require('@babel/register') +require('regenerator-runtime/runtime') + +const launch = require('../helpers/snapshot').default + +launch(async (page, snapshot) => { + await page.goto('http://localhost:9999') + + setTimeout(() => { + throw new Error('Surprise!') + }, 100) // magic number + + for (let index = 0; index < 10; index++) { + await snapshot(`Home Page - ${index}`) + } +}) From 0ec0345edea0c9def75490db3d0c47fc82b65d44 Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 14:02:31 -0600 Subject: [PATCH 2/6] fix: Wait for asset discovery finish when awaiting on snapshots --- src/services/agent-service.ts | 9 ++++++--- test/acceptance/asset-discovery.test.js | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index a5be406c..c0d69cd6 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -75,6 +75,10 @@ export class AgentService { private async handleSnapshot(request: express.Request, response: express.Response) { profile('agentService.handleSnapshot') + let resolve + let reject + const deferred = new Promise((...args) => [resolve, reject] = args) + this.snapshotCreationPromises.push(deferred) // truncate domSnapshot for the logs if it's very large const rootURL = request.body.url @@ -140,15 +144,14 @@ export class AgentService { this.snapshotService.buildLogResource(snapshotLog), ) - const snapshotCreation = this.snapshotService.create( + this.snapshotService.create( request.body.name, resources, snapshotOptions, request.body.clientInfo, request.body.environmentInfo, - ) + ).then(resolve).catch(reject) - this.snapshotCreationPromises.push(snapshotCreation) logger.info(`snapshot taken: '${request.body.name}'`) profile('agentService.handleSnapshot') diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js index 4cc83c8e..62b544b5 100644 --- a/test/acceptance/asset-discovery.test.js +++ b/test/acceptance/asset-discovery.test.js @@ -16,6 +16,7 @@ describe('Asset discovery', () => { it('does not create snapshots while stopping a build', async () => { let [stdout] = await run('percy exec -- node ./test/acceptance/dummy/snapshot-error.js'); + // TODO: Assert the "waitig for x" output & for no snapshot logs to occur *after* expect(stdout).toHaveEntries([ '[percy] percy has started.', "[percy] snapshot taken: 'Home Page - 0'", From 7ae23f08e0f4b31999e3345b1c73641e2b4ca60b Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 14:05:14 -0600 Subject: [PATCH 3/6] test: Clean up test file imports --- test/acceptance/asset-discovery.test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js index 62b544b5..c4134036 100644 --- a/test/acceptance/asset-discovery.test.js +++ b/test/acceptance/asset-discovery.test.js @@ -1,9 +1,5 @@ import expect from 'expect' -import { execSync } from 'child_process' -import { when } from 'interactor.js' - import { - launch, run, setupApiProxy, setupDummyApp From bf419fe3955de7c920ea224cc9d9422a757a9a50 Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 16:06:03 -0600 Subject: [PATCH 4/6] fix integration tests --- src/services/agent-service.ts | 8 ++++---- src/services/resource-service.ts | 2 +- src/services/snapshot-service.ts | 2 +- test/acceptance/asset-discovery.test.js | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index c0d69cd6..f11d52cc 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -75,10 +75,6 @@ export class AgentService { private async handleSnapshot(request: express.Request, response: express.Response) { profile('agentService.handleSnapshot') - let resolve - let reject - const deferred = new Promise((...args) => [resolve, reject] = args) - this.snapshotCreationPromises.push(deferred) // truncate domSnapshot for the logs if it's very large const rootURL = request.body.url @@ -119,6 +115,10 @@ export class AgentService { logger.info(`snapshot skipped[max_file_size_exceeded]: '${request.body.name}'`) return response.json({ success: true }) } + // tslint:disable-next-line + let resolve, reject + const deferred = new Promise((...args) => [resolve, reject] = args) + this.snapshotCreationPromises.push(deferred) let resources = await this.snapshotService.buildResources( rootURL, diff --git a/src/services/resource-service.ts b/src/services/resource-service.ts index d9346e65..8816ee64 100644 --- a/src/services/resource-service.ts +++ b/src/services/resource-service.ts @@ -47,7 +47,7 @@ export default class ResourceService extends PercyClientService { snapshotResponse.response, snapshotResponse.resources, ) - profile('-> resourceService.uploadMissingResources', {resources: resources.length}) + profile(`-> resourceService.uploadMissingResources ${resources.length}`) return true } catch (error) { diff --git a/src/services/snapshot-service.ts b/src/services/snapshot-service.ts index d40231c6..50be0cc1 100644 --- a/src/services/snapshot-service.ts +++ b/src/services/snapshot-service.ts @@ -110,7 +110,7 @@ export default class SnapshotService extends PercyClientService { profile('-> snapshotService.finalizeSnapshot') await this.finalize(response.body.data.id) - profile('-> snapshotService.finalizeSnapshot', {snapshotId}) + profile(`-> snapshotService.finalizeSnapshot ${snapshotId}`) return response }).catch(logError) diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js index c4134036..64252092 100644 --- a/test/acceptance/asset-discovery.test.js +++ b/test/acceptance/asset-discovery.test.js @@ -12,7 +12,7 @@ describe('Asset discovery', () => { it('does not create snapshots while stopping a build', async () => { let [stdout] = await run('percy exec -- node ./test/acceptance/dummy/snapshot-error.js'); - // TODO: Assert the "waitig for x" output & for no snapshot logs to occur *after* + // TODO: Assert the "waiting for x" output & for no snapshot logs to occur *after* expect(stdout).toHaveEntries([ '[percy] percy has started.', "[percy] snapshot taken: 'Home Page - 0'", From 684089bfa9943deed244bee5c7fc6a46cc951f74 Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 17:27:33 -0600 Subject: [PATCH 5/6] Improve tests --- test/acceptance/asset-discovery.test.js | 6 +++++- test/acceptance/helpers/proxy.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js index 64252092..d00a7a26 100644 --- a/test/acceptance/asset-discovery.test.js +++ b/test/acceptance/asset-discovery.test.js @@ -12,9 +12,13 @@ describe('Asset discovery', () => { it('does not create snapshots while stopping a build', async () => { let [stdout] = await run('percy exec -- node ./test/acceptance/dummy/snapshot-error.js'); - // TODO: Assert the "waiting for x" output & for no snapshot logs to occur *after* + let finalizeReqDate = proxy.requests['/builds/123/finalize'][0].requestDate; + let lastSnapshotReqDate = proxy.requests['/builds/123/snapshots'].pop().requestDate; + + expect(finalizeReqDate > lastSnapshotReqDate).toBe(true) expect(stdout).toHaveEntries([ '[percy] percy has started.', + /^\[percy\] waiting for \d+ snapshots to complete\.\.\.$/m, "[percy] snapshot taken: 'Home Page - 0'", '[percy] done.', '[percy] finalized build #4: <>' diff --git a/test/acceptance/helpers/proxy.js b/test/acceptance/helpers/proxy.js index 9d0234de..33e70d92 100644 --- a/test/acceptance/helpers/proxy.js +++ b/test/acceptance/helpers/proxy.js @@ -50,7 +50,7 @@ export function createApiProxy() { app.all('*', (req, res, next) => { let path = req.path.replace(/\/$/, '') requests[path] = requests[path] || [] - requests[path].push(req) + requests[path].push({ requestDate: new Date(), ...req }) next() }) From 46f10f568ec534dae219679ddefe35ad22a12c8b Mon Sep 17 00:00:00 2001 From: Robert DeLuca Date: Thu, 21 Nov 2019 17:56:21 -0600 Subject: [PATCH 6/6] Code review fixes --- test/acceptance/asset-discovery.test.js | 10 +++++----- test/acceptance/helpers/proxy.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/acceptance/asset-discovery.test.js b/test/acceptance/asset-discovery.test.js index d00a7a26..69addff2 100644 --- a/test/acceptance/asset-discovery.test.js +++ b/test/acceptance/asset-discovery.test.js @@ -9,13 +9,13 @@ describe('Asset discovery', () => { let proxy = setupApiProxy() let dummy = setupDummyApp() - it('does not create snapshots while stopping a build', async () => { + it('waits for snapshot creation before build finalization', async () => { let [stdout] = await run('percy exec -- node ./test/acceptance/dummy/snapshot-error.js'); + let finalizeReqDate = proxy.requests['/builds/123/finalize'][0].timestamp; + let snapshotReqs = proxy.requests['/builds/123/snapshots']; + let lastSnapshotReqDate = snapshotReqs[snapshotReqs.length - 1].timestamp; - let finalizeReqDate = proxy.requests['/builds/123/finalize'][0].requestDate; - let lastSnapshotReqDate = proxy.requests['/builds/123/snapshots'].pop().requestDate; - - expect(finalizeReqDate > lastSnapshotReqDate).toBe(true) + expect(finalizeReqDate).toBeGreaterThan(lastSnapshotReqDate) expect(stdout).toHaveEntries([ '[percy] percy has started.', /^\[percy\] waiting for \d+ snapshots to complete\.\.\.$/m, diff --git a/test/acceptance/helpers/proxy.js b/test/acceptance/helpers/proxy.js index 33e70d92..b569ed28 100644 --- a/test/acceptance/helpers/proxy.js +++ b/test/acceptance/helpers/proxy.js @@ -50,7 +50,7 @@ export function createApiProxy() { app.all('*', (req, res, next) => { let path = req.path.replace(/\/$/, '') requests[path] = requests[path] || [] - requests[path].push({ requestDate: new Date(), ...req }) + requests[path].push({ timestamp: Date.now(), ...req }) next() })