chore: update test memory leaks #2394

mkurapov · 2024-02-05T15:51:53Z

Changes proposed in this pull request

Fixes memory leak related to nock, backend tests can now be run serially (via --runInBand) without running out of memory. This was verified using the --detectLeaks flag for jest tests, which would throw an error if detected a memory leak during a test.
Properly spins down tigerbeetle container during tests using a custom environment
Sets log level to silent for tests
Improves commands related to app shutdown & tests teardown (should help with the amount of open handles the tests have). I fixed this error message:

"A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them."

when running the test:ci command, but at 100% workers, it still appears on my machine 🤷 . But at least some progress there.

Context

Fixes #2393
Fixes #1094 (even though it seemed like the thread panic wasn't an issue with newer versions of tigerbeetle)

Checklist

netlify · 2024-02-05T15:51:57Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`924b90f`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65ddb68435417d000885c6b2

mkurapov · 2024-02-05T16:23:36Z

package.json

@@ -43,7 +43,6 @@
    "jest": "^29.7.0",
    "npm-run-all2": "^5.0.2",
    "prettier": "^3.2.4",
-    "ts-jest": "^29.1.2",


we use @swc/jest instead, not needed

looks like its used here? https://github.com/interledger/rafiki/blob/main/jest.config.js#L4

@BlairCurrey was about to merge until I saw this, fixed now 👍

mkurapov · 2024-02-05T17:18:45Z

packages/backend/src/app.ts

+    if (this.apolloServer) {
+      await this.apolloServer.stop()
    }


since we do plugins: [ApolloServerPluginDrainHttpServer({ httpServer })] when creating the apollo server, the httpServer will be automatically stopped/connections will be drained. However, before we weren't closing the actual apollo server that does other operations as well: https://www.apollographql.com/docs/apollo-server/api/apollo-server/#stop

mkurapov · 2024-02-05T17:24:32Z

packages/backend/src/middleware/cache/data-stores/redis.test.ts

    await redis.flushall()
  })

  afterAll(async () => {
-    redis.disconnect()
-    jest.useRealTimers()
+    await redis.quit()


this seems to be the preferred way to stop redis connections: redis/ioredis#1088

What about resetting the timers?

Moved it to the afterEach

mkurapov · 2024-02-05T17:25:25Z

packages/backend/src/accounting/tigerbeetle/service.test.ts

@@ -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 })


gets injected via the tigerbeetle test environment

looks like this suggestion applies here as well: https://github.com/interledger/rafiki/pull/2394/files#r1480529987

mkurapov · 2024-02-05T17:33:22Z

packages/backend/src/accounting/tigerbeetle/service.test.ts

@@ -106,7 +112,7 @@ describe('Accounting Service', (): void => {
          },
          LiquidityAccountType.ASSET
        )
-      ).rejects.toThrowError(


using toThrowError gave me a deprecated warning

mkurapov · 2024-02-05T17:33:45Z

packages/backend/src/payment-method/ilp/connector/core/factories/ilp-packet.ts

@@ -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),


this type signature is deprecated (was giving me warnings)

mkurapov · 2024-02-05T19:51:06Z

packages/backend/jest.custom-environment.ts

@@ -0,0 +1,9 @@
+import { TestEnvironment } from 'jest-environment-node'


Main change. Described here: nock/nock#2358

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:

nock leaking from simply being imported (fixed above)

us not cleaning it up properly which still require these changes

mkurapov · 2024-02-05T19:51:28Z

packages/backend/src/accounting/tigerbeetle/service.test.ts

+/**
+ * @jest-environment ./packages/backend/jest.tigerbeetle-environment.ts
+ */


specifies specific environment for this test suite

BlairCurrey · 2024-02-06T20:04:27Z

I was not familiar with --runInBand so I looked it up. In doing so it seems like it might be faster to use --runInBand in CI? This is tangential, but perhaps its worth testing if we haven't already. --runInBand took about 50% longer for me locally FWIW but thats on an m2 Max and the ubuntu runner is 4 cores/16gb memory so maybe it could be faster? 🤷‍♂️

BlairCurrey · 2024-02-06T20:55:35Z

packages/backend/src/graphql/resolvers/auto-peering.test.ts

@@ -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


What about this?

add this to a new file at rafiki/packages/backend/src/global.d.ts:

declare module globalThis { var nock: typeof import('nock') }

which lets us just do:

const nock = global.nock

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?

I'm not too fond of this long casting, but I think I prefer not mixing the shipped code vs the testing code IMO

BlairCurrey · 2024-02-06T21:11:13Z

Runs fine for me locally (including maxWorkers=100% for whatever that is worth)

sabineschaller · 2024-02-13T09:22:18Z

I was not familiar with --runInBand so I looked it up. In doing so it seems like it might be faster to use --runInBand in CI? This is tangential, but perhaps its worth testing if we haven't already. --runInBand took about 50% longer for me locally FWIW but thats on an m2 Max and the ubuntu runner is 4 cores/16gb memory so maybe it could be faster? 🤷‍♂️

Very interesting. I think we should give it a try. I also didn't know that flag.

sabineschaller · 2024-02-13T09:22:45Z

@mkurapov ♥️♥️♥️ Thank you for tackling this ♥️♥️♥️

# Conflicts: # package.json # pnpm-lock.yaml

mkurapov · 2024-02-15T16:29:54Z

@BlairCurrey --runInBand instead of --maxWorkers=2 in the CI took about 50% longer

https://github.com/interledger/rafiki/actions/runs/7919005784/job/21618820101 (6 mins)
https://github.com/interledger/rafiki/actions/runs/7788547057 (4 mins)

This reverts commit 9b4b53e.

sabineschaller · 2024-02-19T09:54:50Z

Are we ready to get this merged as is then?

mkurapov · 2024-02-19T10:04:59Z

@sabineschaller going to try one final test to move some imports around to see if the "fix" from the nock issue might work for us, otherwise, I'll merge

mkurapov · 2024-02-19T17:19:47Z

@sabineschaller yeah, the order of imports don't seem to make a difference in the repo

BlairCurrey · 2024-02-21T14:23:40Z

@BlairCurrey --runInBand instead of --maxWorkers=2 in the CI took about 50% longer

https://github.com/interledger/rafiki/actions/runs/7919005784/job/21618820101 (6 mins) https://github.com/interledger/rafiki/actions/runs/7788547057 (4 mins)

Good to know, thanks for testing it out.

# Conflicts: # packages/backend/package.json # packages/backend/src/index.ts # packages/backend/src/tests/app.ts # pnpm-lock.yaml

mkurapov added 2 commits February 5, 2024 12:54

chore: cleaning up main files

a91f1d1

chore: update all nock imports

44e5b45

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Feb 5, 2024

mkurapov changed the title ~~2393/mk/memory leak~~ chore: update test memory leaks Feb 5, 2024

mkurapov added 6 commits February 5, 2024 17:19

chore: fix timer mocks

e8ad193

chore: update redis spin down

84fccd2

chore: update faker method

44b4b91

chore: set log level to silent for tests

bab8743

chore: use tigerbeetle environment to spin down container

7658266

chore: remove ts-jest

c369d7a

mkurapov commented Feb 5, 2024

View reviewed changes

mkurapov added 2 commits February 5, 2024 18:01

chore: correct timer faking

d867f5a

chore: log heap usage

1d98844

mkurapov commented Feb 5, 2024

View reviewed changes

mkurapov added 3 commits February 5, 2024 18:35

chore: remove pino pretty in tests

6f87148

chore: dont log heap usage

6502ccf

chore: revert test timeout to default

d480271

mkurapov commented Feb 5, 2024

View reviewed changes

mkurapov marked this pull request as ready for review February 5, 2024 19:51

mkurapov requested review from njlie, BlairCurrey and sabineschaller February 5, 2024 19:51

BlairCurrey reviewed Feb 6, 2024

View reviewed changes

mkurapov added 2 commits February 15, 2024 17:12

ci: try --runInBand

9b4b53e

Merge branch 'main' into 2393/mk/memory-leak

0d31f99

# Conflicts: # package.json # pnpm-lock.yaml

Revert "ci: try --runInBand"

9ab8613

This reverts commit 9b4b53e.

mkurapov requested a review from BlairCurrey February 26, 2024 09:47

Merge branch 'main' into 2393/mk/memory-leak

74c40da

# Conflicts: # packages/backend/package.json # packages/backend/src/index.ts # packages/backend/src/tests/app.ts # pnpm-lock.yaml

BlairCurrey previously approved these changes Feb 26, 2024

View reviewed changes

chore: remove ts-jest from jest.config.js

924b90f

mkurapov dismissed BlairCurrey’s stale review via 924b90f February 27, 2024 10:16

BlairCurrey approved these changes Feb 27, 2024

View reviewed changes

mkurapov merged commit de4b0e3 into main Feb 27, 2024
22 checks passed

mkurapov deleted the 2393/mk/memory-leak branch February 27, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update test memory leaks #2394

chore: update test memory leaks #2394

mkurapov commented Feb 5, 2024 •

edited

Loading

netlify bot commented Feb 5, 2024 •

edited

Loading

mkurapov Feb 5, 2024

BlairCurrey Feb 6, 2024

mkurapov Feb 27, 2024

mkurapov Feb 5, 2024

mkurapov Feb 5, 2024

sabineschaller Feb 13, 2024

mkurapov Feb 13, 2024

mkurapov Feb 5, 2024

BlairCurrey Feb 6, 2024

mkurapov Feb 5, 2024

mkurapov Feb 5, 2024

mkurapov Feb 5, 2024

BlairCurrey Feb 6, 2024

mkurapov Feb 5, 2024

BlairCurrey commented Feb 6, 2024

BlairCurrey Feb 6, 2024

BlairCurrey Feb 6, 2024 •

edited

Loading

mkurapov Feb 15, 2024

BlairCurrey commented Feb 6, 2024

sabineschaller commented Feb 13, 2024

sabineschaller commented Feb 13, 2024

mkurapov commented Feb 15, 2024 •

edited

Loading

sabineschaller commented Feb 19, 2024

mkurapov commented Feb 19, 2024

mkurapov commented Feb 19, 2024

BlairCurrey commented Feb 21, 2024

		@@ -0,0 +1,9 @@
		import { TestEnvironment } from 'jest-environment-node'

chore: update test memory leaks #2394

chore: update test memory leaks #2394

Conversation

mkurapov commented Feb 5, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Feb 5, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Feb 6, 2024

Choose a reason for hiding this comment

BlairCurrey Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Feb 6, 2024

sabineschaller commented Feb 13, 2024

sabineschaller commented Feb 13, 2024

mkurapov commented Feb 15, 2024 • edited Loading

sabineschaller commented Feb 19, 2024

mkurapov commented Feb 19, 2024

mkurapov commented Feb 19, 2024

BlairCurrey commented Feb 21, 2024

mkurapov commented Feb 5, 2024 •

edited

Loading

netlify bot commented Feb 5, 2024 •

edited

Loading

BlairCurrey Feb 6, 2024 •

edited

Loading

mkurapov commented Feb 15, 2024 •

edited

Loading