From 0e4672f6b967f8faf6c3112b577098d2ea70e202 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 5 May 2021 10:43:54 -0500 Subject: [PATCH 1/6] Move health route tests to routes directory --- test/unit/{ => routes}/health.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename test/unit/{ => routes}/health.test.ts (91%) diff --git a/test/unit/health.test.ts b/test/unit/routes/health.test.ts similarity index 91% rename from test/unit/health.test.ts rename to test/unit/routes/health.test.ts index 2c5fc0b7..81472ced 100644 --- a/test/unit/health.test.ts +++ b/test/unit/routes/health.test.ts @@ -1,5 +1,5 @@ -import * as httpserver from "../utils/httpserver" -import * as integration from "../utils/integration" +import * as httpserver from "../../utils/httpserver" +import * as integration from "../../utils/integration" describe("health", () => { let codeServer: httpserver.HttpServer | undefined From 52cf2fcf29400ecc1f1ab196920e440cff0aba40 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 5 May 2021 11:02:34 -0500 Subject: [PATCH 2/6] Move tmpdir test helper to test helpers file --- test/e2e/terminal.test.ts | 6 +++--- test/unit/constants.test.ts | 15 --------------- test/unit/helpers.test.ts | 14 ++++++++++++++ test/utils/constants.ts | 11 ----------- test/utils/helpers.ts | 16 ++++++++++++++++ 5 files changed, 33 insertions(+), 29 deletions(-) create mode 100644 test/unit/helpers.test.ts diff --git a/test/e2e/terminal.test.ts b/test/e2e/terminal.test.ts index f92419c6..15fd5394 100644 --- a/test/e2e/terminal.test.ts +++ b/test/e2e/terminal.test.ts @@ -1,10 +1,10 @@ -import { test, expect } from "@playwright/test" +import { expect, test } from "@playwright/test" import * as cp from "child_process" import * as fs from "fs" -// import { tmpdir } from "os" import * as path from "path" import util from "util" -import { STORAGE, tmpdir } from "../utils/constants" +import { STORAGE } from "../utils/constants" +import { tmpdir } from "../utils/helpers" import { CodeServer } from "./models/CodeServer" test.describe("Integrated Terminal", () => { diff --git a/test/unit/constants.test.ts b/test/unit/constants.test.ts index d7d3c66b..88654457 100644 --- a/test/unit/constants.test.ts +++ b/test/unit/constants.test.ts @@ -1,5 +1,3 @@ -import * as fs from "fs" -import { tmpdir } from "../../test/utils/constants" import { loggerModule } from "../utils/helpers" // jest.mock is hoisted above the imports so we must use `require` here. @@ -89,16 +87,3 @@ describe("constants", () => { }) }) }) - -describe("test constants", () => { - describe("tmpdir", () => { - it("should return a temp directory", async () => { - const testName = "temp-dir" - const pathToTempDir = await tmpdir(testName) - - expect(pathToTempDir).toContain(testName) - - await fs.promises.rmdir(pathToTempDir) - }) - }) -}) diff --git a/test/unit/helpers.test.ts b/test/unit/helpers.test.ts new file mode 100644 index 00000000..74485475 --- /dev/null +++ b/test/unit/helpers.test.ts @@ -0,0 +1,14 @@ +import { promises as fs } from "fs" +import { tmpdir } from "../../test/utils/helpers" + +/** + * This file is for testing test helpers (not core code). + */ +describe("test helpers", () => { + it("should return a temp directory", async () => { + const testName = "temp-dir" + const pathToTempDir = await tmpdir(testName) + expect(pathToTempDir).toContain(testName) + expect(fs.access(pathToTempDir)).resolves.toStrictEqual(undefined) + }) +}) diff --git a/test/utils/constants.ts b/test/utils/constants.ts index a6abd209..ac2250e1 100644 --- a/test/utils/constants.ts +++ b/test/utils/constants.ts @@ -1,14 +1,3 @@ -import * as fs from "fs" -import * as os from "os" -import * as path from "path" - export const CODE_SERVER_ADDRESS = process.env.CODE_SERVER_ADDRESS || "http://localhost:8080" export const PASSWORD = process.env.PASSWORD || "e45432jklfdsab" export const STORAGE = process.env.STORAGE || "" - -export async function tmpdir(testName: string): Promise { - const dir = path.join(os.tmpdir(), "code-server") - await fs.promises.mkdir(dir, { recursive: true }) - - return await fs.promises.mkdtemp(path.join(dir, `test-${testName}-`), { encoding: "utf8" }) -} diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts index b4580401..96a772b9 100644 --- a/test/utils/helpers.ts +++ b/test/utils/helpers.ts @@ -1,3 +1,7 @@ +import * as fs from "fs" +import * as os from "os" +import * as path from "path" + export const loggerModule = { field: jest.fn(), level: 2, @@ -9,3 +13,15 @@ export const loggerModule = { warn: jest.fn(), }, } + +/** + * Create a uniquely named temporary directory. + * + * These directories are placed under a single temporary code-server directory. + */ +export async function tmpdir(testName: string): Promise { + const dir = path.join(os.tmpdir(), "code-server") + await fs.promises.mkdir(dir, { recursive: true }) + + return await fs.promises.mkdtemp(path.join(dir, `test-${testName}-`), { encoding: "utf8" }) +} From 1789cd1bcb3e15c73b5a58c84912bbefeb036c2c Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 5 May 2021 11:03:04 -0500 Subject: [PATCH 3/6] Move temp test dirs under a `tests` sub-directory This is to match the other tests that create temp directories. It also lets you clean up test temp directories all at once separately from other non-test temporary directories. --- test/utils/helpers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts index 96a772b9..df84ec1f 100644 --- a/test/utils/helpers.ts +++ b/test/utils/helpers.ts @@ -20,8 +20,8 @@ export const loggerModule = { * These directories are placed under a single temporary code-server directory. */ export async function tmpdir(testName: string): Promise { - const dir = path.join(os.tmpdir(), "code-server") + const dir = path.join(os.tmpdir(), "code-server/tests") await fs.promises.mkdir(dir, { recursive: true }) - return await fs.promises.mkdtemp(path.join(dir, `test-${testName}-`), { encoding: "utf8" }) + return await fs.promises.mkdtemp(path.join(dir, `${testName}-`), { encoding: "utf8" }) } From 4925e97080e9d62acce333e0e42849fa743e5752 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 5 May 2021 11:14:51 -0500 Subject: [PATCH 4/6] Add static route tests --- test/unit/routes/static.test.ts | 136 ++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 test/unit/routes/static.test.ts diff --git a/test/unit/routes/static.test.ts b/test/unit/routes/static.test.ts new file mode 100644 index 00000000..13897626 --- /dev/null +++ b/test/unit/routes/static.test.ts @@ -0,0 +1,136 @@ +import { promises as fs } from "fs" +import * as path from "path" +import { tmpdir } from "../../utils/helpers" +import * as httpserver from "../../utils/httpserver" +import * as integration from "../../utils/integration" + +describe("/static", () => { + let _codeServer: httpserver.HttpServer | undefined + function codeServer(): httpserver.HttpServer { + if (!_codeServer) { + throw new Error("tried to use code-server before setting it up") + } + return _codeServer + } + + let testFile: string | undefined + let testFileContent: string | undefined + let nonExistentTestFile: string | undefined + + // The static endpoint expects a commit and then the full path of the file. + // The commit is just for cache busting so we can use anything we want. `-` + // and `development` are specially recognized in that they will cause the + // static endpoint to avoid sending cache headers. + const commit = "-" + + beforeAll(async () => { + const testDir = await tmpdir("static") + testFile = path.join(testDir, "test") + testFileContent = "static file contents" + nonExistentTestFile = path.join(testDir, "i-am-not-here") + await fs.writeFile(testFile, testFileContent) + }) + + afterEach(async () => { + if (_codeServer) { + await _codeServer.close() + _codeServer = undefined + } + }) + + function commonTests() { + it("should return a 404 when a commit and file are not provided", async () => { + const resp = await codeServer().fetch("/static") + expect(resp.status).toBe(404) + + const content = await resp.json() + expect(content).toStrictEqual({ error: "Not Found" }) + }) + + it("should return a 404 when a file is not provided", async () => { + const resp = await codeServer().fetch(`/static/${commit}`) + expect(resp.status).toBe(404) + + const content = await resp.json() + expect(content).toStrictEqual({ error: "Not Found" }) + }) + } + + describe("disabled authentication", () => { + beforeEach(async () => { + _codeServer = await integration.setup(["--auth=none"], "") + }) + + commonTests() + + it("should return a 404 for a nonexistent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}/${nonExistentTestFile}`) + expect(resp.status).toBe(404) + + const content = await resp.json() + expect(content.error).toMatch("ENOENT") + }) + + it("should return a 200 and file contents for an existent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}${testFile}`) + expect(resp.status).toBe(200) + + const content = await resp.text() + expect(content).toStrictEqual(testFileContent) + }) + }) + + describe("enabled authentication", () => { + // Store whatever might be in here so we can restore it afterward. + // TODO: We should probably pass this as an argument somehow instead of + // manipulating the environment. + const previousEnvPassword = process.env.PASSWORD + + beforeEach(async () => { + process.env.PASSWORD = "test" + _codeServer = await integration.setup(["--auth=password"], "") + }) + + afterEach(() => { + process.env.PASSWORD = previousEnvPassword + }) + + commonTests() + + describe("inside code-server root", () => { + it("should return a 404 for a nonexistent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}/${__filename}-does-not-exist`) + expect(resp.status).toBe(404) + + const content = await resp.json() + expect(content.error).toMatch("ENOENT") + }) + + it("should return a 200 and file contents for an existent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}${__filename}`) + expect(resp.status).toBe(200) + + const content = await resp.text() + expect(content).toStrictEqual(await fs.readFile(__filename, "utf8")) + }) + }) + + describe("outside code-server root", () => { + it("should return a 401 for a nonexistent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}/${nonExistentTestFile}`) + expect(resp.status).toBe(401) + + const content = await resp.json() + expect(content).toStrictEqual({ error: "Unauthorized" }) + }) + + it("should return a 401 for an existent file", async () => { + const resp = await codeServer().fetch(`/static/${commit}${testFile}`) + expect(resp.status).toBe(401) + + const content = await resp.json() + expect(content).toStrictEqual({ error: "Unauthorized" }) + }) + }) + }) +}) From ad4a70c684487502a253842933e68bb35f22f08e Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 5 May 2021 17:58:04 -0500 Subject: [PATCH 5/6] Use warn log level for integration tests Just to limit all the noise from code-server's startup output. --- test/utils/integration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/integration.ts b/test/utils/integration.ts index 1ad7b44d..5c4f0cc6 100644 --- a/test/utils/integration.ts +++ b/test/utils/integration.ts @@ -3,7 +3,7 @@ import { runCodeServer } from "../../src/node/main" import * as httpserver from "./httpserver" export async function setup(argv: string[], configFile?: string): Promise { - argv = ["--bind-addr=localhost:0", ...argv] + argv = ["--bind-addr=localhost:0", "--log=warn", ...argv] const cliArgs = parse(argv) const configArgs = parseConfigFile(configFile || "", "test/integration.ts") From e8443e2602e4e0ed83e34565d15bff1754b1daf0 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 6 May 2021 11:25:29 -0500 Subject: [PATCH 6/6] Fix helpers not working in e2e tests It errors that jest is not defined so put it behind a function instead of immediately creating the mock (this is probably a better pattern anyway). The constant tests had to be reworked a little. Since the logger mock is hoisted it runs before createLoggerMock is imported. I moved it into a beforeAll which means the require call also needed to be moved there (since we need to mock the logger before requiring the constants or it'll pull the non-mocked logger). This means getPackageJson needs to be a let and assigned afterward. To avoid having to define a type for getPackageJson I just added a let var set to the type of the imported constants file and modified the other areas to use the same paradigm. I also replaced some hardcoded strings with the mocked package.json object. --- test/unit/constants.test.ts | 51 ++++++++++++++----------------------- test/unit/register.test.ts | 4 ++- test/unit/util.test.ts | 4 ++- test/utils/helpers.ts | 25 ++++++++++-------- 4 files changed, 40 insertions(+), 44 deletions(-) diff --git a/test/unit/constants.test.ts b/test/unit/constants.test.ts index 88654457..fe9c26d2 100644 --- a/test/unit/constants.test.ts +++ b/test/unit/constants.test.ts @@ -1,29 +1,22 @@ -import { loggerModule } from "../utils/helpers" - -// jest.mock is hoisted above the imports so we must use `require` here. -jest.mock("@coder/logger", () => require("../utils/helpers").loggerModule) +import { createLoggerMock } from "../utils/helpers" describe("constants", () => { - beforeAll(() => { - jest.clearAllMocks() - jest.resetModules() - }) + let constants: typeof import("../../src/node/constants") + describe("with package.json defined", () => { - const { getPackageJson } = require("../../src/node/constants") - let mockPackageJson = { + const loggerModule = createLoggerMock() + const mockPackageJson = { name: "mock-code-server", description: "Run VS Code on a remote server.", repository: "https://github.com/cdr/code-server", version: "1.0.0", commit: "f6b2be2838f4afb217c2fd8f03eafedd8d55ef9b", } - let version = "" - let commit = "" - beforeEach(() => { + beforeAll(() => { + jest.mock("@coder/logger", () => loggerModule) jest.mock("../../package.json", () => mockPackageJson, { virtual: true }) - commit = require("../../src/node/constants").commit - version = require("../../src/node/constants").version + constants = require("../../src/node/constants") }) afterAll(() => { @@ -32,18 +25,18 @@ describe("constants", () => { }) it("should provide the commit", () => { - expect(commit).toBe("f6b2be2838f4afb217c2fd8f03eafedd8d55ef9b") + expect(constants.commit).toBe(mockPackageJson.commit) }) it("should return the package.json version", () => { - expect(version).toBe(mockPackageJson.version) + expect(constants.version).toBe(mockPackageJson.version) }) describe("getPackageJson", () => { it("should log a warning if package.json not found", () => { const expectedErrorMessage = "Cannot find module './package.json' from 'src/node/constants.ts'" - getPackageJson("./package.json") + constants.getPackageJson("./package.json") expect(loggerModule.logger.warn).toHaveBeenCalled() expect(loggerModule.logger.warn).toHaveBeenCalledWith(expectedErrorMessage) @@ -52,38 +45,32 @@ describe("constants", () => { it("should find the package.json", () => { // the function calls require from src/node/constants // so to get the root package.json we need to use ../../ - const packageJson = getPackageJson("../../package.json") - expect(Object.keys(packageJson).length).toBeGreaterThan(0) - expect(packageJson.name).toBe("mock-code-server") - expect(packageJson.description).toBe("Run VS Code on a remote server.") - expect(packageJson.repository).toBe("https://github.com/cdr/code-server") + const packageJson = constants.getPackageJson("../../package.json") + expect(packageJson).toStrictEqual(mockPackageJson) }) }) }) describe("with incomplete package.json", () => { - let mockPackageJson = { + const mockPackageJson = { name: "mock-code-server", } - let version = "" - let commit = "" - beforeEach(() => { + beforeAll(() => { jest.mock("../../package.json", () => mockPackageJson, { virtual: true }) - version = require("../../src/node/constants").version - commit = require("../../src/node/constants").commit + constants = require("../../src/node/constants") }) - afterEach(() => { + afterAll(() => { jest.clearAllMocks() jest.resetModules() }) it("version should return 'development'", () => { - expect(version).toBe("development") + expect(constants.version).toBe("development") }) it("commit should return 'development'", () => { - expect(commit).toBe("development") + expect(constants.commit).toBe("development") }) }) }) diff --git a/test/unit/register.test.ts b/test/unit/register.test.ts index 4aa2006f..da69f21d 100644 --- a/test/unit/register.test.ts +++ b/test/unit/register.test.ts @@ -1,6 +1,6 @@ import { JSDOM } from "jsdom" import { registerServiceWorker } from "../../src/browser/register" -import { loggerModule } from "../utils/helpers" +import { createLoggerMock } from "../utils/helpers" import { LocationLike } from "./util.test" describe("register", () => { @@ -21,6 +21,7 @@ describe("register", () => { }) }) + const loggerModule = createLoggerMock() beforeEach(() => { jest.clearAllMocks() jest.mock("@coder/logger", () => loggerModule) @@ -75,6 +76,7 @@ describe("register", () => { }) describe("when navigator and serviceWorker are NOT defined", () => { + const loggerModule = createLoggerMock() beforeEach(() => { jest.clearAllMocks() jest.mock("@coder/logger", () => loggerModule) diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 66ea0ce2..e63fcde5 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -11,7 +11,7 @@ import { trimSlashes, normalize, } from "../../src/common/util" -import { loggerModule } from "../utils/helpers" +import { createLoggerMock } from "../utils/helpers" const dom = new JSDOM() global.document = dom.window.document @@ -229,6 +229,8 @@ describe("util", () => { jest.restoreAllMocks() }) + const loggerModule = createLoggerMock() + it("should log an error with the message and stack trace", () => { const message = "You don't have access to that folder." const error = new Error(message) diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts index df84ec1f..f31752b8 100644 --- a/test/utils/helpers.ts +++ b/test/utils/helpers.ts @@ -2,16 +2,21 @@ import * as fs from "fs" import * as os from "os" import * as path from "path" -export const loggerModule = { - field: jest.fn(), - level: 2, - logger: { - debug: jest.fn(), - error: jest.fn(), - info: jest.fn(), - trace: jest.fn(), - warn: jest.fn(), - }, +/** + * Return a mock of @coder/logger. + */ +export function createLoggerMock() { + return { + field: jest.fn(), + level: 2, + logger: { + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + trace: jest.fn(), + warn: jest.fn(), + }, + } } /**