From 946e4e88439a73dac4359cdcbf9132cb1caa3597 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 29 Oct 2021 16:03:57 -0700 Subject: [PATCH] feat(cli): add test for readSocketPath (#4284) * fix: update isNodeJSErrnoException * refactor(cli): export and purify readSocketPath * feat: add tests for readSocketPath * fix(ci): temporarily disable install deps from cache --- .github/workflows/ci.yaml | 20 ++++++++++------- src/node/cli.ts | 40 ++++++++++++++++++++++------------ src/node/util.ts | 2 +- test/unit/node/cli.test.ts | 44 +++++++++++++++++++++++++++++++++++--- 4 files changed, 80 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 386d8e74..60a44a5d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -31,14 +31,18 @@ jobs: - name: Install helm uses: azure/setup-helm@v1.1 - - name: Fetch dependencies from cache - id: cache-yarn - uses: actions/cache@v2 - with: - path: "**/node_modules" - key: yarn-build-${{ hashFiles('**/yarn.lock') }} - restore-keys: | - yarn-build- + # NOTE@jsjoeio + # disabling this until we can audit the build process + # and the usefulness of this step + # See: https://github.com/cdr/code-server/issues/4287 + # - name: Fetch dependencies from cache + # id: cache-yarn + # uses: actions/cache@v2 + # with: + # path: "**/node_modules" + # key: yarn-build-${{ hashFiles('**/yarn.lock') }} + # restore-keys: | + # yarn-build- - name: Install dependencies # if: steps.cache-yarn.outputs.cache-hit != 'true' diff --git a/src/node/cli.ts b/src/node/cli.ts index 5fb53e6a..3122fb00 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -3,7 +3,9 @@ import { promises as fs } from "fs" import yaml from "js-yaml" import * as os from "os" import * as path from "path" -import { canConnect, generateCertificate, generatePassword, humanPath, paths } from "./util" +import { canConnect, generateCertificate, generatePassword, humanPath, paths, isNodeJSErrnoException } from "./util" + +const DEFAULT_SOCKET_PATH = path.join(os.tmpdir(), "vscode-ipc") export enum Feature { // No current experimental features! @@ -657,6 +659,27 @@ function bindAddrFromAllSources(...argsConfig: Args[]): Addr { return addr } +/** + * Reads the socketPath based on path passed in. + * + * The one usually passed in is the DEFAULT_SOCKET_PATH. + * + * If it can't read the path, it throws an error and returns undefined. + */ +export async function readSocketPath(path: string): Promise { + try { + return await fs.readFile(path, "utf8") + } catch (error) { + // If it doesn't exist, we don't care. + // But if it fails for some reason, we should throw. + // We want to surface that to the user. + if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") { + throw error + } + } + return undefined +} + /** * Determine if it looks like the user is trying to open a file or folder in an * existing instance. The arguments here should be the arguments the user @@ -668,24 +691,13 @@ export const shouldOpenInExistingInstance = async (args: Args): Promise => { - try { - return await fs.readFile(path.join(os.tmpdir(), "vscode-ipc"), "utf8") - } catch (error: any) { - if (error.code !== "ENOENT") { - throw error - } - } - return undefined - } - // If these flags are set then assume the user is trying to open in an // existing instance since these flags have no effect otherwise. const openInFlagCount = ["reuse-window", "new-window"].reduce((prev, cur) => { return args[cur as keyof Args] ? prev + 1 : prev }, 0) if (openInFlagCount > 0) { - return readSocketPath() + return readSocketPath(DEFAULT_SOCKET_PATH) } // It's possible the user is trying to spawn another instance of code-server. @@ -693,7 +705,7 @@ export const shouldOpenInExistingInstance = async (args: Args): Promise 0) { - const socketPath = await readSocketPath() + const socketPath = await readSocketPath(DEFAULT_SOCKET_PATH) if (socketPath && (await canConnect(socketPath))) { return socketPath } diff --git a/src/node/util.ts b/src/node/util.ts index ebb380a9..37369d91 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -490,7 +490,7 @@ export function escapeHtml(unsafe: string): string { * it has a .code property. */ export function isNodeJSErrnoException(error: unknown): error is NodeJS.ErrnoException { - return error instanceof Error && (error as NodeJS.ErrnoException).code !== undefined + return error !== undefined && (error as NodeJS.ErrnoException).code !== undefined } // TODO: Replace with proper templating system. diff --git a/test/unit/node/cli.test.ts b/test/unit/node/cli.test.ts index 7ff7ac2e..44987eed 100644 --- a/test/unit/node/cli.test.ts +++ b/test/unit/node/cli.test.ts @@ -11,11 +11,11 @@ import { setDefaults, shouldOpenInExistingInstance, splitOnFirstEquals, + readSocketPath, } from "../../../src/node/cli" -import { tmpdir } from "../../../src/node/constants" import { shouldSpawnCliProcess } from "../../../src/node/main" import { generatePassword, paths } from "../../../src/node/util" -import { useEnv } from "../../utils/helpers" +import { useEnv, tmpdir } from "../../utils/helpers" type Mutable = { -readonly [P in keyof T]: T[P] @@ -390,10 +390,11 @@ describe("parser", () => { describe("cli", () => { let args: Mutable = { _: [] } - const testDir = path.join(tmpdir, "tests/cli") + let testDir: string const vscodeIpcPath = path.join(os.tmpdir(), "vscode-ipc") beforeAll(async () => { + testDir = await tmpdir("cli") await fs.rmdir(testDir, { recursive: true }) await fs.mkdir(testDir, { recursive: true }) }) @@ -667,3 +668,40 @@ password: ${password} cert: false`) }) }) + +describe("readSocketPath", () => { + const fileContents = "readSocketPath file contents" + let tmpDirPath: string + let tmpFilePath: string + + beforeEach(async () => { + tmpDirPath = await tmpdir("readSocketPath") + tmpFilePath = path.join(tmpDirPath, "readSocketPath.txt") + await fs.writeFile(tmpFilePath, fileContents) + }) + + afterEach(async () => { + await fs.rmdir(tmpDirPath, { recursive: true }) + }) + + it("should throw an error if it can't read the file", async () => { + // TODO@jsjoeio - implement + // Test it on a directory.... ESDIR + // TODO@jsjoeio - implement + expect(() => readSocketPath(tmpDirPath)).rejects.toThrow("EISDIR") + }) + it("should return undefined if it can't read the file", async () => { + // TODO@jsjoeio - implement + const socketPath = await readSocketPath(path.join(tmpDirPath, "not-a-file")) + expect(socketPath).toBeUndefined() + }) + it("should return the file contents", async () => { + const contents = await readSocketPath(tmpFilePath) + expect(contents).toBe(fileContents) + }) + it("should return the same file contents for two different calls", async () => { + const contents1 = await readSocketPath(tmpFilePath) + const contents2 = await readSocketPath(tmpFilePath) + expect(contents2).toBe(contents1) + }) +}) -- GitLab