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

Fix misaligned mock eth query types in controller tests #2037

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 14, 2023

Explanation

Fixes // @ts-expect-error Mock eth query does not fulfill type requirements annotations throughout core repo.

References

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

@MajorLift MajorLift self-assigned this Nov 14, 2023
@MajorLift MajorLift requested a review from a team as a code owner November 14, 2023 17:05
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 440 to 446
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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MajorLift MajorLift Nov 14, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

@mcmire mcmire Nov 14, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could keep this PR open until that's done. FakeProvider seems like a definite improvement over {} as Provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, new PR created here: #2039

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize it would be that simple.😅 Just added a review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've merged #2039 so these changes should now work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed all of the mock eth queries to use FakeProvider: 687d3eb

Copy link

socket-security bot commented Nov 14, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

@MajorLift MajorLift merged commit 5f62ad4 into main Nov 15, 2023
128 checks passed
@MajorLift MajorLift deleted the 231114-fix-mock-eth-query branch November 15, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix misaligned mock eth query types in controller tests
2 participants