-
-
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
Changes from all commits
dd83411
55194c8
6a9f424
0b3bcaf
204f54a
0800917
6357b5b
53a3f7d
3bfdfb2
99aaf73
26fce73
ed8abd7
e7fe855
a8b210c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -222,6 +222,16 @@ function newMultichainTrackingHelper( | |
describe('MultichainTrackingHelper', () => { | ||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
|
||
for (const network of [ | ||
'mainnet', | ||
'goerli', | ||
'sepolia', | ||
'customNetworkClientId-1', | ||
] as const) { | ||
MOCK_BLOCK_TRACKERS[network] = buildMockBlockTracker(network); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 ;^^ |
||
MOCK_PROVIDERS[network] = buildMockProvider(network); | ||
} | ||
}); | ||
|
||
describe('onNetworkStateChange', () => { | ||
|
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?