Skip to content

Commit 902a820

Browse files
committed
Mock logger everywhere
This suppresses all the error and debug output we generate which makes it hard to actually find which test has failed. It also gives us a standard way to test logging for the few places we do that.
1 parent 9a8477b commit 902a820

File tree

9 files changed

+71
-81
lines changed

9 files changed

+71
-81
lines changed

test/unit/common/emitter.test.ts

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,16 @@
1-
// Note: we need to import logger from the root
2-
// because this is the logger used in logError in ../src/common/util
31
import { logger } from "@coder/logger"
4-
52
import { Emitter } from "../../../src/common/emitter"
3+
import { mockLogger } from "../../utils/helpers"
64

75
describe("emitter", () => {
8-
let spy: jest.SpyInstance
9-
106
beforeEach(() => {
11-
spy = jest.spyOn(logger, "error")
7+
mockLogger()
128
})
139

1410
afterEach(() => {
1511
jest.clearAllMocks()
1612
})
1713

18-
afterAll(() => {
19-
jest.restoreAllMocks()
20-
})
21-
2214
it("should run the correct callbacks", async () => {
2315
const HELLO_WORLD = "HELLO_WORLD"
2416
const GOODBYE_WORLD = "GOODBYE_WORLD"
@@ -85,8 +77,8 @@ describe("emitter", () => {
8577
await emitter.emit({ event: HELLO_WORLD, callback: mockCallback })
8678

8779
// Check that error was called
88-
expect(spy).toHaveBeenCalled()
89-
expect(spy).toHaveBeenCalledTimes(1)
90-
expect(spy).toHaveBeenCalledWith(message)
80+
expect(logger.error).toHaveBeenCalled()
81+
expect(logger.error).toHaveBeenCalledTimes(1)
82+
expect(logger.error).toHaveBeenCalledWith(message)
9183
})
9284
})

test/unit/common/util.test.ts

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import { logger } from "@coder/logger"
12
import { JSDOM } from "jsdom"
23
import * as util from "../../../src/common/util"
3-
import { createLoggerMock } from "../../utils/helpers"
4+
import { mockLogger } from "../../utils/helpers"
45

56
const dom = new JSDOM()
67
global.document = dom.window.document
@@ -94,31 +95,29 @@ describe("util", () => {
9495
})
9596

9697
describe("logError", () => {
97-
afterEach(() => {
98-
jest.clearAllMocks()
98+
beforeAll(() => {
99+
mockLogger()
99100
})
100101

101-
afterAll(() => {
102-
jest.restoreAllMocks()
102+
afterEach(() => {
103+
jest.clearAllMocks()
103104
})
104105

105-
const loggerModule = createLoggerMock()
106-
107106
it("should log an error with the message and stack trace", () => {
108107
const message = "You don't have access to that folder."
109108
const error = new Error(message)
110109

111-
util.logError(loggerModule.logger, "ui", error)
110+
util.logError(logger, "ui", error)
112111

113-
expect(loggerModule.logger.error).toHaveBeenCalled()
114-
expect(loggerModule.logger.error).toHaveBeenCalledWith(`ui: ${error.message} ${error.stack}`)
112+
expect(logger.error).toHaveBeenCalled()
113+
expect(logger.error).toHaveBeenCalledWith(`ui: ${error.message} ${error.stack}`)
115114
})
116115

117116
it("should log an error, even if not an instance of error", () => {
118-
util.logError(loggerModule.logger, "api", "oh no")
117+
util.logError(logger, "api", "oh no")
119118

120-
expect(loggerModule.logger.error).toHaveBeenCalled()
121-
expect(loggerModule.logger.error).toHaveBeenCalledWith("api: oh no")
119+
expect(logger.error).toHaveBeenCalled()
120+
expect(logger.error).toHaveBeenCalledWith("api: oh no")
122121
})
123122
})
124123
})

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

Lines changed: 0 additions & 3 deletions
This file was deleted.

test/unit/node/app.test.ts

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,24 +6,24 @@ import * as path from "path"
66
import { createApp, ensureAddress, handleArgsSocketCatchError, handleServerError } from "../../../src/node/app"
77
import { OptionalString, setDefaults } from "../../../src/node/cli"
88
import { generateCertificate } from "../../../src/node/util"
9-
import { clean, getAvailablePort, tmpdir } from "../../utils/helpers"
9+
import { clean, mockLogger, getAvailablePort, tmpdir } from "../../utils/helpers"
1010

1111
describe("createApp", () => {
12-
let spy: jest.SpyInstance
1312
let unlinkSpy: jest.SpyInstance
1413
let port: number
1514
let tmpDirPath: string
1615
let tmpFilePath: string
1716

1817
beforeAll(async () => {
18+
mockLogger()
19+
1920
const testName = "unlink-socket"
2021
await clean(testName)
2122
tmpDirPath = await tmpdir(testName)
2223
tmpFilePath = path.join(tmpDirPath, "unlink-socket-file")
2324
})
2425

2526
beforeEach(async () => {
26-
spy = jest.spyOn(logger, "error")
2727
// NOTE:@jsjoeio
2828
// Be mindful when spying.
2929
// You can't spy on fs functions if you do import * as fs
@@ -66,8 +66,8 @@ describe("createApp", () => {
6666
// By emitting an error event
6767
// Ref: https://stackoverflow.com/a/33872506/3015595
6868
app.server.emit("error", testError)
69-
expect(spy).toHaveBeenCalledTimes(1)
70-
expect(spy).toHaveBeenCalledWith(`http server error: ${testError.message} ${testError.stack}`)
69+
expect(logger.error).toHaveBeenCalledTimes(1)
70+
expect(logger.error).toHaveBeenCalledWith(`http server error: ${testError.message} ${testError.stack}`)
7171

7272
// Cleanup
7373
app.dispose()
@@ -148,20 +148,14 @@ describe("ensureAddress", () => {
148148
})
149149

150150
describe("handleServerError", () => {
151-
let spy: jest.SpyInstance
152-
153-
beforeEach(() => {
154-
spy = jest.spyOn(logger, "error")
151+
beforeAll(() => {
152+
mockLogger()
155153
})
156154

157155
afterEach(() => {
158156
jest.clearAllMocks()
159157
})
160158

161-
afterAll(() => {
162-
jest.restoreAllMocks()
163-
})
164-
165159
it("should call reject if resolved is false", async () => {
166160
const resolved = false
167161
const reject = jest.fn((err: Error) => undefined)
@@ -180,33 +174,27 @@ describe("handleServerError", () => {
180174

181175
handleServerError(resolved, error, reject)
182176

183-
expect(spy).toHaveBeenCalledTimes(1)
184-
expect(spy).toThrowErrorMatchingSnapshot()
177+
expect(logger.error).toHaveBeenCalledTimes(1)
178+
expect(logger.error).toHaveBeenCalledWith(`http server error: ${error.message} ${error.stack}`)
185179
})
186180
})
187181

188182
describe("handleArgsSocketCatchError", () => {
189-
let spy: jest.SpyInstance
190-
191-
beforeEach(() => {
192-
spy = jest.spyOn(logger, "error")
183+
beforeAll(() => {
184+
mockLogger()
193185
})
194186

195187
afterEach(() => {
196188
jest.clearAllMocks()
197189
})
198190

199-
afterAll(() => {
200-
jest.restoreAllMocks()
201-
})
202-
203191
it("should log an error if its not an NodeJS.ErrnoException", () => {
204192
const error = new Error()
205193

206194
handleArgsSocketCatchError(error)
207195

208-
expect(spy).toHaveBeenCalledTimes(1)
209-
expect(spy).toHaveBeenCalledWith(error)
196+
expect(logger.error).toHaveBeenCalledTimes(1)
197+
expect(logger.error).toHaveBeenCalledWith(error)
210198
})
211199

212200
it("should log an error if its not an NodeJS.ErrnoException (and the error has a message)", () => {
@@ -215,8 +203,8 @@ describe("handleArgsSocketCatchError", () => {
215203

216204
handleArgsSocketCatchError(error)
217205

218-
expect(spy).toHaveBeenCalledTimes(1)
219-
expect(spy).toHaveBeenCalledWith(errorMessage)
206+
expect(logger.error).toHaveBeenCalledTimes(1)
207+
expect(logger.error).toHaveBeenCalledWith(errorMessage)
220208
})
221209

222210
it("should not log an error if its a iNodeJS.ErrnoException", () => {
@@ -225,7 +213,7 @@ describe("handleArgsSocketCatchError", () => {
225213

226214
handleArgsSocketCatchError(error)
227215

228-
expect(spy).toHaveBeenCalledTimes(0)
216+
expect(logger.error).toHaveBeenCalledTimes(0)
229217
})
230218

231219
it("should log an error if the code is not ENOENT (and the error has a message)", () => {
@@ -236,8 +224,8 @@ describe("handleArgsSocketCatchError", () => {
236224

237225
handleArgsSocketCatchError(error)
238226

239-
expect(spy).toHaveBeenCalledTimes(1)
240-
expect(spy).toHaveBeenCalledWith(errorMessage)
227+
expect(logger.error).toHaveBeenCalledTimes(1)
228+
expect(logger.error).toHaveBeenCalledWith(errorMessage)
241229
})
242230

243231
it("should log an error if the code is not ENOENT", () => {
@@ -246,7 +234,7 @@ describe("handleArgsSocketCatchError", () => {
246234

247235
handleArgsSocketCatchError(error)
248236

249-
expect(spy).toHaveBeenCalledTimes(1)
250-
expect(spy).toHaveBeenCalledWith(error)
237+
expect(logger.error).toHaveBeenCalledTimes(1)
238+
expect(logger.error).toHaveBeenCalledWith(error)
251239
})
252240
})

test/unit/node/constants.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
import { createLoggerMock } from "../../utils/helpers"
1+
import { logger } from "@coder/logger"
2+
import { mockLogger } from "../../utils/helpers"
23

34
describe("constants", () => {
45
let constants: typeof import("../../../src/node/constants")
56

67
describe("with package.json defined", () => {
7-
const loggerModule = createLoggerMock()
88
const mockPackageJson = {
99
name: "mock-code-server",
1010
description: "Run VS Code on a remote server.",
@@ -14,7 +14,7 @@ describe("constants", () => {
1414
}
1515

1616
beforeAll(() => {
17-
jest.mock("@coder/logger", () => loggerModule)
17+
mockLogger()
1818
jest.mock("../../../package.json", () => mockPackageJson, { virtual: true })
1919
constants = require("../../../src/node/constants")
2020
})
@@ -38,8 +38,8 @@ describe("constants", () => {
3838

3939
constants.getPackageJson("./package.json")
4040

41-
expect(loggerModule.logger.warn).toHaveBeenCalled()
42-
expect(loggerModule.logger.warn).toHaveBeenCalledWith(expectedErrorMessage)
41+
expect(logger.warn).toHaveBeenCalled()
42+
expect(logger.warn).toHaveBeenCalledWith(expectedErrorMessage)
4343
})
4444

4545
it("should find the package.json", () => {

test/unit/node/proxy_agent.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
import { shouldEnableProxy } from "../../../src/node/proxy_agent"
2-
import { useEnv } from "../../utils/helpers"
2+
import { mockLogger, useEnv } from "../../utils/helpers"
33

44
describe("shouldEnableProxy", () => {
55
const [setHTTPProxy, resetHTTPProxy] = useEnv("HTTP_PROXY")
66
const [setHTTPSProxy, resetHTTPSProxy] = useEnv("HTTPS_PROXY")
77
const [setNoProxy, resetNoProxy] = useEnv("NO_PROXY")
88

9+
beforeAll(() => {
10+
mockLogger()
11+
})
12+
913
beforeEach(() => {
1014
jest.resetModules() // Most important - it clears the cache
1115
resetHTTPProxy()

test/unit/node/routes/login.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
import { RateLimiter } from "../../../../src/node/routes/login"
2+
import { mockLogger } from "../../../utils/helpers"
23
import * as httpserver from "../../../utils/httpserver"
34
import * as integration from "../../../utils/integration"
45

56
describe("login", () => {
7+
beforeAll(() => {
8+
mockLogger()
9+
})
10+
611
describe("RateLimiter", () => {
712
it("should allow one try ", () => {
813
const limiter = new RateLimiter()

test/unit/node/update.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import * as http from "http"
22
import * as path from "path"
3-
import { clean, tmpdir } from "../../utils/helpers"
3+
import { clean, mockLogger, tmpdir } from "../../utils/helpers"
44
import { SettingsProvider, UpdateSettings } from "../../../src/node/settings"
55
import { LatestResponse, UpdateProvider } from "../../../src/node/update"
66

@@ -45,6 +45,8 @@ describe("update", () => {
4545
}
4646

4747
beforeAll(async () => {
48+
mockLogger()
49+
4850
const testName = "update"
4951
await clean(testName)
5052
const testDir = await tmpdir(testName)

test/utils/helpers.ts

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,26 @@
1+
import { logger } from "@coder/logger"
12
import { promises as fs } from "fs"
23
import * as net from "net"
34
import * as os from "os"
45
import * as path from "path"
56

67
/**
7-
* Return a mock of @coder/logger.
8+
* Spy on the logger and console and replace with mock implementations to
9+
* suppress the output.
810
*/
9-
export function createLoggerMock() {
10-
return {
11-
field: jest.fn(),
12-
level: 2,
13-
logger: {
14-
debug: jest.fn(),
15-
error: jest.fn(),
16-
info: jest.fn(),
17-
trace: jest.fn(),
18-
warn: jest.fn(),
19-
},
20-
}
11+
export function mockLogger() {
12+
jest.spyOn(logger, "debug").mockImplementation()
13+
jest.spyOn(logger, "error").mockImplementation()
14+
jest.spyOn(logger, "info").mockImplementation()
15+
jest.spyOn(logger, "trace").mockImplementation()
16+
jest.spyOn(logger, "warn").mockImplementation()
17+
18+
jest.spyOn(console, "log").mockImplementation()
19+
jest.spyOn(console, "debug").mockImplementation()
20+
jest.spyOn(console, "error").mockImplementation()
21+
jest.spyOn(console, "info").mockImplementation()
22+
jest.spyOn(console, "trace").mockImplementation()
23+
jest.spyOn(console, "warn").mockImplementation()
2124
}
2225

2326
/**

0 commit comments

Comments
 (0)