Fix misaligned mock eth query types in controller tests#2037
Conversation
mcmire
left a comment
There was a problem hiding this comment.
These tests aren't set up very well :( I'm curious whether jest.mock would be a better approach to mock eth-query rather than making fake versions. That would be a bigger refactor though, so for now I've tried to suggest changes that I hope wouldn't take too long to resolve.
| class MockEthQuery extends EthQuery { | ||
| getBlockByHash(blockId: any, cb: any) { | ||
| cb(null, { id: blockId }); | ||
| } | ||
| } | ||
| // @ts-expect-error Mock eth query does not fulfill type requirements | ||
| const result = await util.query(new EthQuery(), 'getBlockByHash', [ | ||
| '0x1234', | ||
| ]); | ||
| const result = await util.query( | ||
| new MockEthQuery({} as Provider), |
There was a problem hiding this comment.
Hmm. Now that we have a fake provider, I'm wondering if a better way to achieve this without the type assertion would be something like this:
| class MockEthQuery extends EthQuery { | |
| getBlockByHash(blockId: any, cb: any) { | |
| cb(null, { id: blockId }); | |
| } | |
| } | |
| // @ts-expect-error Mock eth query does not fulfill type requirements | |
| const result = await util.query(new EthQuery(), 'getBlockByHash', [ | |
| '0x1234', | |
| ]); | |
| const result = await util.query( | |
| new MockEthQuery({} as Provider), | |
| const ethQuery = new EthQuery(new FakeProvider()); | |
| jest.spyOn(ethQuery, 'getBlockByHash').mockResolvedValue({ id: '0x1234' }); | |
| const result = await util.query( | |
| ethQuery, |
(We'll have to import FakeProvider at the top of course)
There was a problem hiding this comment.
Is this the FakeProvider in network-controller? I'm getting this error:
error TS2345: Argument of type 'FakeProvider' is not assignable to parameter of type 'Provider'.It's also currently not exported from network-controller so ideally we'd need to create a new release to add the export.
There was a problem hiding this comment.
Here's my attempt at applying FakeProvider: 0a03684
Errors: https://github.com/MetaMask/core/actions/runs/6868532345/job/18679337496?pr=2037
There was a problem hiding this comment.
I had thought that FakeProvider was defined at the top level like FakeBlockTracker. We'd have to pull it out to use it like this, so never mind :(
The way you had it is probably fine for now, and we can refactor it later.
There was a problem hiding this comment.
We could keep this PR open until that's done. FakeProvider seems like a definite improvement over {} as Provider.
There was a problem hiding this comment.
Didn't realize it would be that simple.😅 Just added a review.
There was a problem hiding this comment.
I've merged #2039 so these changes should now work.
There was a problem hiding this comment.
Fixed all of the mock eth queries to use FakeProvider: 687d3eb
packages/gas-fee-controller/src/fetchGasEstimatesViaEthFeeHistory.test.ts
Show resolved
Hide resolved
a43b654 to
0a03684
Compare
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
6de0c76 to
699e88b
Compare
699e88b to
687d3eb
Compare
Explanation
Fixes
// @ts-expect-error Mock eth query does not fulfill type requirementsannotations throughout core repo.References
NetworkControllerand its downstream packages #1823Changelog
N/A
Checklist