Skip to content

Commit 6efc7b4

Browse files
committed
Fix issues surrounding persistent processes between tests.
1 parent dd8124f commit 6efc7b4

17 files changed

+1113
-1519
lines changed

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
"release:prep": "./ci/build/release-prep.sh",
2020
"test:e2e": "./ci/dev/test-e2e.sh",
2121
"test:standalone-release": "./ci/build/test-standalone-release.sh",
22-
"test:unit": "./ci/dev/test-unit.sh",
22+
"test:unit": "./ci/dev/test-unit.sh --forceExit --detectOpenHandles",
2323
"test:scripts": "./ci/dev/test-scripts.sh",
2424
"package": "./ci/build/build-packages.sh",
2525
"postinstall": "./ci/dev/postinstall.sh",

src/node/app.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,15 +68,18 @@ export const createApp = async (args: DefaultedArgs): Promise<[Express, Express,
6868
* Get the address of a server as a string (protocol *is* included) while
6969
* ensuring there is one (will throw if there isn't).
7070
*/
71-
export const ensureAddress = (server: http.Server): string => {
71+
export const ensureAddress = (server: http.Server, protocol: string): URL => {
7272
const addr = server.address()
73+
7374
if (!addr) {
74-
throw new Error("server has no address")
75+
throw new Error("Server has no address")
7576
}
77+
7678
if (typeof addr !== "string") {
77-
return `http://${addr.address}:${addr.port}`
79+
return new URL(`${protocol}://${addr.address}:${addr.port}`)
7880
}
79-
return addr
81+
82+
return new URL(addr)
8083
}
8184

8285
/**

src/node/link.ts

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,11 @@
11
import { logger } from "@coder/logger"
2-
import { spawn } from "child_process"
2+
import { ChildProcessWithoutNullStreams, spawn } from "child_process"
33
import path from "path"
44

5-
export function startLink(port: number): Promise<void> {
5+
export function startLink(port: number): ChildProcessWithoutNullStreams {
66
logger.debug(`running link targetting ${port}`)
77

8-
const agent = spawn(path.resolve(__dirname, "../../lib/linkup"), ["--devurl", `code:${port}:code-server`], {
8+
return spawn(path.resolve(__dirname, "../../lib/linkup"), ["--devurl", `code:${port}:code-server`], {
99
shell: false,
1010
})
11-
return new Promise((res, rej) => {
12-
agent.on("error", rej)
13-
agent.on("close", (code) => {
14-
if (code !== 0) {
15-
return rej({
16-
message: `Link exited with ${code}`,
17-
})
18-
}
19-
res()
20-
})
21-
})
2211
}

src/node/main.ts

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { field, logger } from "@coder/logger"
2+
import { ChildProcessWithoutNullStreams } from "child_process"
23
import http from "http"
34
import path from "path"
5+
import { Disposable } from "../common/emitter"
46
import { plural } from "../common/util"
57
import { createApp, ensureAddress } from "./app"
68
import { AuthType, DefaultedArgs, Feature } from "./cli"
@@ -105,7 +107,9 @@ export const openInExistingInstance = async (args: DefaultedArgs, socketPath: st
105107
vscode.end()
106108
}
107109

108-
export const runCodeServer = async (args: DefaultedArgs): Promise<http.Server> => {
110+
export const runCodeServer = async (
111+
args: DefaultedArgs,
112+
): Promise<{ dispose: Disposable["dispose"]; server: http.Server }> => {
109113
logger.info(`code-server ${version} ${commit}`)
110114

111115
logger.info(`Using user-data-dir ${humanPath(args["user-data-dir"])}`)
@@ -118,11 +122,11 @@ export const runCodeServer = async (args: DefaultedArgs): Promise<http.Server> =
118122
}
119123

120124
const [app, wsApp, server] = await createApp(args)
121-
const serverAddress = ensureAddress(server)
122-
await register(app, wsApp, server, args)
125+
const serverAddress = ensureAddress(server, args.cert ? "https" : "http")
126+
const disposeApp = await register(app, wsApp, server, args)
123127

124128
logger.info(`Using config file ${humanPath(args.config)}`)
125-
logger.info(`HTTP server listening on ${serverAddress} ${args.link ? "(randomized by --link)" : ""}`)
129+
logger.info(`HTTP server listening on ${serverAddress.toString()} ${args.link ? "(randomized by --link)" : ""}`)
126130
if (args.auth === AuthType.Password) {
127131
logger.info(" - Authentication is enabled")
128132
if (args.usingEnvPassword) {
@@ -148,19 +152,26 @@ export const runCodeServer = async (args: DefaultedArgs): Promise<http.Server> =
148152
}
149153

150154
if (args.link) {
151-
await coderCloudBind(serverAddress.replace(/^https?:\/\//, ""), args.link.value)
155+
await coderCloudBind(serverAddress.host, args.link.value)
152156
logger.info(" - Connected to cloud agent")
153157
}
154158

159+
let linkAgent: undefined | ChildProcessWithoutNullStreams
160+
155161
try {
156-
const port = parseInt(serverAddress.split(":").pop() as string, 10)
157-
startLink(port).catch((ex) => {
158-
logger.debug("Link daemon exited!", field("error", ex))
159-
})
162+
linkAgent = startLink(parseInt(serverAddress.port, 10))
160163
} catch (error) {
161164
logger.debug("Failed to start link daemon!", error as any)
162165
}
163166

167+
linkAgent?.on("error", (error) => {
168+
logger.debug("[Link daemon]", field("error", error))
169+
})
170+
171+
linkAgent?.on("close", (code) => {
172+
logger.debug("[Link daemon]", field("code", `Closed with code ${code}`))
173+
})
174+
164175
if (args.enable && args.enable.length > 0) {
165176
logger.info("Enabling the following experimental features:")
166177
args.enable.forEach((feature) => {
@@ -178,14 +189,25 @@ export const runCodeServer = async (args: DefaultedArgs): Promise<http.Server> =
178189

179190
if (!args.socket && args.open) {
180191
// The web socket doesn't seem to work if browsing with 0.0.0.0.
181-
const openAddress = serverAddress.replace("://0.0.0.0", "://localhost")
192+
if (serverAddress.hostname === "0.0.0.0") {
193+
serverAddress.hostname = "localhost"
194+
}
195+
182196
try {
183-
await open(openAddress)
184-
logger.info(`Opened ${openAddress}`)
197+
await open(serverAddress.toString())
198+
logger.info(`Opened ${serverAddress}`)
185199
} catch (error) {
186-
logger.error("Failed to open", field("address", openAddress), field("error", error))
200+
logger.error("Failed to open", field("address", serverAddress.toString()), field("error", error))
187201
}
188202
}
189203

190-
return server
204+
const dispose = () => {
205+
disposeApp()
206+
linkAgent?.kill()
207+
}
208+
209+
return {
210+
server,
211+
dispose,
212+
}
191213
}

src/node/routes/index.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import http from "http"
66
import * as path from "path"
77
import * as tls from "tls"
88
import * as pluginapi from "../../../typings/pluginapi"
9+
import { Disposable } from "../../common/emitter"
910
import { HttpCode, HttpError } from "../../common/http"
1011
import { plural } from "../../common/util"
1112
import { AuthType, DefaultedArgs } from "../cli"
@@ -33,7 +34,7 @@ export const register = async (
3334
wsApp: express.Express,
3435
server: http.Server,
3536
args: DefaultedArgs,
36-
): Promise<void> => {
37+
): Promise<Disposable["dispose"]> => {
3738
const heart = new Heart(path.join(paths.data, "heartbeat"), async () => {
3839
return new Promise((resolve, reject) => {
3940
server.getConnections((error, count) => {
@@ -161,14 +162,14 @@ export const register = async (
161162
}
162163
}
163164

164-
server.on("close", () => {
165-
vscode?.vscodeServer.close()
166-
})
167-
168165
app.use(() => {
169166
throw new HttpError("Not Found", HttpCode.NotFound)
170167
})
171168

172169
app.use(errorHandler)
173170
wsApp.use(wsErrorHandler)
171+
172+
return () => {
173+
vscode?.codeServerMain.dispose()
174+
}
174175
}

src/node/routes/vscode.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import * as express from "express"
2-
import { Server } from "http"
32
import path from "path"
43
import { AuthType, DefaultedArgs } from "../cli"
54
import { version as codeServerVersion, vsRootPath } from "../constants"
@@ -11,7 +10,7 @@ import { errorHandler } from "./errors"
1110
export interface VSServerResult {
1211
router: express.Router
1312
wsRouter: WebsocketRouter
14-
vscodeServer: Server
13+
codeServerMain: CodeServerLib.IServerProcessMain
1514
}
1615

1716
export const createVSServerRouter = async (args: DefaultedArgs): Promise<VSServerResult> => {
@@ -39,17 +38,19 @@ export const createVSServerRouter = async (args: DefaultedArgs): Promise<VSServe
3938
// Signal processes that we got launched as CLI
4039
process.env["VSCODE_CLI"] = "1"
4140

42-
const vscodeServerMain = await loadAMDModule<CodeServerLib.CreateVSServer>("vs/server/entry", "createVSServer")
41+
const createVSServer = await loadAMDModule<CodeServerLib.CreateVSServer>("vs/server/entry", "createVSServer")
4342

4443
const serverUrl = new URL(`${args.cert ? "https" : "http"}://${args.host}:${args.port}`)
45-
const vscodeServer = await vscodeServerMain({
44+
const codeServerMain = await createVSServer({
4645
codeServerVersion,
4746
serverUrl,
4847
args,
4948
authed: args.auth !== AuthType.None,
5049
disableUpdateCheck: !!args["disable-update-check"],
5150
})
5251

52+
const netServer = await codeServerMain.startup({ listenWhenReady: false })
53+
5354
const router = express.Router()
5455
const wsRouter = WsRouter()
5556

@@ -67,18 +68,18 @@ export const createVSServerRouter = async (args: DefaultedArgs): Promise<VSServe
6768
router.all("*", ensureAuthenticated, (req, res, next) => {
6869
req.on("error", (error) => errorHandler(error, req, res, next))
6970

70-
vscodeServer.emit("request", req, res)
71+
netServer.emit("request", req, res)
7172
})
7273

7374
wsRouter.ws("/", ensureAuthenticated, (req) => {
74-
vscodeServer.emit("upgrade", req, req.socket, req.head)
75+
netServer.emit("upgrade", req, req.socket, req.head)
7576

7677
req.socket.resume()
7778
})
7879

7980
return {
8081
router,
8182
wsRouter,
82-
vscodeServer,
83+
codeServerMain,
8384
}
8485
}

test/package.json

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,20 @@
33
"#": "We must put jest in a sub-directory otherwise VS Code somehow picks up the types and generates conflicts with mocha.",
44
"devDependencies": {
55
"@playwright/test": "^1.12.1",
6-
"@types/jest": "^26.0.20",
6+
"@types/jest": "^27.0.2",
77
"@types/jsdom": "^16.2.13",
88
"@types/node-fetch": "^2.5.8",
9-
"@types/supertest": "^2.0.10",
9+
"@types/supertest": "^2.0.11",
1010
"@types/wtfnode": "^0.7.0",
1111
"argon2": "^0.28.0",
12-
"jest": "^26.6.3",
12+
"jest": "^27.3.1",
1313
"jest-fetch-mock": "^3.0.3",
1414
"jsdom": "^16.4.0",
1515
"node-fetch": "^2.6.1",
1616
"playwright": "^1.12.1",
17-
"supertest": "^6.1.1",
18-
"ts-jest": "^26.4.4",
19-
"wtfnode": "^0.9.0"
17+
"supertest": "^6.1.6",
18+
"ts-jest": "^27.0.7",
19+
"wtfnode": "^0.9.1"
2020
},
2121
"resolutions": {
2222
"ansi-regex": "^5.0.1",

test/unit/browser/pages/login.test.ts

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -35,39 +35,40 @@ describe("login", () => {
3535
spy.mockImplementation(() => mockElement)
3636
})
3737
})
38-
describe("there is not an element with id 'base'", () => {
39-
let spy: jest.SpyInstance
38+
// TODO: Is this still used?
39+
// describe("there is not an element with id 'base'", () => {
40+
// let spy: jest.SpyInstance
4041

41-
beforeAll(() => {
42-
// This is important because we're manually requiring the file
43-
// If you don't call this before all tests
44-
// the module registry from other tests may cause side effects.
45-
jest.resetModuleRegistry()
46-
})
42+
// beforeAll(() => {
43+
// // This is important because we're manually requiring the file
44+
// // If you don't call this before all tests
45+
// // the module registry from other tests may cause side effects.
46+
// jest.resetModules()
47+
// })
4748

48-
beforeEach(() => {
49-
const dom = new JSDOM()
50-
global.document = dom.window.document
51-
spy = jest.spyOn(document, "getElementById")
49+
// beforeEach(() => {
50+
// const dom = new JSDOM()
51+
// global.document = dom.window.document
52+
// spy = jest.spyOn(document, "getElementById")
5253

53-
const location: LocationLike = {
54-
pathname: "/healthz",
55-
origin: "http://localhost:8080",
56-
}
54+
// const location: LocationLike = {
55+
// pathname: "/healthz",
56+
// origin: "http://localhost:8080",
57+
// }
5758

58-
global.location = location as Location
59-
})
59+
// global.location = location as Location
60+
// })
6061

61-
afterEach(() => {
62-
spy.mockClear()
63-
jest.resetModules()
64-
// Reset the global.document
65-
global.document = undefined as any as Document
66-
global.location = undefined as any as Location
67-
})
62+
// afterEach(() => {
63+
// spy.mockClear()
64+
// jest.resetModules()
65+
// // Reset the global.document
66+
// global.document = undefined as any as Document
67+
// global.location = undefined as any as Location
68+
// })
6869

69-
afterAll(() => {
70-
jest.restoreAllMocks()
71-
})
72-
})
70+
// afterAll(() => {
71+
// jest.restoreAllMocks()
72+
// })
73+
// })
7374
})

test/unit/node/app.test.ts

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -156,18 +156,12 @@ describe("ensureAddress", () => {
156156
})
157157

158158
it("should throw and error if no address", () => {
159-
expect(() => ensureAddress(mockServer)).toThrow("server has no address")
160-
})
161-
it("should return the address if it exists and not a string", async () => {
162-
const port = await getAvailablePort()
163-
mockServer.listen(port)
164-
const address = ensureAddress(mockServer)
165-
expect(address).toBe(`http://:::${port}`)
159+
expect(() => ensureAddress(mockServer, "http")).toThrow("Server has no address")
166160
})
167161
it("should return the address if it exists", async () => {
168-
mockServer.address = () => "http://localhost:8080"
169-
const address = ensureAddress(mockServer)
170-
expect(address).toBe(`http://localhost:8080`)
162+
mockServer.address = () => "http://localhost:8080/"
163+
const address = ensureAddress(mockServer, "http")
164+
expect(address.toString()).toBe(`http://localhost:8080/`)
171165
})
172166
})
173167

test/unit/node/proxy.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import bodyParser from "body-parser"
22
import * as express from "express"
33
import * as http from "http"
4-
import * as nodeFetch from "node-fetch"
4+
import nodeFetch from "node-fetch"
55
import { HttpCode } from "../../../src/common/http"
66
import { proxy } from "../../../src/node/proxy"
77
import { getAvailablePort } from "../../utils/helpers"
@@ -202,13 +202,13 @@ describe("proxy (standalone)", () => {
202202
it("should return a 500 when proxy target errors ", async () => {
203203
// Close the proxy target so that proxy errors
204204
await proxyTarget.close()
205-
const errorResp = await nodeFetch.default(`${URL}/error`)
205+
const errorResp = await nodeFetch(`${URL}/error`)
206206
expect(errorResp.status).toBe(HttpCode.ServerError)
207207
expect(errorResp.statusText).toBe("Internal Server Error")
208208
})
209209

210210
it("should proxy correctly", async () => {
211-
const resp = await nodeFetch.default(`${URL}/route`)
211+
const resp = await nodeFetch(`${URL}/route`)
212212
expect(resp.status).toBe(200)
213213
expect(resp.statusText).toBe("OK")
214214
})

0 commit comments

Comments
 (0)