-
Notifications
You must be signed in to change notification settings - Fork 89
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: update test memory leaks #2394
Changes from 13 commits
a91f1d1
44e5b45
e8ad193
84fccd2
44b4b91
bab8743
7658266
c369d7a
d867f5a
1d98844
6f87148
6502ccf
d480271
9b4b53e
0d31f99
9ab8613
74c40da
924b90f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { TestEnvironment } from 'jest-environment-node' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main change. Described here: nock/nock#2358 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess these nock changes are needed even after this fix? nock/nock#2572 It looks like we are on the latest and have this fix. I suppose there were two issues:
|
||
import nock from 'nock' | ||
|
||
export default class CustomEnvironment extends TestEnvironment { | ||
constructor(config, context) { | ||
super(config, context) | ||
this.global.nock = nock | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
require('ts-node/register') | ||
|
||
import { knex } from 'knex' | ||
import { GenericContainer, Wait } from 'testcontainers' | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { startTigerbeetleContainer } from './src/tests/tigerbeetle' | ||
import { StartedTestContainer } from 'testcontainers' | ||
|
||
import CustomTestEnvironment from './jest.custom-environment' | ||
|
||
export default class TigerbeetleEnvironment extends CustomTestEnvironment { | ||
private tbContainer: StartedTestContainer | undefined | ||
|
||
public async setup(): Promise<void> { | ||
await super.setup() | ||
const tbContainer = await startTigerbeetleContainer() | ||
|
||
this.tbContainer = tbContainer.container | ||
this.global.tigerbeetlePort = tbContainer.port | ||
} | ||
|
||
public async teardown(): Promise<void> { | ||
await super.teardown() | ||
await this.tbContainer?.stop() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
/** | ||
* @jest-environment ./packages/backend/jest.tigerbeetle-environment.ts | ||
*/ | ||
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. specifies specific environment for this test suite |
||
|
||
import assert from 'assert' | ||
import { CreateAccountError as CreateTbAccountError } from 'tigerbeetle-node' | ||
import { v4 as uuid } from 'uuid' | ||
|
@@ -9,7 +13,6 @@ import { IocContract } from '@adonisjs/fold' | |
import { initIocContainer } from '../../' | ||
import { AppServices } from '../../app' | ||
import { truncateTables } from '../../tests/tableManager' | ||
import { startTigerbeetleContainer } from '../../tests/tigerbeetle' | ||
import { AccountFactory, FactoryAccount } from '../../tests/accountFactory' | ||
import { isTransferError, TransferError } from '../errors' | ||
import { | ||
|
@@ -20,7 +23,7 @@ import { | |
Withdrawal | ||
} from '../service' | ||
|
||
describe('Accounting Service', (): void => { | ||
describe('Tigerbeetle Accounting Service', (): void => { | ||
let deps: IocContract<AppServices> | ||
let appContainer: TestContainer | ||
let accountingService: AccountingService | ||
|
@@ -33,10 +36,14 @@ describe('Accounting Service', (): void => { | |
} | ||
|
||
beforeAll(async (): Promise<void> => { | ||
const { port } = await startTigerbeetleContainer() | ||
Config.tigerbeetleReplicaAddresses = [port.toString()] | ||
const tigerbeetlePort = (global as unknown as { tigerbeetlePort: number }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gets injected via the tigerbeetle test environment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like this suggestion applies here as well: https://github.com/interledger/rafiki/pull/2394/files#r1480529987 |
||
.tigerbeetlePort | ||
|
||
deps = await initIocContainer({ ...Config, useTigerbeetle: true }) | ||
deps = initIocContainer({ | ||
...Config, | ||
tigerbeetleReplicaAddresses: [tigerbeetlePort.toString()], | ||
useTigerbeetle: true | ||
}) | ||
appContainer = await createTestApp(deps) | ||
accountingService = await deps.use('accountingService') | ||
accountFactory = new AccountFactory(accountingService, newLedger) | ||
|
@@ -48,7 +55,6 @@ describe('Accounting Service', (): void => { | |
|
||
afterAll(async (): Promise<void> => { | ||
await appContainer.shutdown() | ||
// TODO: find a way to gracefully stop TB container without running into a thread panic | ||
}) | ||
|
||
describe('Create Liquidity Account', (): void => { | ||
|
@@ -83,11 +89,11 @@ describe('Accounting Service', (): void => { | |
}, | ||
LiquidityAccountType.ASSET | ||
) | ||
).rejects.toThrowError('unable to create account, invalid id') | ||
).rejects.toThrow('unable to create account, invalid id') | ||
}) | ||
|
||
test('Create throws on error', async (): Promise<void> => { | ||
const tigerbeetle = await deps.use('tigerbeetle') | ||
const tigerbeetle = await deps.use('tigerbeetle')! | ||
jest.spyOn(tigerbeetle, 'createAccounts').mockResolvedValueOnce([ | ||
{ | ||
index: 0, | ||
|
@@ -106,7 +112,7 @@ describe('Accounting Service', (): void => { | |
}, | ||
LiquidityAccountType.ASSET | ||
) | ||
).rejects.toThrowError( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using |
||
).rejects.toThrow( | ||
new TigerbeetleCreateAccountError( | ||
CreateTbAccountError.exists_with_different_ledger | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,7 +236,7 @@ export interface AppServices { | |
autoPeeringService: Promise<AutoPeeringService> | ||
autoPeeringRoutes: Promise<AutoPeeringRoutes> | ||
connectorApp: Promise<ConnectorApp> | ||
tigerbeetle: Promise<TigerbeetleClient> | ||
tigerbeetle?: Promise<TigerbeetleClient> | ||
paymentMethodHandlerService: Promise<PaymentMethodHandlerService> | ||
ilpPaymentService: Promise<IlpPaymentService> | ||
} | ||
|
@@ -611,8 +611,8 @@ export class App { | |
if (this.openPaymentsServer) { | ||
await this.stopServer(this.openPaymentsServer) | ||
} | ||
if (this.adminServer) { | ||
await this.stopServer(this.adminServer) | ||
if (this.apolloServer) { | ||
await this.apolloServer.stop() | ||
} | ||
Comment on lines
+618
to
620
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we do |
||
if (this.ilpConnectorService) { | ||
await this.stopServer(this.ilpConnectorService) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
import { faker } from '@faker-js/faker' | ||
import { gql } from '@apollo/client' | ||
import assert from 'assert' | ||
import nock from 'nock' | ||
|
||
import { createTestApp, TestContainer } from '../../tests/app' | ||
import { IocContract } from '@adonisjs/fold' | ||
|
@@ -20,6 +19,8 @@ import { CreateOrUpdatePeerByUrlInput } from '../generated/graphql' | |
import { AutoPeeringService } from '../../payment-method/ilp/auto-peering/service' | ||
import { v4 as uuid } from 'uuid' | ||
|
||
const nock = (global as unknown as { nock: typeof import('nock') }).nock | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about this? add this to a new file at declare module globalThis {
var nock: typeof import('nock')
} which lets us just do: const nock = global.nock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although I dont love that this suggests nock is on global anywhere (including outside tests, or outside the that specific test environment) ... was just thinking maybe we could avoid doing all this casting every time we want to use it. alternatively maybe we can initialize it like this once somewhere and export it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too fond of this long casting, but I think I prefer not mixing the shipped code vs the testing code IMO |
||
|
||
describe('Auto Peering Resolvers', (): void => { | ||
let deps: IocContract<AppServices> | ||
let appContainer: TestContainer | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,12 +13,12 @@ describe('Redis Data Store', (): void => { | |
const dataStore = createRedisDataStore(redis, ttlMs) | ||
|
||
afterEach(async () => { | ||
jest.useRealTimers() | ||
await redis.flushall() | ||
}) | ||
|
||
afterAll(async () => { | ||
redis.disconnect() | ||
jest.useRealTimers() | ||
await redis.quit() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems to be the preferred way to stop redis connections: redis/ioredis#1088 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about resetting the timers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved it to the |
||
}) | ||
|
||
describe('set', (): void => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import { faker } from '@faker-js/faker' | |
|
||
export const IlpPrepareFactory = Factory.define<IlpPrepare>('IlpPrepare').attrs( | ||
{ | ||
amount: faker.finance.amount(1, 100, 0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this type signature is deprecated (was giving me warnings) |
||
amount: faker.finance.amount({ min: 1, max: 100, dec: 0 }), | ||
data: Buffer.alloc(0), | ||
destination: 'test.rafiki.' + faker.person.firstName(), | ||
expiresAt: new Date(Date.now() + 10 * 1000), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use
@swc/jest
instead, not neededThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like its used here? https://github.com/interledger/rafiki/blob/main/jest.config.js#L4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlairCurrey was about to merge until I saw this, fixed now 👍