mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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 https://github.com/coder/internal/issues/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>
This commit is contained in:
+22
-6
@@ -1,5 +1,6 @@
|
||||
import { type ChildProcess, exec, spawn } from "node:child_process";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import type { Server } from "node:http";
|
||||
import net from "node:net";
|
||||
import path from "node:path";
|
||||
import { Duplex } from "node:stream";
|
||||
@@ -865,17 +866,32 @@ export class Awaiter {
|
||||
}
|
||||
}
|
||||
|
||||
export const createServer = async (
|
||||
port: number,
|
||||
): Promise<ReturnType<typeof express>> => {
|
||||
type MockServer = {
|
||||
app: ReturnType<typeof express>;
|
||||
/** Stops the server and drops keep-alive connections. */
|
||||
close: () => Promise<void>;
|
||||
};
|
||||
|
||||
export const createServer = async (port: number): Promise<MockServer> => {
|
||||
await waitForPort(port); // Wait until the port is available
|
||||
|
||||
const e = express();
|
||||
const app = express();
|
||||
// We need to specify the local IP address as the web server
|
||||
// tends to fail with IPv6 related error:
|
||||
// listen EADDRINUSE: address already in use :::50516
|
||||
await new Promise<void>((r) => e.listen(port, "0.0.0.0", r));
|
||||
return e;
|
||||
const server = await new Promise<Server>((resolve) => {
|
||||
const s = app.listen(port, "0.0.0.0", () => resolve(s));
|
||||
});
|
||||
|
||||
return {
|
||||
app,
|
||||
close: () =>
|
||||
new Promise<void>((resolve, reject) => {
|
||||
// Order matters: stop accepting, then drop keep-alives.
|
||||
server.close((err) => (err ? reject(err) : resolve()));
|
||||
server.closeAllConnections?.();
|
||||
}),
|
||||
};
|
||||
};
|
||||
|
||||
async function waitForPort(
|
||||
|
||||
@@ -14,8 +14,11 @@ import { beforeCoderTest, resetExternalAuthKey } from "../hooks";
|
||||
|
||||
test.describe
|
||||
.skip("externalAuth", () => {
|
||||
let closeWebServer: (() => Promise<void>) | undefined;
|
||||
|
||||
test.beforeAll(async ({ baseURL }) => {
|
||||
const srv = await createServer(gitAuth.webPort);
|
||||
const { app: srv, close } = await createServer(gitAuth.webPort);
|
||||
closeWebServer = close;
|
||||
|
||||
// The GitHub validate endpoint returns the currently authenticated user!
|
||||
srv.use(gitAuth.validatePath, (_req, res) => {
|
||||
@@ -34,6 +37,10 @@ test.describe
|
||||
});
|
||||
});
|
||||
|
||||
test.afterAll(async () => {
|
||||
await closeWebServer?.();
|
||||
});
|
||||
|
||||
test.beforeEach(async ({ context, page }) => {
|
||||
beforeCoderTest(page);
|
||||
await login(page);
|
||||
@@ -51,43 +58,49 @@ test.describe
|
||||
};
|
||||
|
||||
// Start a server to mock the GitHub API.
|
||||
const srv = await createServer(gitAuth.devicePort);
|
||||
srv.use(gitAuth.validatePath, (_req, res) => {
|
||||
res.write(JSON.stringify(ghUser));
|
||||
res.end();
|
||||
});
|
||||
srv.use(gitAuth.codePath, (_req, res) => {
|
||||
res.write(JSON.stringify(device));
|
||||
res.end();
|
||||
});
|
||||
srv.use(gitAuth.installationsPath, (_req, res) => {
|
||||
res.write(JSON.stringify(ghInstall));
|
||||
res.end();
|
||||
});
|
||||
const { app: srv, close: closeServer } = await createServer(
|
||||
gitAuth.devicePort,
|
||||
);
|
||||
try {
|
||||
srv.use(gitAuth.validatePath, (_req, res) => {
|
||||
res.write(JSON.stringify(ghUser));
|
||||
res.end();
|
||||
});
|
||||
srv.use(gitAuth.codePath, (_req, res) => {
|
||||
res.write(JSON.stringify(device));
|
||||
res.end();
|
||||
});
|
||||
srv.use(gitAuth.installationsPath, (_req, res) => {
|
||||
res.write(JSON.stringify(ghInstall));
|
||||
res.end();
|
||||
});
|
||||
|
||||
const token = {
|
||||
access_token: "",
|
||||
error: "authorization_pending",
|
||||
error_description: "",
|
||||
};
|
||||
// First we send a result from the API that the token hasn't been
|
||||
// authorized yet to ensure the UI reacts properly.
|
||||
const sentPending = new Awaiter();
|
||||
srv.use(gitAuth.tokenPath, (_req, res) => {
|
||||
res.write(JSON.stringify(token));
|
||||
res.end();
|
||||
sentPending.done();
|
||||
});
|
||||
const token = {
|
||||
access_token: "",
|
||||
error: "authorization_pending",
|
||||
error_description: "",
|
||||
};
|
||||
// First we send a result from the API that the token hasn't been
|
||||
// authorized yet to ensure the UI reacts properly.
|
||||
const sentPending = new Awaiter();
|
||||
srv.use(gitAuth.tokenPath, (_req, res) => {
|
||||
res.write(JSON.stringify(token));
|
||||
res.end();
|
||||
sentPending.done();
|
||||
});
|
||||
|
||||
await page.goto(`/external-auth/${gitAuth.deviceProvider}`, {
|
||||
waitUntil: "domcontentloaded",
|
||||
});
|
||||
await page.getByText(device.user_code).isVisible();
|
||||
await sentPending.wait();
|
||||
// Update the token to be valid and ensure the UI updates!
|
||||
token.error = "";
|
||||
token.access_token = "hello-world";
|
||||
await page.waitForSelector("text=1 organization authorized");
|
||||
await page.goto(`/external-auth/${gitAuth.deviceProvider}`, {
|
||||
waitUntil: "domcontentloaded",
|
||||
});
|
||||
await page.getByText(device.user_code).isVisible();
|
||||
await sentPending.wait();
|
||||
// Update the token to be valid and ensure the UI updates!
|
||||
token.error = "";
|
||||
token.access_token = "hello-world";
|
||||
await page.waitForSelector("text=1 organization authorized");
|
||||
} finally {
|
||||
await closeServer();
|
||||
}
|
||||
});
|
||||
|
||||
test("external auth web", async ({ page }) => {
|
||||
|
||||
Reference in New Issue
Block a user