Wait for all resources to load before indicating Watcher is idle

[jwir3]

Issue: CAP-1082

What Changed

This adds a set of promises to the Watcher class that represent network requests made by the client prior to the archive being created. For each of the network requests, as long as it is not ignored (i.e. within the baseUrl), the promise will resolve when the network request resolves. Once all of these promises are resolved, OR when a 4s timer has expired, the Watcher will indicate it is idle.

How to test

You can run the tests using yarn test. A new test was added with a large image that takes an amount of time to load. Alternatively, run the test-archiver after having linked a project that can utilize it with yarn link.

Change Type

• maintenance
• documentation
• patch - This should introduce no new behavior and instead should be backward compatible.
• minor
• major

📦 Published PR as canary version: 0.0.25--canary.16.982fd10.0

✨ Test out this PR locally via:

npm install @chromaui/test-archiver@0.0.25--canary.16.982fd10.0
# or 
yarn add @chromaui/test-archiver@0.0.25--canary.16.982fd10.0

[linear]

CAP-1082 Wait for all resources to load

Right now we simply wait 1s after takeArchive is called.

[tmeasday]

I think you missed an important but hairy case -- when a request triggers a second request.

Also I think we need to figure out a way to test this (the timeouts and the cascading requests). Might be something useful in the capture tests for that.

[tmeasday]

Should we add the unresolved requests/promises to the log?

[jwir3]

Yes, I think that's a good idea. I'll add logging information for this.

[tmeasday]

What happens if a request creates further requests in resolving?

I think probably we need to do something more complicated, ie check at the end of each request if "all requests are resolved".

[jwir3]

Ok, I can add this logic. I had another concern, too, that Promise.all might resolve before any of the requests are actually added to the set. I suppose I could add another 500ms wait at the very beginning, but this seems like the incorrect solution.

[tmeasday]

I think these names could be more descriptive. Also why is the second one string=>any?

I'm not sure I quite see why they need to be separate lists either? But maybe I am missing something (if so can you put a comment explaining ;) )

[jwir3]

The first one holds Promises, but the second one holds functions that resolve Promises. They probably don't need to be separate lists - I'm not 100% sure why I separated them, other than I think I was experimenting with a different way of doing some of these. Honestly, I don't think that the unsettledResolvers really needs to exist. I'll refactor this a bit and see if I can coalesce them.

[tmeasday]

Probably worth pulling this out to a helper to make it clearer, maybe something like:

const complete = this.registerUnfulfilledRequest(request.url);

// later
complete();

Then the registerUnfulfilledRequest could look something like:

registerUnfulfilledRequest() {
  const complete = // something similar to what you have
 
  complete.then(() => {
     if (/* all requests are resolved */) {
       this.allRequestsDoneComplete(); // this triggers a "super" promise representing all requests
     }
   }

  return complete;
}

[jwir3]

Also I think we need to figure out a way to test this (the timeouts and the cascading requests). Might be something useful in the capture tests for that.

How would you recommend approaching the testing of this? I can look through the capture tests, which is a good idea, but my naive initial idea was to add something like this to the express server:

      app.get('/timeout', async (req, res) => {
        await setTimeout(() => {
          res.sendStatus(204);
        }, 7000);
      });

Then make a test similar to the following:

    // eslint-disable-next-line jest/expect-expect
    it('should timeout if a request takes too long', async () => {
      const timeoutUrl = `${baseUrl}/timeout`;
      const indexPath = `/?inject=${encodeURIComponent(`<img src="${timeoutUrl}">`)}`;

      const complete = await createResourceArchive(page);

      await page.goto(new URL(indexPath, baseUrl).toString());
      const archive = await complete();

      // expectArchiveContains(archive, [indexPath, '/img.png', '/style.css']);
    });

This doesn't achieve what I want, though, because the test itself times out, rather than the setTimeout promise rejecting. Perhaps I'm doing something wrong?

@codykaup or @jmhobbs thoughts on how I might be able to implement a test for timeouts?

[codykaup]

It looks like we have some capture tests for this and we accomplish it by setting the timeout to basically 0 then run the test. Since all the requests take longer than our timeout, we get the error we want.

I'll admit, I'm not up-to-date on this project so maybe this is warranted but it seems like we're doing a lot of heavy lifting with promises while watching these requests. The spirit of our internal navigation watcher is much like Playwright's waitUntil argument. We're essentially keeping track of requests that start/end/fail and reset a timer each time something updates. As soon as don't see any updates for X amount of time or we hit our max timeout, we exit.

[tmeasday]

I think setting the timeout much lower in the test is probably the key unlock here. Otherwise your testing strategy sounds right to me @jwir3.

I'll admit, I'm not up-to-date on this project so maybe this is warranted but it seems like we're doing a lot of heavy lifting with promises while watching these requests. The spirit of our internal navigation watcher is much like Playwright's waitUntil argument. We're essentially keeping track of requests that start/end/fail and reset a timer each time something updates. As soon as don't see any updates for X amount of time or we hit our max timeout, we exit.

I think this a matter of personal preference and so I would definitely leave it up to @jwir3 or y'all.

But I would say the technique of await Promise.all(requestsThatHadStartedWhenIdleWasCalled) isn't going to work because there may be other promises added later. I guess that makes keeping a list of promises less useful, rather than just a list of booleans of "has this request finished?".

To my mind, a conceptually similar approach is a single "summary" promise of "are all requests done?" that we (maybe) resolve when each request is done. So something like:

wait() {
  await Promise.race([ timeoutPromise, this.allRequestsDonePromise ])
}

// In handler
this.client
  .send(method, params)
  .then((value) => {
     // Or simply remove from list
     this.pendingRequests.set(request.url, true);

     if (!Object.values(this.pendingRequests).any(done => !done)) {
       this.this.allRequestsDoneResolver()
     }
  });

[jwir3]

@tmeasday @tevanoff @skitterm

I think this is ready for review now. I'm actually using playwright's built-in functionality to wait for the network to be idle. This is similar to what we do in capture, but, for some reason, we weren't able to use playwright's functionality there. It's possible that the capture use case is so much more complex than this one that this is the reason, but I'm honestly not sure.

Note: The CI tests are still failing, so I'll clean that up today if you want to review the code. Fixed now.

[tmeasday]

Oh neat! Well that's simpler :)

I wonder if we can use that same thing in capture somehow. I guess we'll learn more in this project.

[tevanoff]

Nice, looks good to me!

[thafryer]

🚀 PR was released in v0.0.25 🚀