Merge pull request #4160 from cdr/jsjoeio-add-tests-node

feat: add tests for src/node/app.ts

src/node/app.ts

@@ -6,6 +6,7 @@ import http from "http"
 import * as httpolyglot from "httpolyglot"
 import * as util from "../common/util"
 import { DefaultedArgs } from "./cli"
+import { isNodeJSErrnoException } from "./util"
 import { handleUpgrade } from "./wsRouter"
 
 /**
@@ -33,21 +34,14 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express,
       resolve2()
     }
     server.on("error", (err) => {
-      if (!resolved) {
+      handleServerError(resolved, err, reject)
-        reject(err)
-      } else {
-        // Promise resolved earlier so this is an unrelated error.
-        util.logError(logger, "http server error", err)
-      }
     })
 
     if (args.socket) {
       try {
         await fs.unlink(args.socket)
-      } catch (error) {
+      } catch (error: any) {
-        if (error.code !== "ENOENT") {
+        handleArgsSocketCatchError(error)
-          logger.error(error.message)
-        }
       }
       server.listen(args.socket, resolve)
     } else {
@@ -69,10 +63,46 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express,
 export const ensureAddress = (server: http.Server): string => {
   const addr = server.address()
   if (!addr) {
-    throw new Error("server has no address") // NOTE@jsjoeio test this line
+    throw new Error("server has no address")
   }
   if (typeof addr !== "string") {
     return `http://${addr.address}:${addr.port}`
   }
-  return addr // NOTE@jsjoeio test this line
+  return addr
+}
+
+/**
+ * Handles error events from the server.
+ *
+ * If the outlying Promise didn't resolve
+ * then we reject with the error.
+ *
+ * Otherwise, we log the error.
+ *
+ * We extracted into a function so that we could
+ * test this logic more easily.
+ */
+export const handleServerError = (resolved: boolean, err: Error, reject: (err: Error) => void) => {
+  // Promise didn't resolve earlier so this means it's an error
+  // that occurs before the server can successfully listen.
+  // Possibly triggered by listening on an invalid port or socket.
+  if (!resolved) {
+    reject(err)
+  } else {
+    // Promise resolved earlier so this is an unrelated error.
+    util.logError(logger, "http server error", err)
+  }
+}
+
+/**
+ * Handles the error that occurs in the catch block
+ * after we try fs.unlink(args.socket).
+ *
+ * We extracted into a function so that we could
+ * test this logic more easily.
+ */
+export const handleArgsSocketCatchError = (error: any) => {
+  if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") {
+    logger.error(error.message ? error.message : error)
+  }
 }

src/node/util.ts

@@ -524,3 +524,12 @@ export function escapeHtml(unsafe: string): string {
     .replace(/"/g, """)
     .replace(/'/g, "'")
 }
+
+/**
+ * A helper function which returns a boolean indicating whether
+ * the given error is a NodeJS.ErrnoException by checking if
+ * it has a .code property.
+ */
+export function isNodeJSErrnoException(error: unknown): error is NodeJS.ErrnoException {
+  return error instanceof Error && (error as NodeJS.ErrnoException).code !== undefined
+}

test/unit/node/__snapshots__/app.test.ts.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`handleServerError should log an error if resolved is true 1`] = `"Cannot read property 'handle' of undefined"`;

test/unit/node/app.test.ts

@@ -1,6 +1,168 @@
+import { logger } from "@coder/logger"
+import { promises, rmdirSync } from "fs"
 import * as http from "http"
-import { ensureAddress } from "../../../src/node/app"
+import * as https from "https"
-import { getAvailablePort } from "../../utils/helpers"
+import * as path from "path"
+import { createApp, ensureAddress, handleArgsSocketCatchError, handleServerError } from "../../../src/node/app"
+import { OptionalString, setDefaults } from "../../../src/node/cli"
+import { generateCertificate } from "../../../src/node/util"
+import { getAvailablePort, tmpdir } from "../../utils/helpers"
+
+describe("createApp", () => {
+  let spy: jest.SpyInstance
+  let unlinkSpy: jest.SpyInstance
+  let port: number
+  let tmpDirPath: string
+  let tmpFilePath: string
+
+  beforeAll(async () => {
+    tmpDirPath = await tmpdir("unlink-socket")
+    tmpFilePath = path.join(tmpDirPath, "unlink-socket-file")
+  })
+
+  beforeEach(async () => {
+    spy = jest.spyOn(logger, "error")
+    // NOTE:@jsjoeio
+    // Be mindful when spying.
+    // You can't spy on fs functions if you do import * as fs
+    // You have to import individually, like we do here with promises
+    // then you can spy on those modules methods, like unlink.
+    // See: https://github.com/aelbore/esbuild-jest/issues/26#issuecomment-893763840
+    unlinkSpy = jest.spyOn(promises, "unlink")
+    port = await getAvailablePort()
+  })
+
+  afterEach(() => {
+    jest.clearAllMocks()
+  })
+
+  afterAll(() => {
+    jest.restoreAllMocks()
+    // Ensure directory was removed
+    rmdirSync(tmpDirPath, { recursive: true })
+  })
+
+  it("should return an Express app, a WebSockets Express app and an http server", async () => {
+    const defaultArgs = await setDefaults({
+      port,
+      _: [],
+    })
+    const [app, wsApp, server] = await createApp(defaultArgs)
+
+    // This doesn't check much, but it's a good sanity check
+    // to ensure we actually get back values from createApp
+    expect(app).not.toBeNull()
+    expect(wsApp).not.toBeNull()
+    expect(server).toBeInstanceOf(http.Server)
+
+    // Cleanup
+    server.close()
+  })
+
+  it("should handle error events on the server", async () => {
+    const defaultArgs = await setDefaults({
+      port,
+      _: [],
+    })
+
+    // This looks funky, but that's because createApp
+    // returns an array like [app, wsApp, server]
+    // We only need server which is at index 2
+    // we do it this way so ESLint is happy that we're
+    // have no declared variables not being used
+    const app = await createApp(defaultArgs)
+    const server = app[2]
+
+    const testError = new Error("Test error")
+    // We can easily test how the server handles errors
+    // By emitting an error event
+    // Ref: https://stackoverflow.com/a/33872506/3015595
+    server.emit("error", testError)
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(`http server error: ${testError.message} ${testError.stack}`)
+
+    // Cleanup
+    server.close()
+  })
+
+  it("should reject errors that happen before the server can listen", async () => {
+    // We listen on an invalid port
+    // causing the app to reject the Promise called at startup
+    const port = 2
+    const defaultArgs = await setDefaults({
+      port,
+      _: [],
+    })
+
+    async function masterBall() {
+      const app = await createApp(defaultArgs)
+      const server = app[2]
+
+      const testError = new Error("Test error")
+
+      server.emit("error", testError)
+
+      // Cleanup
+      server.close()
+    }
+
+    expect(() => masterBall()).rejects.toThrow(`listen EACCES: permission denied 127.0.0.1:${port}`)
+  })
+
+  it("should unlink a socket before listening on the socket", async () => {
+    await promises.writeFile(tmpFilePath, "")
+    const defaultArgs = await setDefaults({
+      _: [],
+      socket: tmpFilePath,
+    })
+
+    const app = await createApp(defaultArgs)
+    const server = app[2]
+
+    expect(unlinkSpy).toHaveBeenCalledTimes(1)
+    server.close()
+  })
+  it("should catch errors thrown when unlinking a socket", async () => {
+    const tmpDir2 = await tmpdir("unlink-socket-error")
+    const tmpFile = path.join(tmpDir2, "unlink-socket-file")
+    // await promises.writeFile(tmpFile, "")
+    const socketPath = tmpFile
+    const defaultArgs = await setDefaults({
+      _: [],
+      socket: socketPath,
+    })
+
+    const app = await createApp(defaultArgs)
+    const server = app[2]
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(`ENOENT: no such file or directory, unlink '${socketPath}'`)
+
+    server.close()
+    // Ensure directory was removed
+    rmdirSync(tmpDir2, { recursive: true })
+  })
+
+  it("should create an https server if args.cert exists", async () => {
+    const testCertificate = await generateCertificate("localhost")
+    const cert = new OptionalString(testCertificate.cert)
+    const defaultArgs = await setDefaults({
+      port,
+      cert,
+      _: [],
+      ["cert-key"]: testCertificate.certKey,
+    })
+    const app = await createApp(defaultArgs)
+    const server = app[2]
+
+    // This doesn't check much, but it's a good sanity check
+    // to ensure we actually get an https.Server
+    expect(server).toBeInstanceOf(https.Server)
+
+    // Cleanup
+    server.close()
+  })
+})
 
 describe("ensureAddress", () => {
   let mockServer: http.Server
@@ -28,3 +190,107 @@ describe("ensureAddress", () => {
     expect(address).toBe(`http://localhost:8080`)
   })
 })
+
+describe("handleServerError", () => {
+  let spy: jest.SpyInstance
+
+  beforeEach(() => {
+    spy = jest.spyOn(logger, "error")
+  })
+
+  afterEach(() => {
+    jest.clearAllMocks()
+  })
+
+  afterAll(() => {
+    jest.restoreAllMocks()
+  })
+
+  it("should call reject if resolved is false", async () => {
+    const resolved = false
+    const reject = jest.fn((err: Error) => undefined)
+    const error = new Error("handleServerError Error")
+
+    handleServerError(resolved, error, reject)
+
+    expect(reject).toHaveBeenCalledTimes(1)
+    expect(reject).toHaveBeenCalledWith(error)
+  })
+
+  it("should log an error if resolved is true", async () => {
+    const resolved = true
+    const reject = jest.fn((err: Error) => undefined)
+    const error = new Error("handleServerError Error")
+
+    handleServerError(resolved, error, reject)
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toThrowErrorMatchingSnapshot()
+  })
+})
+
+describe("handleArgsSocketCatchError", () => {
+  let spy: jest.SpyInstance
+
+  beforeEach(() => {
+    spy = jest.spyOn(logger, "error")
+  })
+
+  afterEach(() => {
+    jest.clearAllMocks()
+  })
+
+  afterAll(() => {
+    jest.restoreAllMocks()
+  })
+
+  it("should log an error if its not an NodeJS.ErrnoException", () => {
+    const error = new Error()
+
+    handleArgsSocketCatchError(error)
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(error)
+  })
+
+  it("should log an error if its not an NodeJS.ErrnoException (and the error has a message)", () => {
+    const errorMessage = "handleArgsSocketCatchError Error"
+    const error = new Error(errorMessage)
+
+    handleArgsSocketCatchError(error)
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(errorMessage)
+  })
+
+  it("should not log an error if its a iNodeJS.ErrnoException", () => {
+    const error: NodeJS.ErrnoException = new Error()
+    error.code = "ENOENT"
+
+    handleArgsSocketCatchError(error)
+
+    expect(spy).toHaveBeenCalledTimes(0)
+  })
+
+  it("should log an error if the code is not ENOENT (and the error has a message)", () => {
+    const errorMessage = "no access"
+    const error: NodeJS.ErrnoException = new Error()
+    error.code = "EACCESS"
+    error.message = errorMessage
+
+    handleArgsSocketCatchError(error)
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(errorMessage)
+  })
+
+  it("should log an error if the code is not ENOENT", () => {
+    const error: NodeJS.ErrnoException = new Error()
+    error.code = "EACCESS"
+
+    handleArgsSocketCatchError(error)
+
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(error)
+  })
+})