From 40810f2fdbafdb955a6d919adb91de279dfe3e12 Mon Sep 17 00:00:00 2001 From: Jaco Date: Fri, 17 Feb 2023 11:57:34 +0200 Subject: [PATCH] Cleanup tests (& fix ws.connect()) (#5493) * Cleanup tests (& fix ws.connect()) * Await mock * Adjust sc connect ordering --- CHANGELOG.md | 7 ++ packages/api-derive/src/index.spec.ts | 24 ++--- packages/api/src/promise/index.spec.ts | 3 - packages/api/src/util/augmentObject.spec.ts | 9 +- packages/rpc-core/src/util/drr.spec.ts | 5 - packages/rpc-provider/src/http/send.spec.ts | 4 +- packages/rpc-provider/src/mock/mockWs.ts | 6 +- packages/rpc-provider/src/mock/types.ts | 2 +- .../src/substrate-connect/index.spec.ts | 81 ++++++++------- packages/rpc-provider/src/ws/connect.spec.ts | 98 ++++++++++--------- packages/rpc-provider/src/ws/index.spec.ts | 2 +- packages/rpc-provider/src/ws/index.ts | 4 + packages/rpc-provider/src/ws/send.spec.ts | 2 +- .../rpc-provider/src/ws/subscribe.spec.ts | 2 +- .../rpc-provider/src/ws/unsubscribe.spec.ts | 2 +- .../src/extrinsic/v4/ExtrinsicPayload.spec.ts | 2 +- 16 files changed, 125 insertions(+), 128 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9e9014ffe..d637934b89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # CHANGELOG +## master + +Changes: + +- Don't allow `WsProvider.connect()` on an open connection (creates resource leaks) + + ## 9.14.1 Feb 12, 2023 Changes: diff --git a/packages/api-derive/src/index.spec.ts b/packages/api-derive/src/index.spec.ts index 5f6fafcdc1..1c70376dbe 100644 --- a/packages/api-derive/src/index.spec.ts +++ b/packages/api-derive/src/index.spec.ts @@ -27,19 +27,20 @@ const testFunction = (api: ApiRx): any => { }; }; +function waitReady (api: ApiRx): Promise { + return new Promise((resolve) => + api.isReady.subscribe((api) => resolve(api)) + ); +} + describe('derive', (): void => { const registry = new TypeRegistry(); describe('builtin', (): void => { const api = new ApiRx({ provider: new MockProvider(registry), registry }); - beforeAll((done): void => { - api.isReady.subscribe(() => done()); - }); - - afterAll(async () => { - await api.disconnect(); - }); + beforeAll(() => waitReady(api)); + afterAll(() => api.disconnect()); testFunction(api)('accounts', 'idAndIndex', []); testFunction(api)('accounts', 'idToIndex', []); @@ -81,13 +82,8 @@ describe('derive', (): void => { throwOnConnect: true }); - beforeAll((done): void => { - api.isReady.subscribe(() => done()); - }); - - afterAll(async () => { - await api.disconnect(); - }); + beforeAll(() => waitReady(api)); + afterAll(() => api.disconnect()); // override testFunction(api)('balances', 'fees', ['a', 'b']); diff --git a/packages/api/src/promise/index.spec.ts b/packages/api/src/promise/index.spec.ts index 0286e54cc0..94f6d3008b 100644 --- a/packages/api/src/promise/index.spec.ts +++ b/packages/api/src/promise/index.spec.ts @@ -4,8 +4,6 @@ import type { HexString } from '@polkadot/util/types'; import type { SubmittableExtrinsic } from '../types'; -import { jest } from '@jest/globals'; - import { createPair } from '@polkadot/keyring/pair'; import { createTestKeyring } from '@polkadot/keyring/testing'; import { MockProvider } from '@polkadot/rpc-provider/mock'; @@ -47,7 +45,6 @@ describe('ApiPromise', (): void => { } beforeEach((): void => { - jest.setTimeout(10000); provider = new MockProvider(registry); }); diff --git a/packages/api/src/util/augmentObject.spec.ts b/packages/api/src/util/augmentObject.spec.ts index 663abc1d82..0eca30bbf7 100644 --- a/packages/api/src/util/augmentObject.spec.ts +++ b/packages/api/src/util/augmentObject.spec.ts @@ -4,15 +4,14 @@ import { augmentObject } from './augmentObject'; describe('augmentObject', (): void => { - let spy: any; + let spy: jest.SpyInstance; beforeEach((): void => { spy = jest.spyOn(console, 'warn'); }); afterEach((): void => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-call,@typescript-eslint/no-unsafe-member-access - spy.mockClear(); + spy.mockRestore(); }); it('logs added/removed sections and methods', (): void => { @@ -24,12 +23,12 @@ describe('augmentObject', (): void => { expect(spy).toHaveBeenCalledWith( expect.anything(), - expect.stringContaining('API/AUGMENT'), + expect.stringMatching(/API\/AUGMENT/), 'api.test: Found 1 added and 1 removed modules:\n\t added: new\n\tremoved: baz' ); expect(spy).toHaveBeenCalledWith( expect.anything(), - expect.stringContaining('API/AUGMENT'), + expect.stringMatching(/API\/AUGMENT/), 'api.test: Found 2 added and 3 removed calls:\n\t added: bar.b, foo.d\n\tremoved: bar.a, bar.c, foo.c' ); }); diff --git a/packages/rpc-core/src/util/drr.spec.ts b/packages/rpc-core/src/util/drr.spec.ts index 82c6197ce0..8147c3db6b 100644 --- a/packages/rpc-core/src/util/drr.spec.ts +++ b/packages/rpc-core/src/util/drr.spec.ts @@ -1,16 +1,11 @@ // Copyright 2017-2023 @polkadot/api-derive authors & contributors // SPDX-License-Identifier: Apache-2.0 -import { jest } from '@jest/globals'; import { of, timer } from 'rxjs'; import { drr } from '.'; describe('drr', (): void => { - beforeEach((): void => { - jest.setTimeout(15000); - }); - it('should not fire twice the same value', (done): void => { let count = 0; diff --git a/packages/rpc-provider/src/http/send.spec.ts b/packages/rpc-provider/src/http/send.spec.ts index 324dbda4f8..6d062dd6fe 100644 --- a/packages/rpc-provider/src/http/send.spec.ts +++ b/packages/rpc-provider/src/http/send.spec.ts @@ -16,9 +16,9 @@ describe.skip('send', (): void => { http = new HttpProvider(TEST_HTTP_URL); }); - afterEach((): void => { + afterEach(async () => { if (mock) { - mock.done(); + await mock.done(); } }); diff --git a/packages/rpc-provider/src/mock/mockWs.ts b/packages/rpc-provider/src/mock/mockWs.ts index a2929a5a28..9fbc33cf20 100644 --- a/packages/rpc-provider/src/mock/mockWs.ts +++ b/packages/rpc-provider/src/mock/mockWs.ts @@ -69,11 +69,7 @@ export function mockWs (requests: Request[], wsUrl: string = TEST_WS_URL): Scope let requestCount = 0; const scope: Scope = { body: {}, - done: (): void => { - server.stop((): void => { - // ignore - }); - }, + done: () => new Promise((resolve) => server.stop(resolve)), requests: 0, server }; diff --git a/packages/rpc-provider/src/mock/types.ts b/packages/rpc-provider/src/mock/types.ts index 509cf99901..87fbbe52f6 100644 --- a/packages/rpc-provider/src/mock/types.ts +++ b/packages/rpc-provider/src/mock/types.ts @@ -14,7 +14,7 @@ export interface Mock { body: Record>; requests: number; server: Server; - done: () => Record; + done: () => Promise; } export type MockStateSubscriptionCallback = (error: Error | null, value: any) => void; diff --git a/packages/rpc-provider/src/substrate-connect/index.spec.ts b/packages/rpc-provider/src/substrate-connect/index.spec.ts index 3b02bfeb0b..1ab53a5c94 100644 --- a/packages/rpc-provider/src/substrate-connect/index.spec.ts +++ b/packages/rpc-provider/src/substrate-connect/index.spec.ts @@ -21,18 +21,33 @@ import type Sc from '@substrate/connect'; import type { HealthChecker, SmoldotHealth } from './types'; -import { jest } from '@jest/globals'; - import { ScProvider } from '.'; -const wait = (ms: number) => new Promise((res) => setTimeout(res, ms)); +type MockChain = Sc.Chain & { + _spec: () => string + _recevedRequests: () => string[] + _isTerminated: () => boolean + _triggerCallback: (response: string | {}) => void + _setTerminateInterceptor: (fn: () => void) => void + _setSendJsonRpcInterceptor: (fn: (rpc: string) => void) => void + _getLatestRequest: () => string +} type MockedHealthChecker = HealthChecker & { _isActive: () => boolean _triggerHealthUpdate: (update: SmoldotHealth) => void } -const healthCheckerMock = (): MockedHealthChecker => { +enum WellKnownChain { + polkadot = 'polkadot', + ksmcc3 = 'ksmcc3', + rococo_v2_2 = 'rococo_v2_2', + westend2 = 'westend2' +} + +const wait = (ms: number) => new Promise((res) => setTimeout(res, ms)); + +function healthCheckerMock (): MockedHealthChecker { let cb: (health: SmoldotHealth) => void = () => {}; let sendJsonRpc: (request: string) => void = () => {}; @@ -57,9 +72,9 @@ const healthCheckerMock = (): MockedHealthChecker => { cb(update); } }; -}; +} -const healthCheckerFactory = () => { +function healthCheckerFactory () { const _healthCheckers: MockedHealthChecker[] = []; return { @@ -73,21 +88,11 @@ const healthCheckerFactory = () => { _healthCheckers, _latestHealthChecker: () => _healthCheckers.slice(-1)[0] }; -}; +} jest.mock('./Health', () => healthCheckerFactory()); -type MockChain = Sc.Chain & { - _spec: () => string - _recevedRequests: () => string[] - _isTerminated: () => boolean - _triggerCallback: (response: string | {}) => void - _setTerminateInterceptor: (fn: () => void) => void - _setSendJsonRpcInterceptor: (fn: (rpc: string) => void) => void - _getLatestRequest: () => string -} - -const getFakeChain = (spec: string, callback: Sc.JsonRpcCallback): MockChain => { +function getFakeChain (spec: string, callback: Sc.JsonRpcCallback): MockChain { const _receivedRequests: string[] = []; let _isTerminated = false; @@ -119,9 +124,9 @@ const getFakeChain = (spec: string, callback: Sc.JsonRpcCallback): MockChain => }, _getLatestRequest: () => _receivedRequests[_receivedRequests.length - 1] }; -}; +} -const getFakeClient = () => { +function getFakeClient () { const chains: MockChain[] = []; let addChainInterceptor: Promise = Promise.resolve(); let addWellKnownChainInterceptor: Promise = Promise.resolve(); @@ -154,16 +159,9 @@ const getFakeClient = () => { return result; }) }; -}; - -enum WellKnownChain { - polkadot = 'polkadot', - ksmcc3 = 'ksmcc3', - rococo_v2_2 = 'rococo_v2_2', - westend2 = 'westend2' } -const connectorFactory = () => { +function connectorFactory () { const clients: ReturnType[] = []; const latestClient = () => clients[clients.length - 1]; @@ -181,26 +179,26 @@ const connectorFactory = () => { latestChain: () => latestClient()._chains()[latestClient()._chains().length - 1] }; -}; +} -let mockSc: typeof Sc & { latestChain: () => MockChain }; -let mockedHealthChecker: ReturnType; -const getCurrentHealthChecker = () => mockedHealthChecker._latestHealthChecker(); - -const setChainSyncyingStatus = (isSyncing: boolean) => { +function setChainSyncyingStatus (isSyncing: boolean) { getCurrentHealthChecker()._triggerHealthUpdate({ isSyncing, peers: 1, shouldHavePeers: true }); -}; +} -beforeAll(() => { - mockSc = connectorFactory() as unknown as typeof Sc & { latestChain: () => MockChain }; - mockedHealthChecker = healthCheckerFactory(); -}); +let mockSc: typeof Sc & { latestChain: () => MockChain }; +let mockedHealthChecker: ReturnType; +const getCurrentHealthChecker = () => mockedHealthChecker._latestHealthChecker(); describe('ScProvider', () => { + beforeAll(() => { + mockSc = connectorFactory() as unknown as typeof Sc & { latestChain: () => MockChain }; + mockedHealthChecker = healthCheckerFactory(); + }); + describe('on', () => { it('emits `connected` as soon as the chain is not syncing', async () => { const provider = new ScProvider(mockSc, ''); @@ -551,8 +549,7 @@ describe('ScProvider', () => { expect(token).toBe(unsubscribeToken); expect(cb).toHaveBeenCalledTimes(1); - expect(cb.mock.lastCall![0]).toBeInstanceOf(Error); - expect(cb.mock.lastCall![1]).toBe(undefined); + expect(cb).toHaveBeenLastCalledWith(expect.any(Error), undefined); }); it('errors when subscribing to an unsupported method', async () => { @@ -561,8 +558,8 @@ describe('ScProvider', () => { await provider.connect(undefined, mockedHealthChecker.healthChecker); setChainSyncyingStatus(false); - await wait(0); + await wait(0); await expect( provider.subscribe('foo', 'bar', ['baz'], () => {}) ).rejects.toThrow('Unsupported subscribe method: bar'); diff --git a/packages/rpc-provider/src/ws/connect.spec.ts b/packages/rpc-provider/src/ws/connect.spec.ts index fd7cfbffc4..71e762f900 100644 --- a/packages/rpc-provider/src/ws/connect.spec.ts +++ b/packages/rpc-provider/src/ws/connect.spec.ts @@ -3,97 +3,104 @@ import type { Mock } from '../mock/types'; -import { jest } from '@jest/globals'; - import { mockWs } from '../mock/mockWs'; import { WsProvider } from './'; const TEST_WS_URL = 'ws://localhost-connect.spec.ts:9988'; -function sleepMs (ms = 0): Promise { +function sleep (ms = 100): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } describe('onConnect', (): void => { let mocks: Mock[]; + let provider: WsProvider | null; beforeEach((): void => { - jest.setTimeout(30000); mocks = [mockWs([], TEST_WS_URL)]; }); - afterEach((): void => { - jest.setTimeout(5000); - mocks.forEach((m) => { - if (m) { - m.done(); - } - }); + afterEach(async () => { + if (provider) { + await provider.disconnect(); + await sleep(); + + provider = null; + } + + await Promise.all(mocks.map((m) => m.done())); + await sleep(); }); it('Does not connect when autoConnect is false', async () => { - const provider: WsProvider = new WsProvider(TEST_WS_URL, 0); + provider = new WsProvider(TEST_WS_URL, 0); + + await sleep(); - await sleepMs(100); // Hack to give the provider time to connect expect(provider.isConnected).toBe(false); await provider.connect(); - await sleepMs(100); // Hack to give the provider time to connect + await sleep(); + expect(provider.isConnected).toBe(true); await provider.disconnect(); + await sleep(); - await sleepMs(100); // Hack to give the provider time to connect expect(provider.isConnected).toBe(false); }); it('Does connect when autoConnect is true', async () => { - const provider: WsProvider = new WsProvider(TEST_WS_URL, 1); + provider = new WsProvider(TEST_WS_URL, 1); + + await sleep(); - await sleepMs(100); // Hack to give the provider time to connect expect(provider.isConnected).toBe(true); - - await provider.disconnect(); }); it('Creates a new WebSocket instance by calling the connect() method', async () => { - const provider: WsProvider = new WsProvider(TEST_WS_URL, false); + provider = new WsProvider(TEST_WS_URL, false); expect(provider.isConnected).toBe(false); expect(mocks[0].server.clients().length).toBe(0); await provider.connect(); - await sleepMs(100); // Hack to give the provider time to connect + await sleep(); expect(provider.isConnected).toBe(true); expect(mocks[0].server.clients()).toHaveLength(1); - - await provider.disconnect(); }); it('Connects to first endpoint when an array is given', async () => { - const provider: WsProvider = new WsProvider([TEST_WS_URL], 1); + provider = new WsProvider([TEST_WS_URL], 1); - await provider.connect(); - await sleepMs(100); // Hack to give the provider time to connect + await sleep(); + + expect(provider.isConnected).toBe(true); + expect(mocks[0].server.clients()).toHaveLength(1); + }); + + it('Does not allow connect() on already-connected', async () => { + provider = new WsProvider([TEST_WS_URL], 1); + + await sleep(); expect(provider.isConnected).toBe(true); - await provider.disconnect(); + await expect( + provider.connect() + ).rejects.toThrow(/already connected/); }); it('Connects to the second endpoint when the first is unreachable', async () => { - /* eslint-disable @typescript-eslint/no-empty-function */ - jest.spyOn(console, 'error').mockImplementation((): void => {}); + const endpoints: string[] = ['ws://localhost-unreachable-connect.spec.ts:9956', TEST_WS_URL]; - const endpoints: string[] = ['ws://localhost-connect.spec.ts:9956', TEST_WS_URL]; - const provider: WsProvider = new WsProvider(endpoints, 1); + provider = new WsProvider(endpoints, 1); - await sleepMs(100); // Hack to give the provider time to connect + await sleep(); + expect(mocks[0].server.clients()).toHaveLength(1); expect(provider.isConnected).toBe(true); - - await provider.disconnect(); }); it('Connects to the second endpoint when the first is dropped', async () => { @@ -101,22 +108,22 @@ describe('onConnect', (): void => { mocks.push(mockWs([], endpoints[1])); - const provider: WsProvider = new WsProvider(endpoints, 1); + provider = new WsProvider(endpoints, 1); + + await sleep(); - await sleepMs(100); // Hack to give the provider time to connect // Check that first server is connected expect(mocks[0].server.clients()).toHaveLength(1); expect(mocks[1].server.clients()).toHaveLength(0); // Close connection from first server mocks[0].server.clients()[0].close(); - await sleepMs(100); + + await sleep(); // Check that second server is connected expect(mocks[1].server.clients()).toHaveLength(1); expect(provider.isConnected).toBe(true); - - await provider.disconnect(); }); it('Round-robin of endpoints on WsProvider', async () => { @@ -138,22 +145,21 @@ describe('onConnect', (): void => { mocks[3], mocks[0] ]; - const provider: WsProvider = new WsProvider(endpoints, 1); - for (let round = 0; round < 5; round++) { + provider = new WsProvider(endpoints, 1); + + for (let round = 0; round < 2; round++) { for (let mock = 0; mock < mocks.length; mock++) { - await sleepMs(100); // Hack to give the provider time to connect + await sleep(); - // Check that first server is connected and the next one isn't + // Wwe are connected, the current mock has the connection and the next doesn't + expect(provider.isConnected).toBe(true); expect(mocks[mock].server.clients()).toHaveLength(1); expect(mockNext[mock].server.clients()).toHaveLength(0); - expect(provider.isConnected).toBe(true); // Close connection from first server mocks[mock].server.clients()[0].close(); } } - - await provider.disconnect(); }); }); diff --git a/packages/rpc-provider/src/ws/index.spec.ts b/packages/rpc-provider/src/ws/index.spec.ts index 6294e6b6ac..4558a2a54c 100644 --- a/packages/rpc-provider/src/ws/index.spec.ts +++ b/packages/rpc-provider/src/ws/index.spec.ts @@ -22,7 +22,7 @@ function createWs (requests: Request[], autoConnect = 1000, headers?: Record { afterEach(async () => { if (mock) { - mock.done(); + await mock.done(); } if (provider) { diff --git a/packages/rpc-provider/src/ws/index.ts b/packages/rpc-provider/src/ws/index.ts index dd83ef335d..26d874ed45 100644 --- a/packages/rpc-provider/src/ws/index.ts +++ b/packages/rpc-provider/src/ws/index.ts @@ -188,6 +188,10 @@ export class WsProvider implements ProviderInterface { */ // eslint-disable-next-line @typescript-eslint/require-await public async connect (): Promise { + if (this.#websocket) { + throw new Error('WebSocket is already connected'); + } + try { this.#endpointIndex = (this.#endpointIndex + 1) % this.#endpoints.length; diff --git a/packages/rpc-provider/src/ws/send.spec.ts b/packages/rpc-provider/src/ws/send.spec.ts index 385a37630e..9a3e8ae05a 100644 --- a/packages/rpc-provider/src/ws/send.spec.ts +++ b/packages/rpc-provider/src/ws/send.spec.ts @@ -36,7 +36,7 @@ describe('send', (): void => { global.WebSocket = globalWs; if (mock) { - mock.done(); + await mock.done(); } if (provider) { diff --git a/packages/rpc-provider/src/ws/subscribe.spec.ts b/packages/rpc-provider/src/ws/subscribe.spec.ts index 21e8a92834..48967b46a7 100644 --- a/packages/rpc-provider/src/ws/subscribe.spec.ts +++ b/packages/rpc-provider/src/ws/subscribe.spec.ts @@ -36,7 +36,7 @@ describe('subscribe', (): void => { global.WebSocket = globalWs; if (mock) { - mock.done(); + await mock.done(); } if (provider) { diff --git a/packages/rpc-provider/src/ws/unsubscribe.spec.ts b/packages/rpc-provider/src/ws/unsubscribe.spec.ts index e3dab606b5..1432e918b2 100644 --- a/packages/rpc-provider/src/ws/unsubscribe.spec.ts +++ b/packages/rpc-provider/src/ws/unsubscribe.spec.ts @@ -36,7 +36,7 @@ describe('subscribe', (): void => { global.WebSocket = globalWs; if (mock) { - mock.done(); + await mock.done(); } if (provider) { diff --git a/packages/types/src/extrinsic/v4/ExtrinsicPayload.spec.ts b/packages/types/src/extrinsic/v4/ExtrinsicPayload.spec.ts index 9dee87b5f1..86332dba8f 100644 --- a/packages/types/src/extrinsic/v4/ExtrinsicPayload.spec.ts +++ b/packages/types/src/extrinsic/v4/ExtrinsicPayload.spec.ts @@ -21,7 +21,7 @@ describe('ExtrinsicPayload', (): void => { expect(new ExtrinsicPayload(registry, { method: tx.timestamp.set(0).toHex() } as never).inspect()).toEqual({ inner: [ { name: 'method', outer: [new Uint8Array([3, 0, 0])] }, - { name: 'era', outer: [new Uint8Array([0]), new Uint8Array([0])] }, + { inner: undefined, name: 'era', outer: [new Uint8Array([0]), new Uint8Array([0])] }, { name: 'nonce', outer: [new Uint8Array([0])] }, { name: 'tip', outer: [new Uint8Array([0])] }, { name: 'assetId', outer: [new Uint8Array([0])] },