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

chore(test): Refactor FakeBlockTracker provider injection #4345

Merged

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented May 30, 2024

Explanation

transaction-controller constructor takes a provider and a blockTracker. In tests, there is mocking of these. A BlockTracker has its own provider and while in reality the blockTracker.provider == provider, it appears inconsistent in tests where a test may use both a mocked and a "real" provider instance at the same time.

This aims at making this more transparent by making the provider parameter of FakeBlockTracker mandatory.

References

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

@legobeat legobeat changed the title chore: Refactor transaction-controller test chore(test): Refactor provider injection in tests May 30, 2024
super({
provider: new SafeEventEmitterProvider({ engine: new JsonRpcEngine() }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the SafeEventEmitterProvider gets replaced and tests making use of the FakeBlockTracker now have to supply a provider explicitly.

@legobeat legobeat marked this pull request as ready for review May 30, 2024 23:31
@legobeat legobeat requested a review from a team as a code owner May 30, 2024 23:31
@legobeat legobeat changed the title chore(test): Refactor provider injection in tests chore(test): Refactor FakeBlockTracker provider injection May 30, 2024
@legobeat legobeat force-pushed the transaction-controller-test-refactor branch 2 times, most recently from 41718b0 to eeb3f4e Compare May 31, 2024 06:55
dbrans
dbrans previously approved these changes Jun 4, 2024
Copy link
Contributor

@dbrans dbrans left a comment

Choose a reason for hiding this comment

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

@legobeat Thank you, this is fantastic work.

@legobeat
Copy link
Contributor Author

legobeat commented Jun 4, 2024

@legobeat Thank you, this is fantastic work.

Thanks!

Just had to resolve a conflict in yarn.lock when syncing with main which invalidated your approve, no further changes.

@legobeat legobeat requested review from dbrans and a team June 4, 2024 20:23
@legobeat legobeat force-pushed the transaction-controller-test-refactor branch from 360299a to 0a8fbbf Compare June 5, 2024 01:57
@legobeat legobeat merged commit ba07fd8 into MetaMask:main Jun 5, 2024
113 checks passed
@legobeat legobeat deleted the transaction-controller-test-refactor branch June 5, 2024 16:08
legobeat added a commit to legobeat/metamask-controllers that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants