Increase Jest unit test coverage for the Swaps feature to ~25%

[dan437]

Explanation

I've added the most popular way to test React components - snapshot testing, starting with happy paths. I've combined it with targeted assertions for specific things as well.

The goal is to increase coverage at least to 90%, so next PRs will cover:

• Testing with different sets of props
• Covering more files
• Testing events (click, typing, submit, client-side validations, etc.)

I've created a new folder test/jest, where we have the setup.js file for setting up React Testing Library with Jest assertions and then the test/jest folder contains files with functions that are reusable across Jest tests (rendering, returning a mock Redux store). I believe that all things for Jest except for the tests themselves should be in the test/jest folder, so it's all nicely organized in one place.

Manual testing steps

• To run Jest tests in the silent watch mode you can run this: yarn test:unit:jest:watch:silent (it will not show a plenty of React warnings, since it's the silent mode). You can also run yarn test:unit:jest:watch
• For Jest coverage that is only set up for the Swaps feature now you can run: npm run test:coverage:jest
• If you only want to run tests for the Swaps feature, you can open jest.config.js and use this line: testMatch: ['<rootDir>/ui/app/**/swaps/**/*.test.js'],

Screenshots

Increased test coverage for the Swaps feature:

All Swaps tests are green:

All Jest tests are green with these changes:

CircleCI is red if snapshots are longer than 50 lines:

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[brad-decker]

Took a first pass at this to give some early feedback and ask a few questions

[brad-decker]

We have a similar helper in test/lib/render-helpers which we should probably use instead of adding a new file?

[dan437]

Hi, I actually deliberately took it from there, because I believe it's better to have all things for Jest in the test/jest folder, except the Jest tests themselves. I don't think we should mix shared functions in the same files for Mocha and Jest tests.

Also, our test folder looks like this:

There are folders like data, helpers, lib, mocks, stub and it's not really immediately clear if they have code for Mocha unit tests, Jest unit tests, e2e tests or upcoming Playwright e2e tests or some combination of these. With the test/jest folder we can have all shared functions for Jest nicely organized in one place, so it will be easier to do Jest testing.

I will happily search + replace all paths with test/lib/render-helpers to test/jest for the renderWithProvider function and move all Jest-specific shared functions to test/jest.

[brad-decker]

How does this differ from setupFiles? Can we not set up jest-dom in one of our existing files?

[dan437]

I've tried to load the file in setupFiles, but I'm getting this error for many tests:

With setupFilesAfterEnv it works correctly. setupFilesAfterEnv is also mentioned on the official react testing library site: /~https://github.com/testing-library/jest-dom#usage

One more reason - as I mentioned in a previous comment, I would like to avoid mixing Mocha and Jest code in the same files.

[brad-decker]

I like having a mock store for tests! Do we think there is value in creating specific store mocks (swaps, vs general state?) I think instead of separating by feature it might make sense to separate by a scenario where it makes sense. Like "mainnetAcctWithMultipleTokens" or "customNetworkPowerUser" -- this is just brainstorming atm!

also related but unrelated: #10895 introduces a localization "mock" to the provider

[dan437]

Nice, will check the localization "mock", so we can get translations into our unit tests instead of just keys!

About creating a mock store for a feature vs one general state.. I think a general state could be pretty big if it would contain data for all features. I would do it per feature and if there is some shared state across features, it can be abstracted in the mock-store.js file into its own function and used in feature specific functions like createSwapsMockStore. For example:

export const createSwapsMockStore = () => {
  ...createGeneralMockStore(),
  swapsKey1: 'swapsValue1',
  swapsKey2: 'swapsValue2',
}

For specific scenarios within a feature there is always an option to call e.g. createSwapsMockStore to get default feature state and then modify that state in the code. For example:

const swapsMockStore = createSwapsMockStore();
swapsMockStore.metamask.provider.chainId = BSC_CHAIN_ID;
swapsMockStore.metamask.selectedAddress = 'differentAddress0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc'

Another way is that we could pass a customState object into the createSwapsMockStore function to override default state, but that's little tricky to do (especially with nested objects) and I found that overriding things like this is more straightforward:

const swapsMockStore = createSwapsMockStore();
swapsMockStore.metamask.provider.chainId = BSC_CHAIN_ID;

[brad-decker]

is this an automock? Does jest look for these in every sub-level folder? I thought it only looked for node_modules mocks at the app root.

[dan437]

This behaves like if you would have "automock": true in jest config, except it only mocks one function of this node module and only if that module is imported somewhere in the ui/app folder. More about mocking node modules in Jest here: https://jestjs.io/docs/manual-mocks#mocking-node-modules

Without this we would need to do jest.mock('react-router-dom') in many React files. With this we just do it once here and then this partially mocked version will be used every time we import that module in our React files:
import { useHistory } from 'react-router-dom';

[brad-decker]

cool, i didn't realize it would mock node_modules via relative path like that. neat.

[brad-decker]

Is there any reason that this method is created inside the describe block?

[dan437]

No reason, I will move it outside.

[dan437]

Done in all tests.

[brad-decker]

this is one where we should break apart this snapshot, hopefully in such a way that we don't end up covering the SVG / animation with a snapshot. Polygons. Polygons everywhere.

[dan437]

Haha yes! This is a great example when taking a snapshot of the whole component is not easily readable.

[dan437]

Done.

[brad-decker]

This [swapProcessing] string should be the actual text value we'd expect to see, imo. The change mentioned in the localization mock will make that a possibility.

[dan437]

Absolutely agree. I have this on my todo list to make sure we use real translations instead of keys in our unit tests. I will try to do that as part of this PR.

[dan437]

Done.

[dan437]

FYI, the reason for this file is to export all shared functions from test/jest and then when we need to import some of them in our tests, we only need to import this file instead of multiple files.

[tmashuang]

The fuseSearchKeys prop is require by ListItemSearch,

metamask-extension/ui/app/pages/swaps/searchable-item-list/list-item-search/list-item-search.component.js

Line 83 in 1e44c34

fuseSearchKeys: PropTypes.arrayOf(PropTypes.object).isRequired,

metamask-extension/ui/app/pages/swaps/searchable-item-list/searchable-item-list.js

Lines 65 to 70 in 76a2a9b

	fuseSearchKeys: PropTypes.arrayOf(
	PropTypes.shape({
	name: PropTypes.string,
	weight: PropTypes.number,
	}),
	),

An example is

  "fuseSearchKeys": [
    {
      "name": "name",
      "weight": 0.499
    },
    {
      "name": "symbol",
      "weight": 0.499
    },
    {
      "name": "address",
      "weight": 0.002
    }
  ],

[dan437]

Thanks! I didn't see .isRequired for that specific prop inside searchable-item-list.js, but will add the array you mentioned into default props. With future PRs I will be adding testing of different sets of props, not just happy paths.

[tmashuang]

The primaryFee prop is an object.

metamask-extension/ui/app/pages/swaps/fee-card/fee-card.js

Lines 174 to 177 in 16f533e

	primaryFee: PropTypes.shape({
	fee: PropTypes.string.isRequired,
	maxFee: PropTypes.string.isRequired,
	}).isRequired,

[tmashuang]

An example could be

  "primaryFee": {
    "fee": "0.0441 ETH",
    "maxFee": "0.04851 ETH"
  },

[dan437]

Nice catch! I see it's there from a previous PR, will update it.

[tmashuang]

The secondaryFee prop is an object.

metamask-extension/ui/app/pages/swaps/fee-card/fee-card.js

Lines 178 to 181 in 16f533e

	secondaryFee: PropTypes.shape({
	fee: PropTypes.string.isRequired,
	maxFee: PropTypes.string.isRequired,
	}),

[tmashuang]

An example could be

  "secondaryFee": {
    "fee": "$101.98",
    "maxFee": "$112.17"
  },

[tmashuang]

The only other react warnings after the aforementioned ones are

Warning: Each child in a list should have a unique "key" prop.

Check the render method of `FeeCard`. See https://fb.me/react-warning-keys for more information.
    in Fragment
    in FeeCard
    in LegacyI18nProvider (created by WithoutStore)
    in I18nProvider (created by WithoutStore)
    in Router (created by MemoryRouter)
    in MemoryRouter (created by WithoutStore)
    in WithoutStore (created by Wrapper)
    in Wrapper

and

Warning: Failed prop type: ActionableMessage: prop type `withRightButton` is invalid; it must be a function, usually from the `prop-types` package, but received `undefined`.
      in ActionableMessage
      in ViewQuotePriceDifference
      in Context.Provider

Which are issues with the implementation of those components, not necessarily the tests, and can be viewed as an extraneous fix from the scope of this PR.

[dan437]

Agree, I can solve these pre-existing React warnings as part of this PR.

[brad-decker]

One more thing I'm concerned about with the introduction of a new render helper -- we now have two with differing implementation. I'd like to solve this here by either 1) removing the new one, or 2) moving the newly added code from the old one to the new one in jest/ folder and upgrading all usages to use that one.

Thoughts?

Also fine with doing #2 in an immediate follow up

[dan437]

Can I create a user story to migrate all shared Jest functions into test/jest folder and update all references to it in our Jest tests?

[brad-decker]

LGTM!