-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase Jest unit test coverage for the Swaps feature to ~43% #10934
Conversation
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. |
it('renders the component with initial props', async () => { | ||
const store = configureMockStore(middleware)(createSwapsMockStore()); | ||
const { container, getByText } = renderWithProvider(<Swap />, store); | ||
await waitFor(() => expect(tokensNock.isDone()).toBe(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great way to wait until a mocked API call is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nifty!
@@ -3,4 +3,9 @@ const originalModule = jest.requireActual('react-router-dom'); | |||
module.exports = { | |||
...originalModule, | |||
useHistory: jest.fn(), | |||
useLocation: jest.fn(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presumably when we expand test coverage in our other areas of the repo we'll need to mock this out in those files to override the pathname?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. My assumption is that most of the tests will be fine with this path, but if they need to use a different one, they can just do this:
jest.mock('react-router-dom', () => {
const originalModule = jest.requireActual('react-router-dom');
return {
...originalModule,
useHistory: jest.fn(),
useLocation: jest.fn(() => {
return {
pathname: '/feature-z/page-1',
};
}),
}
});
})} | ||
/>, | ||
); | ||
expect(getByText('View at custom-blockchain.explorer')).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for us to test what the URL will go to here instead of just checking the link text? It'd help to ensure that changes made to the URL but not the text don't pass through our test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, will add an assertion for a URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the code and we don't render a link, but open a new tab via an onClick
event on a div
element:
onClick={() => global.platform.openTab({ url: blockExplorerUrl })}
I will be testing events in following PRs, so I would prefer to do it there.
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`storiesMetadata matches expected values for storiesMetadata 1`] = ` | ||
Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming this snapshot exists just to confirm that the loading-swaps-quotes-stories-metadata file isn't touched, or if it is that it is intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. If there would be any change in loading-swaps-quotes-stories-metadata.js
, a test would fail with an outdated snapshot. It's just a double-check that someone didn't accidentally modify it + it was showing 0% test coverage, which was lowering our overall coverage.
@@ -130,6 +130,7 @@ module.exports = { | |||
rules: { | |||
'jest/no-restricted-matchers': 'off', | |||
'import/unambiguous': 'off', | |||
'import/named': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this get turned off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! That's because I used module.exports
in the link below: https://github.com/MetaMask/metamask-extension/pull/10934/files#diff-8fd96322fc664541ebc0326e3c5963e55db825809abb977c6ffad8be7555cbd1R7
After which I started getting these errors:
I will give it little more time to use export
instead of module.exports
when re-exporting imported things. Then we hopefully wouldn't need to turn import/named
off for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I've changed it to export
only:
export { createSwapsMockStore } from './mock-store';
export { renderWithProvider } from './rendering';
export { setBackgroundConnection } from './background';
export * as MOCKS from './mocks';
export * as CONSTANTS from './constants';
But even while tests are green, I'm getting the same ESLint issue when re-exporting:
That's why I would prefer to keep 'import/named': 'off',
, just for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks legit. import-js/eslint-plugin-import#1998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
@@ -33,7 +33,7 @@ | |||
"test:e2e:firefox": "SELENIUM_BROWSER=firefox test/e2e/run-all.sh", | |||
"test:e2e:firefox:metrics": "SELENIUM_BROWSER=firefox mocha test/e2e/metrics.spec.js", | |||
"test:coverage": "nyc --silent --check-coverage yarn test:unit:strict && nyc --silent --no-clean yarn test:unit:lax && nyc report --reporter=text --reporter=html", | |||
"test:coverage:jest": "jest --coverage --maxWorkers=2 --collectCoverageFrom=**/ui/app/**/swaps/**", | |||
"test:coverage:jest": "jest --coverage --maxWorkers=2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the removal of the collectCoverageFrom flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e3c3c26
to
9aa65f8
Compare
Explanation
This PR increases Swaps coverage from 25% to about 43%, which is getting us one step closer to 90% coverage.
Manual testing steps
npm run test:coverage:jest
Screenshots
Swaps Jest unit test coverage is getting greener (43.12% for Lines coverage):
![image](https://user-images.githubusercontent.com/80175477/116156340-3212c080-a6a0-11eb-8f5c-78a65f15150e.png)