Slightly improve heart tests

- Get rid of the global isActive mock; in particular the way it shadows
  local ones seemed sketchy.
- No need for requireActual from my testing.
- Reword the comment for why we need setImmediate.
- Add the setImmediate to another test that seemed to only pass because
  of an await on the timer call which is not actually a promise but had
  the side effect of yielding.
- Always set fake/real timers in the before/after handlers and never in
  individual tests.

test/unit/node/heart.test.ts

@@ -1,10 +1,9 @@
 import { logger } from "@coder/logger"
 import { readFile, writeFile, stat, utimes } from "fs/promises"
+import { setImmediate } from "timers"
 import { Heart } from "../../../src/node/heart"
 import { clean, mockLogger, tmpdir } from "../../utils/helpers"
 
-const mockIsActive = (resolveTo: boolean) => jest.fn().mockResolvedValue(resolveTo)
-
 describe("Heart", () => {
   const testName = "heartTests"
   let testDir = ""
@@ -15,12 +14,15 @@ describe("Heart", () => {
     await clean(testName)
     testDir = await tmpdir(testName)
   })
-  beforeEach(() => {
-    heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive(true))
-  })
+
   afterAll(() => {
     jest.restoreAllMocks()
   })
+
+  beforeEach(() => {
+    heart = new Heart(`${testDir}/shutdown.txt`, jest.fn().mockResolvedValue(true))
+  })
+
   afterEach(() => {
     jest.resetAllMocks()
     jest.useRealTimers()
@@ -28,6 +30,7 @@ describe("Heart", () => {
       heart.dispose()
     }
   })
+
   it("should write to a file when given a valid file path", async () => {
     // Set up heartbeat file with contents
     const text = "test"
@@ -42,7 +45,7 @@ describe("Heart", () => {
 
     expect(fileContents).toBe(text)
 
-    heart = new Heart(pathToFile, mockIsActive(true))
+    heart = new Heart(pathToFile, jest.fn().mockResolvedValue(true))
     await heart.beat()
     // Check that the heart wrote to the heartbeatFilePath and overwrote our text
     const fileContentsAfterBeat = await readFile(pathToFile, { encoding: "utf8" })
@@ -51,31 +54,30 @@ describe("Heart", () => {
     const fileStatusAfterEdit = await stat(pathToFile)
     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))
+    heart = new Heart(`fakeDir/fake.txt`, jest.fn().mockResolvedValue(false))
     await heart.beat()
     expect(logger.warn).toHaveBeenCalled()
   })
+
   it("should be active after calling beat", async () => {
     await heart.beat()
 
     const isAlive = heart.alive()
     expect(isAlive).toBe(true)
   })
+
   it("should not be active after dispose is called", () => {
     heart.dispose()
 
     const isAlive = heart.alive()
     expect(isAlive).toBe(false)
   })
+
   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))
+    heart = new Heart(`${testDir}/hello.txt`, jest.fn().mockResolvedValue(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()
   })
@@ -84,43 +86,48 @@ describe("Heart", () => {
 describe("heartbeatTimer", () => {
   const testName = "heartbeatTimer"
   let testDir = ""
+
   beforeAll(async () => {
     await clean(testName)
     testDir = await tmpdir(testName)
     mockLogger()
   })
+
   afterAll(() => {
     jest.restoreAllMocks()
   })
+
   beforeEach(() => {
     jest.useFakeTimers()
   })
+
   afterEach(() => {
     jest.resetAllMocks()
     jest.clearAllTimers()
     jest.useRealTimers()
   })
+
   it("should call isActive when timeout expires", async () => {
-    const isActive = true
-    const mockIsActive = jest.fn().mockResolvedValue(isActive)
+    const mockIsActive = jest.fn().mockResolvedValue(true)
     const heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive)
     await heart.beat()
     jest.advanceTimersByTime(60 * 1000)
     expect(mockIsActive).toHaveBeenCalled()
   })
+
   it("should log a warning when isActive rejects", async () => {
-    const errorMsg = "oh no"
-    const error = new Error(errorMsg)
+    const error = new Error("oh no")
     const mockIsActive = jest.fn().mockRejectedValue(error)
     const heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive)
     await heart.beat()
     jest.advanceTimersByTime(60 * 1000)
-    // Yield a real macrotask so the callback's rejection handler can run first
-    // (fake timers stub the global setImmediate).
-    await new Promise((resolve) => jest.requireActual("timers").setImmediate(resolve))
-
     expect(mockIsActive).toHaveBeenCalled()
-    expect(logger.warn).toHaveBeenCalledWith(errorMsg)
+
+    // The timer callback waits on mockIsActive, so we need to yield to let the
+    // callback finish.
+    await new Promise((resolve) => setImmediate(resolve))
+
+    expect(logger.warn).toHaveBeenCalledWith(error.message)
   })
 })
 
@@ -128,38 +135,54 @@ describe("stateChange", () => {
   const testName = "stateChange"
   let testDir = ""
   let heart: Heart
+
   beforeAll(async () => {
     await clean(testName)
     testDir = await tmpdir(testName)
     mockLogger()
   })
+
   afterAll(() => {
     jest.restoreAllMocks()
   })
+
+  beforeEach(() => {
+    jest.useFakeTimers()
+  })
+
   afterEach(() => {
     jest.resetAllMocks()
+    jest.useRealTimers()
     if (heart) {
       heart.dispose()
     }
   })
+
   it("should change to alive after a beat", async () => {
-    heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive(true))
+    const mockIsActive = jest.fn().mockResolvedValue(true)
+    heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive)
     const mockOnChange = jest.fn()
     heart.onChange(mockOnChange)
+
     await heart.beat()
 
     expect(mockOnChange.mock.calls[0][0]).toBe("alive")
   })
+
   it("should change to expired when not active", async () => {
-    jest.useFakeTimers()
-    heart = new Heart(`${testDir}/shutdown.txt`, () => new Promise((resolve) => resolve(false)))
+    const mockIsActive = jest.fn().mockResolvedValue(false)
+    heart = new Heart(`${testDir}/shutdown.txt`, mockIsActive)
     const mockOnChange = jest.fn()
     heart.onChange(mockOnChange)
-    await heart.beat()
 
-    await jest.advanceTimersByTime(60 * 1000)
+    await heart.beat()
+    jest.advanceTimersByTime(60 * 1000)
+    expect(mockIsActive).toHaveBeenCalled()
+
+    // The timer callback waits on the isActive promise, so we need to yield to
+    // let the callback finish.
+    await new Promise((resolve) => setImmediate(resolve))
+
     expect(mockOnChange.mock.calls[1][0]).toBe("expired")
-    jest.clearAllTimers()
-    jest.useRealTimers()
   })
 })