From a9ab0cfd61a80ab317a6966e2498fa971624a11d Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 28 Apr 2022 14:27:57 -0700 Subject: [PATCH 1/6] feat: set up new test for beat twice --- test/unit/node/heart.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index 5360c865e131..3ad92078465a 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -75,6 +75,7 @@ describe("Heart", () => { const isAlive = heart.alive() expect(isAlive).toBe(false) }) + it.todo("should beat twice without warnings") }) describe("heartbeatTimer", () => { From e00d4b598193c42bbea5f584ac36bfba5e5809b3 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 28 Apr 2022 14:58:16 -0700 Subject: [PATCH 2/6] refactor: make Heart.beat() async This allows us to properly await heart.beat() in our tests and remove the HACK I added before. --- src/node/heart.ts | 16 ++++++++++++---- src/node/routes/index.ts | 2 ++ test/unit/node/heart.test.ts | 17 +++++------------ 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/node/heart.ts b/src/node/heart.ts index a03802035c38..c4ecc5ac3a36 100644 --- a/src/node/heart.ts +++ b/src/node/heart.ts @@ -20,20 +20,28 @@ export class Heart { * timeout and start or reset a timer that keeps running as long as there is * activity. Failures are logged as warnings. */ - public beat(): void { + public async beat(): Promise { if (this.alive()) { return } logger.trace("heartbeat") - fs.writeFile(this.heartbeatPath, "").catch((error) => { + try { + await fs.writeFile(this.heartbeatPath, "") + } catch (error: any) { logger.warn(error.message) - }) + } this.lastHeartbeat = Date.now() if (typeof this.heartbeatTimer !== "undefined") { clearTimeout(this.heartbeatTimer) } - this.heartbeatTimer = setTimeout(() => heartbeatTimer(this.isActive, this.beat), this.heartbeatInterval) + this.heartbeatTimer = setTimeout(async () => { + try { + await heartbeatTimer(this.isActive, this.beat) + } catch (error: any) { + logger.warn(error.message) + } + }, this.heartbeatInterval) } /** diff --git a/src/node/routes/index.ts b/src/node/routes/index.ts index 71ce557c5412..13d53df86fb3 100644 --- a/src/node/routes/index.ts +++ b/src/node/routes/index.ts @@ -56,6 +56,8 @@ export const register = async (app: App, args: DefaultedArgs): Promise { }) afterEach(() => { jest.resetAllMocks() + jest.useRealTimers() if (heart) { heart.dispose() } @@ -42,11 +43,7 @@ describe("Heart", () => { expect(fileContents).toBe(text) heart = new Heart(pathToFile, mockIsActive(true)) - heart.beat() - // HACK@jsjoeio - beat has some async logic but is not an async method - // Therefore, we have to create an artificial wait in order to make sure - // all async code has completed before asserting - await new Promise((r) => setTimeout(r, 100)) + await heart.beat() // Check that the heart wrote to the heartbeatFilePath and overwrote our text const fileContentsAfterBeat = await readFile(pathToFile, { encoding: "utf8" }) expect(fileContentsAfterBeat).not.toBe(text) @@ -56,15 +53,11 @@ describe("Heart", () => { }) it("should log a warning when given an invalid file path", async () => { heart = new Heart(`fakeDir/fake.txt`, mockIsActive(false)) - heart.beat() - // HACK@jsjoeio - beat has some async logic but is not an async method - // Therefore, we have to create an artificial wait in order to make sure - // all async code has completed before asserting - await new Promise((r) => setTimeout(r, 100)) + await heart.beat() expect(logger.warn).toHaveBeenCalled() }) - it("should be active after calling beat", () => { - heart.beat() + it("should be active after calling beat", async () => { + await heart.beat() const isAlive = heart.alive() expect(isAlive).toBe(true) From 79ce2b547294ea790707d6114d4d3ee272a2e8e0 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 28 Apr 2022 15:00:14 -0700 Subject: [PATCH 3/6] refactor: bind heart methods .beat and .alive This allows the functions to maintain access to the Heart instance (or `this`) even when they are passed to other functions. We do this because we pass both `isActive` and `beat` to `heartbeatTimer`. --- src/node/heart.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node/heart.ts b/src/node/heart.ts index c4ecc5ac3a36..c13ea5a0c2e0 100644 --- a/src/node/heart.ts +++ b/src/node/heart.ts @@ -9,7 +9,10 @@ export class Heart { private heartbeatInterval = 60000 public lastHeartbeat = 0 - public constructor(private readonly heartbeatPath: string, private readonly isActive: () => Promise) {} + public constructor(private readonly heartbeatPath: string, private readonly isActive: () => Promise) { + this.beat = this.beat.bind(this) + this.alive = this.alive.bind(this) + } public alive(): boolean { const now = Date.now() From 4d29cd559f10dfaea69045dcdcf9a359b387aaa1 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 28 Apr 2022 15:03:33 -0700 Subject: [PATCH 4/6] feat(heart): add test to ensure no warnings called --- test/unit/node/heart.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index 6dd9495ffbf1..7aa6f08dc2bf 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -68,7 +68,17 @@ describe("Heart", () => { const isAlive = heart.alive() expect(isAlive).toBe(false) }) - it.todo("should beat twice without warnings") + it("should beat twice without warnings", async () => { + // Use fake timers so we can speed up setTimeout + jest.useFakeTimers() + heart = new Heart(`${testDir}/hello.txt`, mockIsActive(true)) + await heart.beat() + // we need to speed up clocks, timeouts + // call heartbeat again (and it won't be alive I think) + // then assert no warnings were called + jest.runAllTimers() + expect(logger.warn).not.toHaveBeenCalled() + }) }) describe("heartbeatTimer", () => { From 9fca6f0d4c69e268685db633beeedef80c7567ad Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 4 May 2022 22:01:42 +0000 Subject: [PATCH 5/6] fixup!: revert setTimeout for heartbeatTimer --- src/node/heart.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/node/heart.ts b/src/node/heart.ts index c13ea5a0c2e0..f008308e409e 100644 --- a/src/node/heart.ts +++ b/src/node/heart.ts @@ -38,13 +38,7 @@ export class Heart { if (typeof this.heartbeatTimer !== "undefined") { clearTimeout(this.heartbeatTimer) } - this.heartbeatTimer = setTimeout(async () => { - try { - await heartbeatTimer(this.isActive, this.beat) - } catch (error: any) { - logger.warn(error.message) - } - }, this.heartbeatInterval) + this.heartbeatTimer = setTimeout(() => heartbeatTimer(this.isActive, this.beat), this.heartbeatInterval) } /** From d5dd4b4887352592d919298858114386cbf9efc9 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 4 May 2022 22:26:49 +0000 Subject: [PATCH 6/6] fixup!: return promise in beat --- src/node/heart.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node/heart.ts b/src/node/heart.ts index f008308e409e..19f9aa4ad804 100644 --- a/src/node/heart.ts +++ b/src/node/heart.ts @@ -29,16 +29,16 @@ export class Heart { } logger.trace("heartbeat") - try { - await fs.writeFile(this.heartbeatPath, "") - } catch (error: any) { - logger.warn(error.message) - } this.lastHeartbeat = Date.now() if (typeof this.heartbeatTimer !== "undefined") { clearTimeout(this.heartbeatTimer) } this.heartbeatTimer = setTimeout(() => heartbeatTimer(this.isActive, this.beat), this.heartbeatInterval) + try { + return await fs.writeFile(this.heartbeatPath, "") + } catch (error: any) { + logger.warn(error.message) + } } /**