Rewrite tests using Jest

[mcmire]

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.

---

Fixes #101.

[Gudahtt]

Ganache is being used to simulate mining.

This hardly seems like a problem. Ganache is an excellent mock network, far better than anything we would be able to construct with ad-hoc mocks.

Edit: Well, I guess since this module only interacts with the network via eth_blockNumber, a simple mock does make sense for polling block tracker. And maybe for the subscription as well, I'm less familiar with that. Most of our tests that use ganache do things with contracts that would not be reasonable to mock a the network layer.

[mcmire]

Tests are failing on this due to coverage not being met. I'm still working on this and should have this ready to review a bit later today.

[mcmire]

@Gudahtt I'm happy with this, what do you think?

[mcmire]

@Gudahtt What do you think — ready to go?

[mcmire]

Ready for review again.

[macky22am]

Why am I receiving this?

On Fri, Apr 29, 2022 at 5:41 PM Mark Stacey @.***> wrote:

@.**** commented on this pull request.

In tests/recordCallsToSetTimeout.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r862163807 :

• • setTimeout should be called within a test, however, you may specify that
• • here, and each time setTimeout is called, its callback will be called
• • immediately, up to this many times (default: 0).
• • @param options.interceptCallback - A function that can be used to replace a
• • callback that is passed to setTimeout, allowing you to call it yourself
• • (perhaps in a try/catch block, or something else).
• • @returns An object that can be used to interact with calls to setTimeout.
• */ +export default function recordCallsToSetTimeout({
• numAutomaticCalls = 0,
• interceptCallback = (callback) => callback, +}: {
• numAutomaticCalls?: number;
• interceptCallback?: InterceptingCallback; +} = {}): SetTimeoutRecorder {
• const originalSetTimeout = global.setTimeout;

Nit: Any reason this is passed in, rather than being used directly by the SetTimeoutRecorder constructor?

In tests/recordCallsToSetTimeout.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r862164147 :

• callback: SetTimeoutCallback;
• duration: number;
• timeout: NodeJS.Timeout; +}

• +type InterceptingCallback = (

• callback: SetTimeoutCallback,
• stopPassingThroughCalls: () => void, +) => SetTimeoutCallback;

• +/**

• • A class that provides a mock implementation for setTimeout which records
• • the callback given so that it can be replayed later.
• */ +class SetTimeoutRecorder {
• private originalSetTimeout: OriginalSetTimeout;

Nit: These can be ECMA private fields

In tests/recordCallsToSetTimeout.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r862171957 :

• • The function with which to replace the global clearTimeout function. This
• • mock implementation will find a call to setTimeout that returned the
• • given Timeout object and remove it from the queue. If no such call has been
• • made, then this does nothing.
• *
• • @param timeout - A Timeout object as returned by setTimeout.
• */
• _mockClearTimeoutImplementation = (timeout: NodeJS.Timeout): void => {
• const index = this.calls.findIndex((c) => c.timeout === timeout);
• if (index !== -1) {
• this.calls.splice(index, 1);
• }
• };
• _stopPassingThroughCalls = () => {

Nit: this seems like it should be a private field or method

In tests/recordCallsToSetTimeout.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r862181029 :

• return new Promise((resolve) => {
• const index = this.calls.findIndex((call) => call.duration === duration);
• if (index === -1) {
• const listener = (call: SetTimeoutCall, callIndex: number) => {
• if (call.duration === duration) {
• call.callback();
• this.calls.splice(callIndex, 1);
• this.events.off('setTimeoutAdded', listener);
• resolve();
• }
• };
• this.events.on('setTimeoutAdded', listener);
• } else {
• this.calls[index].callback();
• this.calls.splice(index, 1);

Should this come before the previous line? I was thinking, if this callback triggers something that also calls nextMatchingDuration, this might end up doing the wrong thing (i.e. it might be at a different index than before). If that happened it would be very tricky to debug.

In tests/setupAfterEnv.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r861742478 :

+

• let resolutionValue: any;
• let rejectionValue: any;
• try {
• resolutionValue = await Promise.race([
• promise,
• treatUnresolvedAfter(TIME_TO_WAIT_UNTIL_UNRESOLVED),
• ]);
• } catch (e) {
• rejectionValue = e;
• }
• return resolutionValue === UNRESOLVED
• ? {
• message: () =>
• Expected promise to resolve after ${TIME_TO_WAIT_UNTIL_UNRESOLVED}ms, but it did not,

Ah I see, makes sense

In tests/recordCallsToSetTimeout.ts https://github.com/MetaMask/eth-block-tracker/pull/103#discussion_r862183407 :

• return new Promise((resolve) => {
• const index = this.calls.findIndex((call) => call.duration === duration);
• if (index === -1) {
• const listener = (call: SetTimeoutCall, callIndex: number) => {
• if (call.duration === duration) {
• call.callback();
• this.calls.splice(callIndex, 1);
• this.events.off('setTimeoutAdded', listener);
• resolve();
• }
• };
• this.events.on('setTimeoutAdded', listener);
• } else {
• this.calls[index].callback();
• this.calls.splice(index, 1);

The same goes for line 68 - we should remove the call before calling the callback.

— Reply to this email directly, view it on GitHub https://github.com/MetaMask/eth-block-tracker/pull/103#pullrequestreview-957642683, or unsubscribe https://github.com/notifications/unsubscribe-auth/AV4JIUWCQF2PYBKAGQC447LVHRJSBANCNFSM5UAQN4PQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Alex mackenzie

[Gudahtt]

@macky22am you must have watched this repository, subscribing to all updates. Log in to GitHub and unwatch the repository, or update your GitHub notification settings to stop getting emails.

Please do not reply to this conversation anymore. Contact GitHub support if you require assistance.

[socket-security[bot]]

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package	Script field	Location
@swc/core@1.2.223 (added)	postinstall	package.json via ts-node@10.7.0

Socket.dev scan summary

Issue	Status
Did you mean?	✅ no new possible package typos
Install scripts	⚠️ 1 new install script detected
Telemetry	✅ no new telemetry
Troll package	✅ no new troll packages
Malware	✅ no new malware
Native code	✅ no new native modules

Powered by socket.dev

[naugtur]

Let me know what you did when you saw the socket.dev warning. I'm hoping to collect feedback on how you used it.

[mcmire]

@naugtur I'm wondering if this is a false negative. I looked at the package.json for ts-node and it looks like @swc/core is a development dependency. Additionally if I run yarn why @swc/core locally, then Yarn says it can't find anything. Therefore, although it's interesting that @swc/core has a postinstall script, I don't think it applies in our case.

Do we have any way to tweak what Socket is doing?

[Gudahtt]

I think we can add a socket.yml file to ignore this warning (see https://socket.dev/docs)

[mcmire]

@Gudahtt Hmm. That would appear to let us ignore warnings for packages but not ignore specific types of warnings.