Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b341ce6

Browse filesBrowse files
jakehwlljakehwll
authored
fix(site/e2e): close mock external-auth servers in teardown (#26575)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Stack: 1. #26575 `fix(site/e2e): close mock external-auth servers in teardown` ← this PR 2. #26793 `fix(site/e2e): accept 404 from external auth reset hook` 3. #26795 `fix(site/src): refresh provider state after device-flow exchange` 4. #26798 `fix(site/e2e): reset both providers in external auth hook` 5. #26648 `chore(site/e2e): re-enable externalAuth suite` The externalAuth e2e suite has been skipped since #17235 because `createServer` in `site/e2e/helpers.ts` started an express server but never gave callers a way to close it. On retries or repeated runs, the listener from the previous invocation was still bound to the hardcoded port and the next `beforeAll` failed with `EADDRINUSE`, eventually timing out in `waitForPort`. `createServer` now returns a `{ app, close }` pair. The web flow closes in `afterAll`; the device flow uses `try/finally`. `closeAllConnections()` is called before `close()` so teardown stays bounded if keep-alive connections linger. The suite remains `test.describe.skip` here; #26648 flips the skip off once the rest of the stack is in. Refs https://linear.app/codercom/issue/DEVEX-413 Refs coder/internal#356 <details> <summary>Decision log</summary> Discussed the full options list with @jakehwll before drafting. Picked option A (minimal teardown) because: - Two prior PRs (#15537, #16528) attacked symptoms (port probing, longer timeout) without addressing the leaked listener. - Kayla's diagnosis on coder/internal#356 pointed at exactly this case: nothing else in CI is grabbing the port, the listener from the previous run is still bound. - A is mechanical and orthogonal: it adds a real teardown without changing port allocation, fixture wiring, or what gets mocked. If the flake persists after A, we know to escalate to a worker-scoped fixture or dynamically allocated ports. Returning `close` rather than the raw `http.Server` encapsulates the `closeAllConnections` + `close` choreography so callers don't repeat it. `closeAllConnections` is optional-chained because it landed in Node 18.2; coder/coder runs newer, but the chain costs nothing. The device test uses `try/finally` rather than a shared `afterEach` to keep per-test state local. The web flow's `afterAll` mirrors its `beforeAll`. </details>
1 parent a73e677 commit b341ce6
Copy full SHA for b341ce6

2 files changed

+71-42Lines changed: 71 additions & 42 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎site/e2e/helpers.ts‎

Copy file name to clipboardExpand all lines: site/e2e/helpers.ts
+22-6Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { type ChildProcess, exec, spawn } from "node:child_process";
22
import { randomUUID } from "node:crypto";
3+
import type { Server } from "node:http";
34
import net from "node:net";
45
import path from "node:path";
56
import { Duplex } from "node:stream";
@@ -865,17 +866,32 @@ export class Awaiter {
865866
}
866867
}
867868

868-
export const createServer = async (
869-
port: number,
870-
): Promise<ReturnType<typeof express>> => {
869+
type MockServer = {
870+
app: ReturnType<typeof express>;
871+
/** Stops the server and drops keep-alive connections. */
872+
close: () => Promise<void>;
873+
};
874+
875+
export const createServer = async (port: number): Promise<MockServer> => {
871876
await waitForPort(port); // Wait until the port is available
872877

873-
const e = express();
878+
const app = express();
874879
// We need to specify the local IP address as the web server
875880
// tends to fail with IPv6 related error:
876881
// listen EADDRINUSE: address already in use :::50516
877-
await new Promise<void>((r) => e.listen(port, "0.0.0.0", r));
878-
return e;
882+
const server = await new Promise<Server>((resolve) => {
883+
const s = app.listen(port, "0.0.0.0", () => resolve(s));
884+
});
885+
886+
return {
887+
app,
888+
close: () =>
889+
new Promise<void>((resolve, reject) => {
890+
// Order matters: stop accepting, then drop keep-alives.
891+
server.close((err) => (err ? reject(err) : resolve()));
892+
server.closeAllConnections?.();
893+
}),
894+
};
879895
};
880896

881897
async function waitForPort(
Collapse file

‎site/e2e/tests/externalAuth.spec.ts‎

Copy file name to clipboardExpand all lines: site/e2e/tests/externalAuth.spec.ts
+49-36Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,11 @@ import { beforeCoderTest, resetExternalAuthKey } from "../hooks";
1414

1515
test.describe
1616
.skip("externalAuth", () => {
17+
let closeWebServer: (() => Promise<void>) | undefined;
18+
1719
test.beforeAll(async ({ baseURL }) => {
18-
const srv = await createServer(gitAuth.webPort);
20+
const { app: srv, close } = await createServer(gitAuth.webPort);
21+
closeWebServer = close;
1922

2023
// The GitHub validate endpoint returns the currently authenticated user!
2124
srv.use(gitAuth.validatePath, (_req, res) => {
@@ -34,6 +37,10 @@ test.describe
3437
});
3538
});
3639

40+
test.afterAll(async () => {
41+
await closeWebServer?.();
42+
});
43+
3744
test.beforeEach(async ({ context, page }) => {
3845
beforeCoderTest(page);
3946
await login(page);
@@ -51,43 +58,49 @@ test.describe
5158
};
5259

5360
// Start a server to mock the GitHub API.
54-
const srv = await createServer(gitAuth.devicePort);
55-
srv.use(gitAuth.validatePath, (_req, res) => {
56-
res.write(JSON.stringify(ghUser));
57-
res.end();
58-
});
59-
srv.use(gitAuth.codePath, (_req, res) => {
60-
res.write(JSON.stringify(device));
61-
res.end();
62-
});
63-
srv.use(gitAuth.installationsPath, (_req, res) => {
64-
res.write(JSON.stringify(ghInstall));
65-
res.end();
66-
});
61+
const { app: srv, close: closeServer } = await createServer(
62+
gitAuth.devicePort,
63+
);
64+
try {
65+
srv.use(gitAuth.validatePath, (_req, res) => {
66+
res.write(JSON.stringify(ghUser));
67+
res.end();
68+
});
69+
srv.use(gitAuth.codePath, (_req, res) => {
70+
res.write(JSON.stringify(device));
71+
res.end();
72+
});
73+
srv.use(gitAuth.installationsPath, (_req, res) => {
74+
res.write(JSON.stringify(ghInstall));
75+
res.end();
76+
});
6777

68-
const token = {
69-
access_token: "",
70-
error: "authorization_pending",
71-
error_description: "",
72-
};
73-
// First we send a result from the API that the token hasn't been
74-
// authorized yet to ensure the UI reacts properly.
75-
const sentPending = new Awaiter();
76-
srv.use(gitAuth.tokenPath, (_req, res) => {
77-
res.write(JSON.stringify(token));
78-
res.end();
79-
sentPending.done();
80-
});
78+
const token = {
79+
access_token: "",
80+
error: "authorization_pending",
81+
error_description: "",
82+
};
83+
// First we send a result from the API that the token hasn't been
84+
// authorized yet to ensure the UI reacts properly.
85+
const sentPending = new Awaiter();
86+
srv.use(gitAuth.tokenPath, (_req, res) => {
87+
res.write(JSON.stringify(token));
88+
res.end();
89+
sentPending.done();
90+
});
8191

82-
await page.goto(`/external-auth/${gitAuth.deviceProvider}`, {
83-
waitUntil: "domcontentloaded",
84-
});
85-
await page.getByText(device.user_code).isVisible();
86-
await sentPending.wait();
87-
// Update the token to be valid and ensure the UI updates!
88-
token.error = "";
89-
token.access_token = "hello-world";
90-
await page.waitForSelector("text=1 organization authorized");
92+
await page.goto(`/external-auth/${gitAuth.deviceProvider}`, {
93+
waitUntil: "domcontentloaded",
94+
});
95+
await page.getByText(device.user_code).isVisible();
96+
await sentPending.wait();
97+
// Update the token to be valid and ensure the UI updates!
98+
token.error = "";
99+
token.access_token = "hello-world";
100+
await page.waitForSelector("text=1 organization authorized");
101+
} finally {
102+
await closeServer();
103+
}
91104
});
92105

93106
test("external auth web", async ({ page }) => {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.