Skip to content
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

Mock all network requests #1499

Merged
merged 10 commits into from
Jul 20, 2023
Merged

Mock all network requests #1499

merged 10 commits into from
Jul 20, 2023

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Jul 13, 2023

Explanation

By default HTTP requests are allowed to be made in tests. Because
requests are not guaranteed to succeed, this can cause intermittently
failing CI builds, forcing engineers to wait until PRs are ready to
merge.

This commit addresses this problem by enabling Nock globally and then
modifying each test that hits the network to create proper mocks. This
was somewhat difficult to do with the assets controller tests as they
involve interactions with smart contracts on Mainnet.

Also, Nock was upgraded to 13.3.x, as without this change,
nock.recorder, used for debugging, was not working correctly.

References

Fixes #1
Fixes #770.
Fixes #769
Fixes #771
Fixes #773

Changelog

(N/A)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mcmire mcmire force-pushed the mock-all-network-requests branch 2 times, most recently from 3a49dab to fba30a2 Compare July 13, 2023 23:21
Mocking JSON-RPC requests in tests is somewhat inconvenient due to
needing to spell out the full path for an Infura endpoint and specify
full JSON-RPC request and response objects, but to make this easier,
we've written a small wrapper around Nock. Currently we use this in one
test within the `network-controller` package, but we plan to use this in
a different package, so this commit extracts the class responsible for
doing this. This also makes a small improvement to the API and adds a
function that makes the class even easier to use.
@mcmire mcmire mentioned this pull request Jul 14, 2023
3 tasks
By default HTTP requests are allowed to be made in tests. Because
requests are not guaranteed to succeed, this can cause intermittently
failing CI builds, forcing engineers to wait until PRs are ready to
merge.

This commit addresses this problem by enabling Nock globally and then
modifying each test that hits the network to create proper mocks. This
was somewhat difficult to do with the assets controller tests as they
involve interactions with smart contracts on Mainnet.

Also, Nock was upgraded to 13.3.x, as without this change,
`nock.recorder`, used for debugging, was not working correctly.
@mcmire mcmire force-pushed the mock-all-network-requests branch from fba30a2 to b14bfda Compare July 18, 2023 22:37
@mcmire mcmire changed the base branch from main to extract-mock-network July 18, 2023 22:37
@mcmire mcmire changed the title WIP - Mock all network requests Mock all network requests Jul 18, 2023
@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
nock 13.3.1 network, filesystem, environment +12 322 kB nockbot

@socket-security
Copy link

socket-security bot commented Jul 18, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: propagate@2.0.1, json-stringify-safe@5.0.1, nock@13.3.1

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@mcmire mcmire marked this pull request as ready for review July 19, 2023 17:22
@mcmire mcmire requested a review from a team as a code owner July 19, 2023 17:22
@mcmire mcmire marked this pull request as draft July 19, 2023 17:22
Base automatically changed from extract-mock-network to main July 19, 2023 17:29
@mcmire mcmire marked this pull request as ready for review July 19, 2023 17:47
@legobeat
Copy link
Contributor

@SocketSecurity ignore propagate@2.0.1
@SocketSecurity ignore json-stringify-safe@5.0.1
@SocketSecurity ignore nock@13.3.1

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvement! Looks great.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mcmire mcmire merged commit c47a6be into main Jul 20, 2023
91 checks passed
@mcmire mcmire deleted the mock-all-network-requests branch July 20, 2023 16:33
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
By default HTTP requests are allowed to be made in tests. Because
requests are not guaranteed to succeed, this can cause intermittently
failing CI builds, forcing engineers to wait until PRs are ready to
merge.

This commit addresses this problem by enabling Nock globally and then
modifying each test that hits the network to create proper mocks. This
was somewhat difficult to do with the assets controller tests as they
involve interactions with smart contracts on Mainnet.

Also, Nock was upgraded to 13.3.x, as without this change,
`nock.recorder`, used for debugging, was not working correctly.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
By default HTTP requests are allowed to be made in tests. Because
requests are not guaranteed to succeed, this can cause intermittently
failing CI builds, forcing engineers to wait until PRs are ready to
merge.

This commit addresses this problem by enabling Nock globally and then
modifying each test that hits the network to create proper mocks. This
was somewhat difficult to do with the assets controller tests as they
involve interactions with smart contracts on Mainnet.

Also, Nock was upgraded to 13.3.x, as without this change,
`nock.recorder`, used for debugging, was not working correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants