From f85772337a2972aeb4ab9b5b9a4174b4a228cd9a Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 9 Mar 2022 16:20:27 -0700 Subject: [PATCH 01/11] feat(testing): add test for app > listen --- src/node/app.ts | 2 +- test/unit/node/app.test.ts | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/node/app.ts b/src/node/app.ts index 7c868c2bc6df..615f770c2332 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -22,7 +22,7 @@ export interface App extends Disposable { server: http.Server } -const listen = (server: http.Server, { host, port, socket, "socket-mode": mode }: ListenOptions) => { +export const listen = (server: http.Server, { host, port, socket, "socket-mode": mode }: ListenOptions) => { return new Promise(async (resolve, reject) => { server.on("error", reject) diff --git a/test/unit/node/app.test.ts b/test/unit/node/app.test.ts index 29811d4f940f..91fe99386426 100644 --- a/test/unit/node/app.test.ts +++ b/test/unit/node/app.test.ts @@ -3,7 +3,7 @@ import { promises } from "fs" import * as http from "http" import * as https from "https" import * as path from "path" -import { createApp, ensureAddress, handleArgsSocketCatchError, handleServerError } from "../../../src/node/app" +import { createApp, ensureAddress, handleArgsSocketCatchError, handleServerError, listen } from "../../../src/node/app" import { OptionalString, setDefaults } from "../../../src/node/cli" import { generateCertificate } from "../../../src/node/util" import { clean, mockLogger, getAvailablePort, tmpdir } from "../../utils/helpers" @@ -219,7 +219,7 @@ describe("handleArgsSocketCatchError", () => { expect(logger.error).toHaveBeenCalledWith(errorMessage) }) - it("should not log an error if its a iNodeJS.ErrnoException", () => { + it("should not log an error if its a NodeJS.ErrnoException", () => { const error: NodeJS.ErrnoException = new Error() error.code = "ENOENT" @@ -250,3 +250,34 @@ describe("handleArgsSocketCatchError", () => { expect(logger.error).toHaveBeenCalledWith(error) }) }) + +describe("listen", () => { + let tmpDirPath: string + let mockServer: http.Server + + const testName = "listen" + + beforeEach(async () => { + await clean(testName) + mockLogger() + tmpDirPath = await tmpdir(testName) + mockServer = http.createServer() + }) + + afterEach(() => { + mockServer.close() + jest.clearAllMocks() + }) + + it.only("should log an error if a directory is passed in instead of a file", async () => { + const errorMessage = "EPERM: operation not permitted, unlink" + const port = await getAvailablePort() + const mockArgs = { port, host: "0.0.0.0", socket: tmpDirPath } + try { + await listen(mockServer, mockArgs) + } catch (error) {} + + expect(logger.error).toHaveBeenCalledTimes(1) + expect(logger.error).toHaveBeenCalledWith(expect.stringContaining(errorMessage)) + }) +}) From bdc1f56c61861ea40fc606cd749df344c73c400b Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 9 Mar 2022 16:23:31 -0700 Subject: [PATCH 02/11] Update test/unit/node/app.test.ts --- test/unit/node/app.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/node/app.test.ts b/test/unit/node/app.test.ts index 91fe99386426..42e4648f60db 100644 --- a/test/unit/node/app.test.ts +++ b/test/unit/node/app.test.ts @@ -269,7 +269,7 @@ describe("listen", () => { jest.clearAllMocks() }) - it.only("should log an error if a directory is passed in instead of a file", async () => { + it("should log an error if a directory is passed in instead of a file", async () => { const errorMessage = "EPERM: operation not permitted, unlink" const port = await getAvailablePort() const mockArgs = { port, host: "0.0.0.0", socket: tmpDirPath } From 35688b64d9aee1c2fc4b5bc5be23603b15959e6e Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 10 Mar 2022 14:40:07 -0700 Subject: [PATCH 03/11] refactor: modernize listen fn in app --- src/node/app.ts | 39 ++++++++++++-------------- test/unit/node/app.test.ts | 56 ++++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 49 deletions(-) diff --git a/src/node/app.ts b/src/node/app.ts index 615f770c2332..e24e6c71f36d 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -22,39 +22,33 @@ export interface App extends Disposable { server: http.Server } -export const listen = (server: http.Server, { host, port, socket, "socket-mode": mode }: ListenOptions) => { - return new Promise(async (resolve, reject) => { +export const listen = async (server: http.Server, { host, port, socket, "socket-mode": mode }: ListenOptions) => { + if (socket) { + try { + await fs.unlink(socket) + } catch (error: any) { + handleArgsSocketCatchError(error) + } + } + await new Promise(async (resolve, reject) => { server.on("error", reject) - const onListen = () => { // Promise resolved earlier so this is an unrelated error. server.off("error", reject) server.on("error", (err) => util.logError(logger, "http server error", err)) - - if (socket && mode) { - fs.chmod(socket, mode) - .then(resolve) - .catch((err) => { - util.logError(logger, "socket chmod", err) - reject(err) - }) - } else { - resolve() - } + resolve() } - if (socket) { - try { - await fs.unlink(socket) - } catch (error: any) { - handleArgsSocketCatchError(error) - } - server.listen(socket, onListen) } else { // [] is the correct format when using :: but Node errors with them. server.listen(port, host.replace(/^\[|\]$/g, ""), onListen) } + if (socket && mode) { + await fs.chmod(socket, mode).catch((err) => { + util.logError(logger, "socket chmod", err) + }) + } }) } @@ -137,7 +131,8 @@ export const handleServerError = (resolved: boolean, err: Error, reject: (err: E * test this logic more easily. */ export const handleArgsSocketCatchError = (error: any) => { + console.log("hello") if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") { - logger.error(error.message ? error.message : error) + throw Error(error.message ? error.message : error) } } diff --git a/test/unit/node/app.test.ts b/test/unit/node/app.test.ts index 42e4648f60db..49ebb4ed7d57 100644 --- a/test/unit/node/app.test.ts +++ b/test/unit/node/app.test.ts @@ -201,31 +201,33 @@ describe("handleArgsSocketCatchError", () => { }) it("should log an error if its not an NodeJS.ErrnoException", () => { - const error = new Error() + const message = "other message" + const error = new Error(message) - handleArgsSocketCatchError(error) - - expect(logger.error).toHaveBeenCalledTimes(1) - expect(logger.error).toHaveBeenCalledWith(error) + expect(() => { + handleArgsSocketCatchError(error) + }).toThrowError(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(logger.error).toHaveBeenCalledTimes(1) - expect(logger.error).toHaveBeenCalledWith(errorMessage) + expect(() => { + handleArgsSocketCatchError(error) + }).toThrowError(error) }) it("should not log an error if its a NodeJS.ErrnoException", () => { - const error: NodeJS.ErrnoException = new Error() - error.code = "ENOENT" + const code = "ENOENT" + const error: NodeJS.ErrnoException = new Error(code) + error.code = code handleArgsSocketCatchError(error) - expect(logger.error).toHaveBeenCalledTimes(0) + expect(() => { + handleArgsSocketCatchError(error) + }).not.toThrowError() }) it("should log an error if the code is not ENOENT (and the error has a message)", () => { @@ -234,20 +236,19 @@ describe("handleArgsSocketCatchError", () => { error.code = "EACCESS" error.message = errorMessage - handleArgsSocketCatchError(error) - - expect(logger.error).toHaveBeenCalledTimes(1) - expect(logger.error).toHaveBeenCalledWith(errorMessage) + expect(() => { + handleArgsSocketCatchError(error) + }).toThrowError(error) }) it("should log an error if the code is not ENOENT", () => { - const error: NodeJS.ErrnoException = new Error() - error.code = "EACCESS" + const code = "EACCESS" + const error: NodeJS.ErrnoException = new Error(code) + error.code = code - handleArgsSocketCatchError(error) - - expect(logger.error).toHaveBeenCalledTimes(1) - expect(logger.error).toHaveBeenCalledWith(error) + expect(() => { + handleArgsSocketCatchError(error) + }).toThrowError(error) }) }) @@ -269,15 +270,16 @@ describe("listen", () => { jest.clearAllMocks() }) - it("should log an error if a directory is passed in instead of a file", async () => { + it("should throw an error if a directory is passed in instead of a file", async () => { const errorMessage = "EPERM: operation not permitted, unlink" const port = await getAvailablePort() const mockArgs = { port, host: "0.0.0.0", socket: tmpDirPath } + try { await listen(mockServer, mockArgs) - } catch (error) {} - - expect(logger.error).toHaveBeenCalledTimes(1) - expect(logger.error).toHaveBeenCalledWith(expect.stringContaining(errorMessage)) + } catch (error) { + expect(error).toBeInstanceOf(Error) + expect((error as any).message).toMatch(errorMessage) + } }) }) From e521b7e08faaf501307ddcb030880fb60738fbad Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 10 Mar 2022 15:48:35 -0700 Subject: [PATCH 04/11] wip --- test/unit/node/constants.test.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/test/unit/node/constants.test.ts b/test/unit/node/constants.test.ts index 24501cbd2be8..83db6dbc3416 100644 --- a/test/unit/node/constants.test.ts +++ b/test/unit/node/constants.test.ts @@ -1,6 +1,7 @@ import { logger } from "@coder/logger" import { mockLogger } from "../../utils/helpers" import * as semver from "semver" +import path from "path" describe("constants", () => { let constants: typeof import("../../../src/node/constants") @@ -20,9 +21,14 @@ describe("constants", () => { } beforeAll(() => { + jest.clearAllMocks() mockLogger() - jest.mock("../../../package.json", () => mockPackageJson, { virtual: true }) - jest.mock("../../../vendor/modules/code-oss-dev/package.json", () => mockCodePackageJson, { virtual: true }) + jest.mock(path.resolve(__dirname, "../../../package.json"), () => mockPackageJson, { virtual: true }) + jest.mock( + path.resolve(__dirname, "../../../vendor/modules/code-oss-dev/package.json"), + () => mockCodePackageJson, + { virtual: true }, + ) constants = require("../../../src/node/constants") }) @@ -106,8 +112,13 @@ describe("constants", () => { } beforeAll(() => { - jest.mock("../../../package.json", () => mockPackageJson, { virtual: true }) - jest.mock("../../../vendor/modules/code-oss-dev/package.json", () => mockCodePackageJson, { virtual: true }) + jest.clearAllMocks() + jest.mock(path.resolve(__dirname, "../../../package.json"), () => mockPackageJson, { virtual: true }) + jest.mock( + path.resolve(__dirname, "../../../vendor/modules/code-oss-dev/package.json"), + () => mockCodePackageJson, + { virtual: true }, + ) constants = require("../../../src/node/constants") }) From c683e0f096f44bd1c9d818341caf1ce66e1f9fdf Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 10 Mar 2022 16:03:10 -0700 Subject: [PATCH 05/11] fix: update error message --- test/unit/node/app.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/node/app.test.ts b/test/unit/node/app.test.ts index 49ebb4ed7d57..62b2887f665c 100644 --- a/test/unit/node/app.test.ts +++ b/test/unit/node/app.test.ts @@ -271,7 +271,7 @@ describe("listen", () => { }) it("should throw an error if a directory is passed in instead of a file", async () => { - const errorMessage = "EPERM: operation not permitted, unlink" + const errorMessage = "EISDIR: illegal operation on a directory, unlink" const port = await getAvailablePort() const mockArgs = { port, host: "0.0.0.0", socket: tmpDirPath } From 4c07cc45cff91d2d70158e327f37ec29a310ecb6 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 12:31:02 -0700 Subject: [PATCH 06/11] fixup: remove console.log --- src/node/app.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node/app.ts b/src/node/app.ts index e24e6c71f36d..6fab12e602ab 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -131,7 +131,6 @@ export const handleServerError = (resolved: boolean, err: Error, reject: (err: E * test this logic more easily. */ export const handleArgsSocketCatchError = (error: any) => { - console.log("hello") if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") { throw Error(error.message ? error.message : error) } From f4153b0f5e93830ebda1feda2aa71c274fe0f989 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 12:32:09 -0700 Subject: [PATCH 07/11] fixup: use clearAllMocks once in beforeAll --- test/unit/node/constants.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/unit/node/constants.test.ts b/test/unit/node/constants.test.ts index 83db6dbc3416..d2aa68ab51ca 100644 --- a/test/unit/node/constants.test.ts +++ b/test/unit/node/constants.test.ts @@ -33,7 +33,6 @@ describe("constants", () => { }) afterAll(() => { - jest.clearAllMocks() jest.resetModules() }) @@ -123,7 +122,6 @@ describe("constants", () => { }) afterAll(() => { - jest.clearAllMocks() jest.resetModules() }) From a21e454516d54483398d9b2fd18615d432ff5d2b Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 12:33:52 -0700 Subject: [PATCH 08/11] fix: move chmod after socket listen --- src/node/app.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/node/app.ts b/src/node/app.ts index 6fab12e602ab..143696c0d3df 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -44,12 +44,15 @@ export const listen = async (server: http.Server, { host, port, socket, "socket- // [] is the correct format when using :: but Node errors with them. server.listen(port, host.replace(/^\[|\]$/g, ""), onListen) } - if (socket && mode) { - await fs.chmod(socket, mode).catch((err) => { - util.logError(logger, "socket chmod", err) - }) - } }) + + // NOTE@jsjoeio: we need to chmod after the server is finished + // listening. Otherwise, teh socket may not have been created yet. + if (socket && mode) { + await fs.chmod(socket, mode).catch((err) => { + util.logError(logger, "socket chmod", err) + }) + } } /** From 75f4d96c1381c858af046a7238c781fef5b99e6f Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 12:58:45 -0700 Subject: [PATCH 09/11] fixup: formatting --- src/node/app.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/app.ts b/src/node/app.ts index 143696c0d3df..b12a4027b518 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -46,7 +46,7 @@ export const listen = async (server: http.Server, { host, port, socket, "socket- } }) - // NOTE@jsjoeio: we need to chmod after the server is finished + // NOTE@jsjoeio: we need to chmod after the server is finished // listening. Otherwise, teh socket may not have been created yet. if (socket && mode) { await fs.chmod(socket, mode).catch((err) => { From 1329a769a63481c4fee187cab3145b4888224ff9 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 13:11:56 -0700 Subject: [PATCH 10/11] Update src/node/app.ts Co-authored-by: Jonathan Yu --- src/node/app.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/app.ts b/src/node/app.ts index b12a4027b518..ee6d8c6d20b8 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -47,7 +47,7 @@ export const listen = async (server: http.Server, { host, port, socket, "socket- }) // NOTE@jsjoeio: we need to chmod after the server is finished - // listening. Otherwise, teh socket may not have been created yet. + // listening. Otherwise, the socket may not have been created yet. if (socket && mode) { await fs.chmod(socket, mode).catch((err) => { util.logError(logger, "socket chmod", err) From 756ec1261d8d057a43eef0d0645e6383c3ecdbac Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Mar 2022 13:21:58 -0700 Subject: [PATCH 11/11] Update src/node/app.ts Co-authored-by: Asher --- src/node/app.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/node/app.ts b/src/node/app.ts index ee6d8c6d20b8..c1b1006dfecb 100644 --- a/src/node/app.ts +++ b/src/node/app.ts @@ -49,9 +49,7 @@ export const listen = async (server: http.Server, { host, port, socket, "socket- // NOTE@jsjoeio: we need to chmod after the server is finished // listening. Otherwise, the socket may not have been created yet. if (socket && mode) { - await fs.chmod(socket, mode).catch((err) => { - util.logError(logger, "socket chmod", err) - }) + await fs.chmod(socket, mode) } }