🐛 Only delay uploads when actively running (#1009)

* ✅ Fix client test helper spy strategy retention

When mocking requests, withArgs will return any existing spy strategy before creating a new
one. However it seems for arguments based on asymmetricMatch, the existing spy strategy never
matches itself. This makes it impossible to provide alternative mock options after already mocking a
request. While this is potentially due to our asymmetricMatch not being class-based, we can work
around it by keeping a reference to existing strategies based on the provided mock URL. When new
spies are created, old strategies are discarded to also create new ones.

* 🐛 Only delay uploads when actively running

When stopping, uploads should not pause the queue and should be processed normally as dictated by
the queue's concurrency.

* 🐛 Do not process delayed uploads before build creation

packages/client/test/helpers.js

@@ -79,17 +79,19 @@ export async function mockRequests(baseUrl, defaultReply = () => [200]) {
   if (!jasmine.isSpy(http.request)) {
     spyOn(http, 'request').and.callFake((...a) => new MockRequest(null, ...a));
     spyOn(http, 'get').and.callFake((...a) => new MockRequest(null, ...a).end());
+    mockRequests.spies = new Map();
   }
 
   let any = jasmine.anything();
-  let match = o => o.hostname === hostname &&
-    (o.path ?? o.pathname).startsWith(pathname);
+  let match = o => o.hostname === hostname && (o.path ?? o.pathname).startsWith(pathname);
   let reply = jasmine.createSpy('reply').and.callFake(defaultReply);
+  let spies = mockRequests.spies.get(baseUrl) ?? {};
 
-  http.request.withArgs({ asymmetricMatch: match })
-    .and.callFake((...a) => new MockRequest(reply, ...a));
-  http.get.withArgs({ asymmetricMatch: u => match(new URL(u)) }, any, any)
-    .and.callFake((...a) => new MockRequest(reply, ...a).end());
+  spies.request = spies.request ?? http.request.withArgs({ asymmetricMatch: match });
+  spies.request.and.callFake((...a) => new MockRequest(reply, ...a));
+  spies.get = spies.get ?? http.get.withArgs({ asymmetricMatch: u => match(new URL(u)) }, any, any);
+  spies.get.and.callFake((...a) => new MockRequest(reply, ...a).end());
+  mockRequests.spies.set(baseUrl, spies);
 
   return reply;
 }

packages/core/src/percy.js

@@ -82,10 +82,6 @@ export class Percy {
       this.#snapshots.concurrency = concurrency;
     }
 
-    if (this.delayUploads) {
-      this.#uploads.concurrency = 1;
-    }
-
     this.client = new PercyClient({ token, clientInfo, environmentInfo });
     if (server) this.server = createPercyServer(this, port);
     this.browser = new Browser(this);
@@ -164,12 +160,13 @@ export class Percy {
     let buildTask = this.#uploads.push('build/create', () => {
       // pause other queued tasks until after the build is created
       this.#uploads.stop();
+      this.build = {};
 
       return this.client.createBuild()
         .then(({ data: { id, attributes } }) => {
-          this.build = { id };
-          this.build.number = attributes['build-number'];
-          this.build.url = attributes['web-url'];
+          let url = attributes['web-url'];
+          let number = attributes['build-number'];
+          Object.assign(this.build, { id, url, number });
           if (!this.delayUploads) this.#uploads.run();
         });
     }, 0);
@@ -245,6 +242,8 @@ export class Percy {
 
         // prevent creating an empty build when deferred
         if (!this.deferUploads || !this.#uploads.has('build/create') || this.#uploads.size > 1) {
+          if (this.build && !this.build.id) yield* this.#uploads.idle();
+
           yield* this.#uploads.flush(s => {
             // do not log a count when not closing or while creating a build
             if (!close || this.#uploads.has('build/create')) return;
@@ -446,11 +445,13 @@ export class Percy {
     if (this.build?.error) throw new Error(this.build.error);
 
     // maybe process any existing delayed uploads
-    if (!this.skipUploads && this.delayUploads) this.#uploads.run();
+    if (!this.skipUploads && this.delayUploads && (
+      !this.build || this.build.id
+    )) this.#uploads.run();
 
     return this.#uploads.push(`upload/${name}`, async () => {
       // when delayed, stop the queue before other uploads are processed
-      if (this.delayUploads) this.#uploads.stop();
+      if (this.readyState < 2 && this.delayUploads) this.#uploads.stop();
 
       try {
         /* istanbul ignore if: useful for other internal packages */

packages/core/test/snapshot.test.js

@@ -239,6 +239,46 @@ describe('Snapshot', () => {
     expect(api.requests['/builds/123/snapshots']).toBeUndefined();
   });
 
+  it('uploads remaining snapshots at the end when delayed', async () => {
+    // stop and recreate a percy instance with the desired option
+    await percy.stop(true);
+    await api.mock({ delay: 100 });
+
+    percy = await Percy.start({
+      token: 'PERCY_TOKEN',
+      discovery: { concurrency: 1 },
+      delayUploads: true
+    });
+
+    // delay build creation to ensure uploads are queued whens stopping
+    let resolveBuild;
+
+    api.reply('/builds', () => new Promise(resolve => {
+      resolveBuild = () => resolve(api.DEFAULT_REPLIES['/builds']());
+    }));
+
+    // take several snapshots before resolving the build
+    await Promise.all(Array.from({ length: 10 }, (_, i) => (
+      percy.snapshot(`http://localhost:8000/${i}`)
+    )));
+
+    // resolve after stopping to test that uploads don't start before build creation
+    setTimeout(() => resolveBuild(), 100);
+    await percy.stop();
+
+    // all uploaded after stopping
+    expect(api.requests['/builds']).toBeDefined();
+    expect(api.requests['/builds/123/snapshots']).toHaveSize(10);
+    expect(api.requests['/builds/123/finalize']).toBeDefined();
+
+    let roots = api.requests['/builds/123/snapshots'].map(s =>
+      s.body.data.relationships.resources.data.find(r => r.attributes['is-root']));
+
+    for (let i = 0; i < 10; i++) {
+      expect(roots[i]).toHaveProperty('attributes.resource-url', `http://localhost:8000/${i}`);
+    }
+  });
+
   it('uploads all snapshots at the end when deferred', async () => {
     // stop and recreate a percy instance with the desired option
     await percy.stop(true);