WIP: Use Polly to mock HTTP requests in tests

[mcmire]

tl;dr: I've introduced Polly as a replacement for Nock as I
believe it will lead to more readable and authentic tests. A few things:

• This is a proof of concept – I've only added it to
  CollectiblesController to start out with.
• All I've done is replace Nock requests with Polly requests. Ideally,
  we wouldn't need to mock anything manually at all and let Polly do its
  thing. Unfortunately, some HTTP requests that we are mocking no longer
  work, so we have to keep mocking for now. That can be refactored in a
  future commit.
• In the future, we should investigate using Ganache instead of hitting
  mainnet and exposing our Infura project ID in tests.

---

Currently there are a couple of problems with our tests as it relates to
network requests:

1. We do not prevent network requests from being made. There are
   some tests which use Nock to mock requests, but not all requests are
   being mocked, and we are unaware which ones are actually going
   through.
2. Nock, although a popular package, is rather cumbersome to use, and
   makes tests difficult to understand and follow.

To address these problems, this commit replaces Nock with Polly. Polly
is a package developed by Netflix that intercepts all HTTP requests and
automatically captures snapshots (recordings) of these requests as they
are being made. These recordings are then used in successive test runs.
You can also manually mock requests if you want more manual control.

Why introduce another package? Why is Nock not sufficient?

There are a few problems with Nock that make it difficult to work with:

1. Mocks take up space in your test, so you will probably find yourself
   extending a collection of mocks you've assembled at the top of the
   file. Once you have enough, who knows which mocks serve which tests?

2. Once you've added a mock to your test, that mock stays forever. If
   the API changes in the future, or stops working altogether, you will
   never know until you remove that mock.

3. If you turn nock.disableNetConnect() on, and you haven't mocked a
   request, it will throw a NetConnectNotAllowedError. This works
   in most cases, but what happens if you have a try/catch block
   around the request you're making and you are swallowing the error?
   Then your test may fail for no apparent reason. And how do you know
   which request you need to mock? Nock has a way to peek behind the
   curtain and log the requests that are being made by running
   DEBUG=nock.* yarn jest ..., but this output is seriously difficult
   to read:

   2022-03-23T03:50:09.033Z nock.scope:api.opensea.io query matching skipped
   2022-03-23T03:50:09.033Z nock.scope:api.opensea.io matching [https://api.opensea.io:443/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193](https://api.opensea.io/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193) to GET
   [https://api.opensea.io:443/api/v1/asset_contract/0x2aEa4Add166EBf38b63d09a75dE1a7b94Aa24163](https://api.opensea.io/api/v1/asset_contract/0x2aEa4Add166EBf38b63d09a75dE1a7b94Aa24163): false
   2022-03-23T03:50:09.033Z nock.scope:api.opensea.io attempting match {"protocol":"https:","slashes":true,"auth":null,"host":"api.opensea.io:443","port":443,"hostname":"api.opensea.io","hash":null,"search":null,"query":null,"pathname":"/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193","path":"/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193","href":"[https://api.opensea.io/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193","method":"GET","headers":{"accept":["*/*"],"user-agent":["node-fetch/1.0](https://api.opensea.io/api/v1/asset/0x495f947276749Ce646f68AC8c248420045cb7b5e/40815311521795738946686668571398122012172359753720345430028676522525371400193%22,%22method%22:%22GET%22,%22headers%22:%7B%22accept%22:[%22*/*%22],%22user-agent%22:[%22node-fetch/1.0) (+[https://github.com/bitinn/node-fetch)"],"accept-encoding":["gzip,deflate"],"connection":["close"]},"proto":"https](https://github.com/bitinn/node-fetch)%22],%22accept-encoding%22:[%22gzip,deflate%22],%22connection%22:[%22close%22]%7D,%22proto%22:%22https)"}, body = ""
   

Polly removes all of these problems:

1. Because Polly records request snapshots, you don't usually have to
   worry about mocking requests manually. These recordings are kept in
   files which are categorized by the tests which made the requests.

2. Because Polly actually hits the API you're mocking, you can work with
   real data. You can instruct Polly to expire these recordings after a
   designated timeframe to guarantee that your code doesn't break if
   the API ceases to work in the way you expect.

3. Polly will also print a message when a request is made that it
   doesn't recognize without needing to run an extra command:

   Errored ➞ POST https://mainnet.infura.io/v3/ad3a368836ff4596becc3be8e2f137ac PollyError: [Polly] [adapter:node-http] Recording for the following request is not found and `recordIfMissing` is `false`.
       {
         "url": "https://mainnet.infura.io/v3/ad3a368836ff4596becc3be8e2f137ac",
         "method": "POST",
         "headers": {
           "content-type": "application/json",
           "connection": "keep-alive",
           "host": "mainnet.infura.io",
           "user-agent": "Mozilla/5.0 (Darwin x64) node.js/12.22.6 v8/7.8.279.23-node.56",
           "content-length": "201"
         },
         "body": "{\"jsonrpc\":\"2.0\",\"id\":30,\"method\":\"eth_call\",\"params\":[{\"to\":\"0x18E8E76aeB9E2d9FA2A2b88DD9CF3C8ED45c3660\",\"data\":\"0x0e89341c0000000000000000000000000000000000000000000000000000000000000024\"},\"latest\"]}",
         "recordingName": "CollectiblesController/updateCollectibleFavoriteStatus/should keep the favorite status as false after updating metadata",
         "id": "53192eab94ddedfb300b625b1cb79843",
   ...
   

What about Nock Back? Doesn't this do the same thing?

It's true that Nock contains a feature to capture requests called Nock
Back. However, this is cumbersome, too:

1. You have to wrap your test in a nockBack call.
2. You have to name your recording files.
3. You have to call nockDone() in your test.

There is a package called jest-nock-back that fixes these issues and
makes using Nock Back very easy. However, it isn't maintained (last
release was 3 years ago) and is likely incompatible with the newest
version of Jest.

What about requests that irreversibly change data?

Some requests make irreversible changes to resources, such as
transferring a token from one account to another.

To handle these types of requests, Polly still lets you manually mock
requests just like Nock. We've configured Polly to not record a request
it doesn't recognize by default, so in this case you'll get a message
when running the test that makes such a request. From there you can make
a decision about how you'd like to mock this request — whether you want
Polly to record the request or whether you'd like to manually mock it.

That said, if the request in question involves a blockchain network,
instead of mocking the request, we might investigate whether it's
possible to use Ganache to perform the irreversible action rather than
actually hitting mainnet or some other network.

What are the tradeoffs for Polly vs. Nock?

• Because Polly automatically creates request mocks and these mocks are
  no longer contained in tests, developers will need to be educated
  about Polly (Polly is not as popular as Nock) and remember that it
  exists whenever updating tests.
• If a recording is no longer needed or needs to be updated, the
  associated file holding the recorded information about the request
  needs to be deleted. Finding this file is not a great experience.
• Nock has virtually zero initialization code. Polly's initialization
  code is surprisingly complicated (although that is largely hidden, so
  no one should have to mess with it).

[mcmire]

I am not 100% sure why these tests changed after I replaced the Nock mocks. I noticed that some requests were being called for real and I attempted to mock those. It's possible I missed something. If this were not a draft PR, I would correct this, but for the time being, I just tried to get the tests passing.

[mcmire]

This is a bit of a hack to set up Polly globally and make it accessible in any test. It is a variation on these instructions: https://netflix.github.io/pollyjs/#/test-frameworks/jest-jasmine

[mcmire]

Polly has an expiresIn setting, which causes the recordings Polly has captured to automatically expire after a set time. Since we set recordIfMissing: false, this will cause tests with expired recordings to fail tests, which may force developers to recapture recordings for tests that they've never touched. For this reason, I've opted to write this script to check for expired recordings instead of using expiresIn. That way we could run this on CI or locally or wherever and it wouldn't interfere with development.

[mcmire]

Usually Polly records requests by default. That is fine if you are adding Polly to a fresh project, but since we're adding Polly to an existing project we need to know whether there are requests being made in tests that we forgot to mock using Nock. This allows us to do this — we will see a warning if Polly sees a request it doesn't recognize.

[Gudahtt]

Hmm. I was expecting there to be some way to throw if a record is missing. Otherwise we might miss these warnings creeping in, and we might not see tests fail either for the same reason you mentioned for disableNetConnect.

[Gudahtt]

Honestly this is the main sticking point for me at the moment. I'd feel better knowing that it was impossible for it to hit the network by default. Is it possible for us to add a catch-all mock that throws an error, something that would act similarly to nock's disableNetConnect?

[mcmire]

I misspoke: it does throw if a recording is missing. This assumes that nothing will catch that error, though. CollectiblesController has a fair amount of try/catchs, so what would have been errors were simply warnings.

[Gudahtt]

Oh... that's great! Well. I am for switching to Polly then. Seems equally effective and at least a bit easier to use than nock.

[mcmire]

I've made a couple of changes to this PR (notably a script to look for outdated recordings/snapshots) which means that we could conceivably use Nock Back instead of Polly. I'll try some more things out tomorrow. Edit: there are still some things I don't like about Nock and I've tried to highlight them more in the description above.

[Gudahtt]

Polly seems like a cool project. I don't have the same issues with nock that you have though.

you will probably find yourself extending a collection of mocks you've assembled at the top of the file

This sounds like an example of sharing mocks between tests, which is generally a bad practice regardless what the mock is. Removing cases like this is on the "improve unit tests" lists. Or if it's being reset in between each test, technically it wouldn't be a shared mock but it would be awful for exactly the reason you listed - which mock is for which test? Total abuse of the test hooks imo. Either way it's awful to deal with for any mock, and it's easily avoided.

For simpler requests it's easy enough to build the nock request inline. For more complex APIs, it can be moved to helper functions rather than shared mocks. Or we could load the mock requests from JSON files, like a manual version of what Polly is doing. Polly seems easier to use for complex APIs though.

Once you've added a mock to your test, that mock stays forever. If the API changes in the future, or stops working altogether, you will never know until you remove that mock.

This... is an interesting point. Generally I think of this sort of thing as "out of scope" for a unit test. I think of unit tests as solely to test our chunk of code, independent of the outside world, ensuring it does what is expected in all cases.

But we could have integration/e2e tests here at some point. Having the ability to verify whether mocks are accurate would be pretty convenient. I expect we could get something like this working with nock, with some difficulty, but this does seem easier to do with Polly.

If you turn nock.disableNetConnect() on, and you haven't mocked a request, it will throw a NetConnectNotAllowedError. This works in most cases, but what happens if you have a try/catch block around the request you're making and you are swallowing the error?

In most cases I'd consider catch-all try/catch blocks an error handling anti-pattern. Or at least it's a red flag, a sign that we might be making a false assumption in our error handling. We should try to list any expected errors when possible so that if something blows up that we don't expect we know about it.

We kinda have to do this for third-party APIs though, like for anything that we could be sending to a custom network. We could look for NetConnectNotAllowedError specifically and always re-throw it in these cases. Doesn't seem ideal to me, but... I don't know. As single wrapper around all network interactions, it might not be so bad. Seems manageable.

---

I do like that Polly makes recording easier though, and that the recording format is a HAR file. HAR files are a widely understood format, and lots of tools support them (e.g. chrome dev tools). So that's nice. Pretty sure the format nock uses is custom for nock.

[mcmire]

Okay, so it sounds like the next step here is to try to make some changes to the structure of these tests. That should mitigate some of the pain points with Nock I mentioned.

I would also like to see if I can modify Nock so that when there is a request made that you haven't mocked yet, it will output information about that request in a similar way that Polly does. That should make working with Nock easier as it will be easy to understand which request was made and how exactly you can mock that request.

[mcmire]

I ran into more issues with Nock today and wanted to write them down. I was trying to write some tests in the extension and was trying to figure out why, despite my attempts to mock requests to Infura, Nock was producing an error indicating that a request was made but it couldn't find a mock matching the request. I eventually realized a few things:

1. Every time you say .get or .post you're adding an interceptor. Once a request is made, if that request matches an interceptor, that interceptor is removed from a list. That means that if two requests are made with identical request bodies, then you need to mock the request twice. You can get around this by calling .persist(), but then you have to call nock.cleanAll() in an afterEach or something to clear the "persisted" nocks.
2. The error message that Nock produces is really confusing. It'll say "Nock: No match for request Got instead ". Which is the expected and which is the actual? Eventually, after staring at it for a bit, I realized the first is the actual and the second is the expected, but even so, the second request didn't contain a request body, even though I'd specified an expected request body in my test.
3. In cases where you have a try/catch around a request and you don't throw the error appropriately, you won't get a proper error message when running the test. You can get around this by assigning the Nock mock to scope and then calling scope.done() at the end of the test, except that what happens if the test throws an error before scope.done()? So you have to end up commenting out some things in your test that would cause the error so that scope.done() is run appropriately. Really awkward.

I haven't tested whether these things are better in Polly yet but for now I just wanted to log this.

[mcmire]

More Nock fun today. It appears that if you call nock.cleanAll() in an afterEach (which is necessary if you're using the .persist() option for a nock) and you have tests which add nocks, and your implementation makes multiple requests, then only the first test that has nocks will work; the rest won't. For instance:

describe('Test case', () => {
  afterEach(() => {
    nock.cleanAll();
  });

  it('does something', () => {
    // this will work
    nock('http://google.com')
      .post('/foo')
      .reply(200);
    
    callFunctionUnderTest();
  });

  it('does something else', () => {
    // this won't
    nock('http://google.com')
      .post('/foo')
      .reply(200);
    
    callFunctionUnderTest();
  });
});

Maybe the solution here is not to use .persist(), ever. That's fine, but people now need to learn this, and that nock.cleanAll() is a footgun.

[mcmire]

I'm going to close this PR. I'm no longer fully convinced that using Polly would be the right move. While Polly is a more mature project than Nock, Nock is vastly more popular than Polly — within our organization and within the greater JavaScript community — and is maintained way more often (Polly has not been updated for over a year and its status is unknown, while Nock continues to be updated). Given this, I think it would be more economical for us to improve our usage of Nock rather than switch to a different tool entirely. If we have gripes with Nock, we should be good Samaritans and try to fix them so that other users of Nock can benefit.