From b14024f7ef9952f0a57b6e656f1d5d85058bacc6 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 26 Apr 2022 16:43:51 +0000 Subject: [PATCH 1/4] refactor: add timeout for race condition in heart test --- test/unit/node/heart.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index 7996fdab5468..e26aee94c318 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -38,12 +38,16 @@ describe("Heart", () => { 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)) // Check that the heart wrote to the heartbeatFilePath and overwrote our text const fileContentsAfterBeat = await readFile(pathToFile, { encoding: "utf8" }) expect(fileContentsAfterBeat).not.toBe(text) // Make sure the modified timestamp was updated. const fileStatusAfterEdit = await stat(pathToFile) - expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThan(fileStatusBeforeEdit.mtimeMs) + expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThanOrEqual(fileStatusBeforeEdit.mtimeMs) }) it("should log a warning when given an invalid file path", async () => { heart = new Heart(`fakeDir/fake.txt`, mockIsActive(false)) @@ -52,7 +56,6 @@ describe("Heart", () => { // 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)) - // expect(logger.trace).toHaveBeenCalled() expect(logger.warn).toHaveBeenCalled() }) it("should be active after calling beat", () => { From e203cc3169026de22ed3e88e7886542dc1be9301 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 26 Apr 2022 17:01:38 +0000 Subject: [PATCH 2/4] fixup!: set mtime to 0 and check for update --- test/unit/node/heart.test.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index e26aee94c318..3026d17c644e 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -1,5 +1,5 @@ import { logger } from "@coder/logger" -import { readFile, writeFile, stat } from "fs/promises" +import { readFile, writeFile, stat, open } from "fs/promises" import { Heart, heartbeatTimer } from "../../../src/node/heart" import { clean, mockLogger, tmpdir } from "../../utils/helpers" @@ -33,7 +33,12 @@ describe("Heart", () => { const pathToFile = `${testDir}/file.txt` await writeFile(pathToFile, text) const fileContents = await readFile(pathToFile, { encoding: "utf8" }) - const fileStatusBeforeEdit = await stat(pathToFile) + // Explicitly set the modified time to 0 so that we can check + // that the file was indeed modified after calling heart.beat(). + // This works around any potential race conditions. + const fileHandle = await open(pathToFile, "r+") + await fileHandle.utimes(0, 0) + expect(fileContents).toBe(text) heart = new Heart(pathToFile, mockIsActive(true)) @@ -47,7 +52,7 @@ describe("Heart", () => { expect(fileContentsAfterBeat).not.toBe(text) // Make sure the modified timestamp was updated. const fileStatusAfterEdit = await stat(pathToFile) - expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThanOrEqual(fileStatusBeforeEdit.mtimeMs) + expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThan(0) }) it("should log a warning when given an invalid file path", async () => { heart = new Heart(`fakeDir/fake.txt`, mockIsActive(false)) From b719d2ab98cf03589fc9ad6bf9e524ab61f1011f Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 26 Apr 2022 17:13:35 +0000 Subject: [PATCH 3/4] fixup!: use utimes directly instead of file open --- test/unit/node/heart.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index 3026d17c644e..5aebbf9ef578 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -1,5 +1,5 @@ import { logger } from "@coder/logger" -import { readFile, writeFile, stat, open } from "fs/promises" +import { readFile, writeFile, stat, open, utimes } from "fs/promises" import { Heart, heartbeatTimer } from "../../../src/node/heart" import { clean, mockLogger, tmpdir } from "../../utils/helpers" @@ -36,8 +36,8 @@ describe("Heart", () => { // Explicitly set the modified time to 0 so that we can check // that the file was indeed modified after calling heart.beat(). // This works around any potential race conditions. - const fileHandle = await open(pathToFile, "r+") - await fileHandle.utimes(0, 0) + // Docs: https://nodejs.org/api/fs.html#fspromisesutimespath-atime-mtime + await utimes(pathToFile, 0, 0) expect(fileContents).toBe(text) From 4ad1d028fe2f44a37d7c3e75b6660a72f9166fb1 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 26 Apr 2022 17:17:22 +0000 Subject: [PATCH 4/4] fixup!: remove import --- test/unit/node/heart.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/node/heart.test.ts b/test/unit/node/heart.test.ts index 5aebbf9ef578..5360c865e131 100644 --- a/test/unit/node/heart.test.ts +++ b/test/unit/node/heart.test.ts @@ -1,5 +1,5 @@ import { logger } from "@coder/logger" -import { readFile, writeFile, stat, open, utimes } from "fs/promises" +import { readFile, writeFile, stat, utimes } from "fs/promises" import { Heart, heartbeatTimer } from "../../../src/node/heart" import { clean, mockLogger, tmpdir } from "../../utils/helpers"