-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
testfix(transaction-controller): Refactor provider setup to provide correct providers and tracker #4391
testfix(transaction-controller): Refactor provider setup to provide correct providers and tracker #4391
Conversation
97da9a5
to
a0e3f37
Compare
a0e3f37
to
5f25d4e
Compare
provider: MAINNET_PROVIDER, | ||
blockTracker: buildMockBlockTracker('0x102833C', MAINNET_PROVIDER), | ||
provider: HTTP_PROVIDERS.goerli, | ||
blockTracker: buildMockBlockTracker('0x102833C', HTTP_PROVIDERS.goerli), |
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.
Note: Mismatching provider here (e.g. supplying HTTPS_PROVIDERS.mainnet
) did not cause tests to fail. This suggests that some further coverage could be desired, to catch situations were blockTracker and provider inadvertently are on different chains/networks?
b525411
to
ac84c1c
Compare
previously a single provider referencing "palm mainnet" stood in for all except ethereum-mainnet
…ck rpc providers" This reverts commit 7521978.
…ttp providers instead of `palm-mainnet` for all networks
…entical across provider urls
…entical across provider urls
…being identical across provider urls" This reverts commit c7a2791.
…entical across provider urls
ac84c1c
to
a8b210c
Compare
(lint fix due to rebase on top of #4382) |
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.
Ideally we shouldn't need to create specific providers for specific use cases. We shouldn't be hitting the network at all, so if we do need a provider we should be able to make a fake version using FakeProvider. But, as a fix this PR makes sense.
'sepolia', | ||
'customNetworkClientId-1', | ||
] as const) { | ||
MOCK_BLOCK_TRACKERS[network] = buildMockBlockTracker(network); |
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.
The problem this solves is a good example of why sharing variables among tests by initializing them outside of all tests can lead to misleading and difficult to debug behavior (since a variable may have state that's changed in one test and then bleeds over into another test). This fix makes sense, but if we really need specific block trackers and providers it would be better to create them on the fly in tests, using a helper function to assist. See here for more: https://github.com/MetaMask/contributor-docs/blob/main/docs/unit-testing.md#reset-shared-variables (expand the triangle for an example)
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.
Fully agreed! The end state here is still not as I'd like it either but had to draw the line somewhere ;^^
…orrect providers and tracker (#4391)
…orrect providers and tracker (#4391)
…orrect providers and tracker (#4391)
…orrect providers and tracker (#4391)
…orrect providers and tracker (#4391)
…orrect providers and tracker (MetaMask#4391)
…orrect providers and tracker (#4391)
Explanation
Refactors tests so that providers are injected appropriately in the
MockMultichainTransaction
.The error is interacting with a separate bug in
@metamask/eth-block-tracker
<v9.0.3
(@metamask/nonce-tracker
< 6.0.0), which is why we don't see them failing until #4329 / #4327References
!isMultichainEnabled
#4390Changelog
Checklist