Add destroy method to block tracker classes
#106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing tests have a few problems: * They use a test library we no longer use. * They're written in a style which exercises functionality across the board, which makes it difficult to tell if an individual method is being properly tested or not. * Private methods are being stubbed to test functionality. * Ganache is being used to simulate mining. * Test coverage is lacking in general. To address this, this commit rewrites the test suite using Jest and backfills a bunch of tests to ensure that they cover every part of both PollingBlockTracker and SubscribeBlockTracker. To accomplish this, and to write tests for both classes in a consistent way, a few minor alterations needed to be made. * Ganache has been removed as a dev dependency and the provider's `sendAsync` method is stubbed to simulate data that the network produces. * Block tracker classes now emit a few extra events: `_started`, `_ended`, and `_waitingForNextIteration` (PollingBlockTracker only). As the prefix indicates, these are not intended to be consumed publicly, but merely aid testing. * PollingBlockTracker now exports `PollingBlockTrackerOptions` (which no longer needs to be wrapped in `Partial` to use). * PollingBlockTracker now takes a `blockResetDuration` option which can be allowed to customize how long it waits after stopping before the current block number, which is usually cached, is cleared. * SubscribeBlockTracker now exports `SubscribeBlockTrackerOptions` (which no longer needs to be wrapped in `Partial` to use) * The type of `removeAllListeners` now matches the EventEmitter interface (that is, its argument is optional). * `_start` and `_stop` are now always async methods (previously this was inconsistent between PollingBlockTracker and SubscribeBlockTracker). All of the existing tests continued to pass with these changes.
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
This reverts commit 2f0b9b2.
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 12, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][2] which has yet to be merged. [1]: MetaMask/core#753 [2]: MetaMask/eth-block-tracker#106
6 tasks
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 12, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 12, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 15, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 15, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 15, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 15, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 15, 2022
We are working on migrating the extension to a unified network controller, but before we do so we want to extract some of the existing pieces, specifically `createInfuraClient` and `createJsonRpcClient`, which provide the majority of the behavior exhibited within the provider API that the existing NetworkController exposes. This necessitates that we understand and test that behavior as a whole. With that in mind, this commit starts with the Infura-specific network client and adds some initial functional tests for `createInfuraClient`, specifically covering three pieces of middleware provided by `eth-json-rpc-middleware`: `createNetworkAndChainIdMiddleware`, `createBlockCacheMiddleware`, and `createBlockRefMiddleware`. These tests exercise logic that originate from multiple different places and combine in sometimes surprising ways, and as a result, understanding the nature of the tests can be tricky. I've tried to explain the logic (both of the implementation and the tests) via comments. Additionally, debugging why a certain test is failing is not the most fun thing in the world, so to aid with this, I've added some logging to the underlying packages used when a request passes through the middleware stack. Because some middleware change the request being made, or make new requests altogether, this greatly helps to peel back the curtain, as failures from Nock do not supply much meaningful information on their own. This logging is disabled by default, but can be activated by setting `DEBUG_PROVIDER_TESTS=1` alongside the `jest` command. Also note that we are using a custom version of `eth-block-tracker` which provides a `destroy` method, which we use in tests to properly ensure that the block tracker is stopped before moving on to the next step. This change comes from [this PR][1] which has yet to be merged. [1]: MetaMask/eth-block-tracker#106
Gudahtt
reviewed
Aug 15, 2022
| @@ -45,6 +45,12 @@ export abstract class BaseBlockTracker extends SafeEventEmitter { | |||
| this._setupInternalEvents(); | |||
| } | |||
|
|
|||
| async destroy() { | |||
| await this._maybeEnd(); | |||
| this._cancelBlockResetTimeout(); | |||
There was a problem hiding this comment.
Should this come before this._maybeEnd()? _maybeEnd() might take some time to resolve for the SubscribeBlockTracker, because of the eth_unsubscribe call.
There was a problem hiding this comment.
Yeah, good call. Fixed in 7d956e6.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PollingBlockTracker and SubscribeBlockTracker contain asynchronous code
that is run somewhat out-of-band. Because the NetworkController uses a
block tracker under the hood, we need to ensure that the block tracker
and its asynchronous code is properly stopped after each test that we
write, otherwise we may get warnings from Jest that promises are still
outstanding before the test run ends. Adding a
destroymethod makesthat possible.
Fixes #102.