Skip to content

Commit

Permalink
WIP: Use Polly to mock HTTP requests in tests
Browse files Browse the repository at this point in the history
**tl;dr:** I've introduced [Polly][1] 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][2]. 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 to GET
   https://api.opensea.io:443/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://github.com/bitinn/node-fetch)"],"accept-encoding":["gzip,deflate"],"connection":["close"]},"proto":"https"}, 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][3]. 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).

[1]: https://netflix.github.io/pollyjs/#/README
[2]: https://github.com/nock/nock#enabledisable-real-http-requests
[3]: https://github.com/nock/nock#nock-back
  • Loading branch information
mcmire committed Mar 23, 2022
1 parent e3a972e commit e5f4b60
Show file tree
Hide file tree
Showing 24 changed files with 5,762 additions and 766 deletions.
7 changes: 6 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,16 @@ module.exports = {
extends: ['@metamask/eslint-config-jest'],
},
{
files: ['*.js'],
files: ['scripts/*.js'],
parserOptions: {
sourceType: 'script',
ecmaVersion: '2018',
},
rules: {
'node/no-process-exit': 'off',
'node/no-sync': 'off',
'node/shebang': 'off',
},
},
{
files: ['*.ts'],
Expand Down
2 changes: 2 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Collapse changes to Polly recordings in PRs.
tests/__recordings__/** linguist-generated
4 changes: 2 additions & 2 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ module.exports = {
// modules.
// restoreMocks: true,
setupFiles: ['./tests/setupTests.ts'],
testEnvironment: 'jsdom',
testRegex: ['\\.test\\.(ts|js)$'],
setupFilesAfterEnv: ['./tests/setupTestsAfterEnv.ts'],
testEnvironment: 'setup-polly-jest/jest-environment-node',
testTimeout: 5000,
transform: {
'^.+\\.tsx?$': 'ts-jest',
Expand Down
7 changes: 7 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@metamask/contract-metadata": "^1.31.0",
"@metamask/metamask-eth-abis": "3.0.0",
"@metamask/types": "^1.1.0",
"@types/setup-polly-jest": "^0.5.1",
"@types/uuid": "^8.3.0",
"abort-controller": "^3.0.0",
"async-mutex": "^0.2.6",
Expand Down Expand Up @@ -77,6 +78,10 @@
"@metamask/eslint-config-jest": "^9.0.0",
"@metamask/eslint-config-nodejs": "^9.0.0",
"@metamask/eslint-config-typescript": "^9.0.1",
"@pollyjs/adapter-fetch": "^6.0.4",
"@pollyjs/adapter-node-http": "^6.0.4",
"@pollyjs/core": "^6.0.4",
"@pollyjs/persister-fs": "^6.0.4",
"@types/deep-freeze-strict": "^1.1.0",
"@types/jest": "^26.0.22",
"@types/jest-when": "^2.7.3",
Expand All @@ -95,13 +100,15 @@
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-prettier": "^3.3.1",
"ethjs-provider-http": "^0.1.6",
"glob": "^7.2.0",
"jest": "^26.4.2",
"jest-environment-jsdom": "^25.0.0",
"jest-when": "^3.4.2",
"nock": "^13.0.7",
"prettier": "^2.2.1",
"prettier-plugin-packagejson": "^2.2.11",
"rimraf": "^3.0.2",
"setup-polly-jest": "^0.10.0",
"sinon": "^9.2.4",
"ts-jest": "^26.5.2",
"typedoc": "^0.20.32",
Expand Down
50 changes: 50 additions & 0 deletions scripts/check-for-expired-polly-recordings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#!/usr/bin/env node

const path = require('path');
const fs = require('fs');
const glob = require('glob');

const getThirtyDaysAgo = () => {
const now = new Date();
now.setMonth(now.getMonth() - 1);
return now;
};

/**
* This script scans tests/__recordings__, which is where Polly captures
* recordings of requests, and looks for files that have captured requests that
* are older than 30 days. If any are found, the script will report the names of
* the tests that have these recordings and exit with 1. Otherwise, it will
* exit with 0.
*/
function main() {
const thirtyDaysAgoTimestamp = getThirtyDaysAgo().getTime();
const outdatedTests = [];
const filePaths = glob.sync(
path.resolve(__dirname, '../tests/__recordings__/**/*.har'),
{ realpath: true },
);
filePaths.forEach((filePath) => {
const encodedJson = fs.readFileSync(filePath, 'utf8');
const decodedJson = JSON.parse(encodedJson);
const isOutdated = decodedJson.log.entries.some((entry) => {
const timestamp = Date.parse(entry.startedDateTime);
return timestamp < thirtyDaysAgoTimestamp;
});
if (isOutdated) {
outdatedTests.push(decodedJson.log._recordingName);
}
});

if (outdatedTests.length > 0) {
console.log('Some tests have outdated Polly recordings!');
outdatedTests.forEach((test) => {
console.log(`- ${test.replace(/\//gu, ' -> ')}`);
});
process.exit(1);
} else {
console.log('No tests have outdated Polly recordings, all good!');
}
}

main();
Loading

0 comments on commit e5f4b60

Please sign in to comment.