core WIP: Use Polly to mock HTTP requests in tests

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:

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.
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:

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?
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.

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:

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.
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.

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:

You have to wrap your test in a nockBack call.
You have to name your recording files.
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).

Mar 22 '22 22:03 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.

Mar 22 '22 23:03 mcmire

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.

Mar 29 '22 22:03 Gudahtt

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.

Mar 30 '22 17:03 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:

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.
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.
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.

Apr 04 '22 22:04 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.

Apr 05 '22 16:04 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.

May 12 '23 17:05 mcmire