Merge pull request #3858 from cdr/jsjoeio-add-proxy-test

feat: add more tests for proxy.ts

test/unit/helpers.test.ts

@@ -1,5 +1,5 @@
 import { promises as fs } from "fs"
-import { tmpdir, useEnv } from "../../test/utils/helpers"
+import { getAvailablePort, tmpdir, useEnv } from "../../test/utils/helpers"
 
 /**
  * This file is for testing test helpers (not core code).
@@ -39,3 +39,16 @@ describe("useEnv", () => {
     expect(process.env[envKey]).toEqual("test environment variable")
   })
 })
+
+describe("getAvailablePort", () => {
+  it("should return a valid port", async () => {
+    const port = await getAvailablePort()
+    expect(port).toBeGreaterThan(0)
+    expect(port).toBeLessThanOrEqual(65535)
+  })
+  it("should return different ports for different calls", async () => {
+    const portOne = await getAvailablePort()
+    const portTwo = await getAvailablePort()
+    expect(portOne).not.toEqual(portTwo)
+  })
+})

test/unit/node/proxy.test.ts

@@ -1,7 +1,12 @@
 import bodyParser from "body-parser"
 import * as express from "express"
+import * as nodeFetch from "node-fetch"
+import * as http from "http"
+import { HttpCode } from "../../../src/common/http"
+import { proxy } from "../../../src/node/proxy"
 import * as httpserver from "../../utils/httpserver"
 import * as integration from "../../utils/integration"
+import { getAvailablePort } from "../../utils/helpers"
 
 describe("proxy", () => {
   const nhooyrDevServer = new httpserver.HttpServer()
@@ -102,4 +107,109 @@ describe("proxy", () => {
     expect(resp.status).toBe(200)
     expect(await resp.json()).toBe("coder is the best")
   })
+
+  it("should handle bad requests", async () => {
+    e.use(bodyParser.json({ strict: false }))
+    e.post("/wsup", (req, res) => {
+      res.json(req.body)
+    })
+    codeServer = await integration.setup(["--auth=none"], "")
+    const resp = await codeServer.fetch(proxyPath, {
+      method: "post",
+      body: "coder is the best",
+      headers: {
+        "Content-Type": "application/json",
+      },
+    })
+    expect(resp.status).toBe(400)
+    expect(resp.statusText).toMatch("Bad Request")
+  })
+
+  it("should handle invalid routes", async () => {
+    e.post("/wsup", (req, res) => {
+      res.json(req.body)
+    })
+    codeServer = await integration.setup(["--auth=none"], "")
+    const resp = await codeServer.fetch(`${proxyPath}/hello`)
+    expect(resp.status).toBe(404)
+    expect(resp.statusText).toMatch("Not Found")
+  })
+
+  it("should handle errors", async () => {
+    e.use(bodyParser.json({ strict: false }))
+    e.post("/wsup", (req, res) => {
+      throw new Error("BROKEN")
+    })
+    codeServer = await integration.setup(["--auth=none"], "")
+    const resp = await codeServer.fetch(proxyPath, {
+      method: "post",
+      body: JSON.stringify("coder is the best"),
+      headers: {
+        "Content-Type": "application/json",
+      },
+    })
+    expect(resp.status).toBe(500)
+    expect(resp.statusText).toMatch("Internal Server Error")
+  })
+})
+
+// NOTE@jsjoeio
+// Both this test suite and the one above it are very similar
+// The main difference is this one uses http and node-fetch
+// and specifically tests the proxy in isolation vs. using
+// the httpserver abstraction we've built.
+//
+// Leaving this as a separate test suite for now because
+// we may consider refactoring the httpserver abstraction
+// in the future.
+//
+// If you're writing a test specifically for code in
+// src/node/proxy.ts, you should probably add it to
+// this test suite.
+describe("proxy (standalone)", () => {
+  let URL = ""
+  let PROXY_URL = ""
+  let testServer: http.Server
+  let proxyTarget: http.Server
+
+  beforeEach(async () => {
+    const PORT = await getAvailablePort()
+    const PROXY_PORT = await getAvailablePort()
+    URL = `http://localhost:${PORT}`
+    PROXY_URL = `http://localhost:${PROXY_PORT}`
+    // Define server and a proxy server
+    testServer = http.createServer((req, res) => {
+      proxy.web(req, res, {
+        target: PROXY_URL,
+      })
+    })
+
+    proxyTarget = http.createServer((req, res) => {
+      res.writeHead(200, { "Content-Type": "text/plain" })
+      res.end()
+    })
+
+    // Start both servers
+    await proxyTarget.listen(PROXY_PORT)
+    await testServer.listen(PORT)
+  })
+
+  afterEach(async () => {
+    await testServer.close()
+    await proxyTarget.close()
+  })
+
+  it("should return a 500 when proxy target errors ", async () => {
+    // Close the proxy target so that proxy errors
+    await proxyTarget.close()
+    const errorResp = await nodeFetch.default(`${URL}/error`)
+    expect(errorResp.status).toBe(HttpCode.ServerError)
+    expect(errorResp.statusText).toBe("Internal Server Error")
+  })
+
+  it("should proxy correctly", async () => {
+    const resp = await nodeFetch.default(`${URL}/route`)
+    expect(resp.status).toBe(200)
+    expect(resp.statusText).toBe("OK")
+  })
 })

test/utils/helpers.ts

@@ -1,6 +1,7 @@
 import { promises as fs } from "fs"
 import * as os from "os"
 import * as path from "path"
+import * as net from "net"
 
 /**
  * Return a mock of @coder/logger.
@@ -61,3 +62,23 @@ export function useEnv(key: string): [(nextValue: string | undefined) => string
 
   return [setValue, resetValue]
 }
+
+/**
+ * Helper function to get a random port.
+ *
+ * Source: https://github.com/sindresorhus/get-port/blob/main/index.js#L23-L33
+ */
+export const getAvailablePort = (options?: net.ListenOptions): Promise<number> =>
+  new Promise((resolve, reject) => {
+    const server = net.createServer()
+    server.unref()
+    server.on("error", reject)
+    server.listen(options, () => {
+      // NOTE@jsjoeio: not a huge fan of the type assertion
+      // but it works for now.
+      const { port } = server.address() as net.AddressInfo
+      server.close(() => {
+        resolve(port)
+      })
+    })
+  })