Second review of PR #45, non-blocking: two behaviours were right by hand and held by no test. - a listener on 3099 that closes at once, then one that accepts and stalls: the port rule is "anything but connection refused", not "an HTTP answer" - a server that stays alive and never listens: the wait ends on its deadline, with a message and exit 1 Also moves the port check above the "Fixtures" comment it had split from its mkdir lines, and counts four protections in CLAUDE.md, not three. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
466 lines
20 KiB
JavaScript
466 lines
20 KiB
JavaScript
const fs = require("node:fs");
|
|
const path = require("node:path");
|
|
const http = require("node:http");
|
|
const net = require("node:net");
|
|
const os = require("node:os");
|
|
const { spawn } = require("node:child_process");
|
|
|
|
// Pins on the CI workflow and on what the boot check assumes of the Dockerfile.
|
|
// The runtime stays 0-dependency and vitest ships no YAML parser, so these read
|
|
// the files with narrow regexes: each one looks at a single key, never at the
|
|
// YAML structure as a whole.
|
|
|
|
const ROOT = path.join(__dirname, "..");
|
|
const CI_PATH = path.join(ROOT, ".forgejo", "workflows", "ci.yml");
|
|
const ci = fs.readFileSync(CI_PATH, "utf-8");
|
|
const dockerfile = fs.readFileSync(path.join(ROOT, "Dockerfile"), "utf-8");
|
|
|
|
// Comment lines stripped, so an explanation that quotes a key cannot satisfy
|
|
// (or break) a pin.
|
|
function stripCommentLines(text) {
|
|
return text
|
|
.split("\n")
|
|
.filter((line) => !/^\s*#/.test(line))
|
|
.join("\n");
|
|
}
|
|
const ciCode = stripCommentLines(ci);
|
|
|
|
// Lines of a top-level-indented block (` pull_request:`), up to the next key at
|
|
// the same or a lower indentation.
|
|
function block(text, key) {
|
|
const lines = text.split("\n");
|
|
const start = lines.findIndex((l) => new RegExp(`^(\\s*)${key}:`).test(l));
|
|
if (start === -1) return null;
|
|
const indent = lines[start].match(/^(\s*)/)[1].length;
|
|
const out = [lines[start]];
|
|
for (const line of lines.slice(start + 1)) {
|
|
if (line.trim() === "") continue;
|
|
if (line.match(/^(\s*)/)[1].length <= indent) break;
|
|
out.push(line);
|
|
}
|
|
return out.join("\n");
|
|
}
|
|
|
|
describe("CI workflow", () => {
|
|
test("runs in the Dockerfile's own base image", () => {
|
|
const from = dockerfile.match(/^\s*FROM\s+(\S+)/m);
|
|
const container = ciCode.match(/^\s*container:\s*["']?([^"'\s]+)["']?\s*$/m);
|
|
|
|
expect(from).not.toBeNull();
|
|
expect(container).not.toBeNull();
|
|
expect(container[1]).toBe(from[1]);
|
|
});
|
|
|
|
test("runs the unit tests, the socket pass and the boot check", () => {
|
|
expect(ciCode).toMatch(/^\s*run:\s*npm ci\s*$/m);
|
|
expect(ciCode).toMatch(/^\s*run:\s*npm test\s*$/m);
|
|
expect(ciCode).toMatch(/^\s*run:\s*bash test-curl\.sh\s*$/m);
|
|
expect(ciCode).toMatch(/^\s*run:\s*sh scripts\/boot-from-copy\.sh\s*$/m);
|
|
});
|
|
|
|
test("builds every pull request, whatever branch it targets", () => {
|
|
// A `branches: [main]` here would leave the intermediate links of a stack
|
|
// without any status.
|
|
const pr = block(ciCode, "pull_request");
|
|
|
|
expect(pr).not.toBeNull();
|
|
expect(pr).not.toMatch(/branches(-ignore)?:/);
|
|
});
|
|
|
|
test("builds pushes to main", () => {
|
|
const push = block(ciCode, "push");
|
|
|
|
expect(push).not.toBeNull();
|
|
expect(push).toMatch(/branches:\s*\[\s*main\s*\]/);
|
|
});
|
|
|
|
test("avoids the two forms the runner rejects before the first command", () => {
|
|
// `defaults:` anywhere, and `working-directory` on a `uses:` step.
|
|
expect(ciCode).not.toMatch(/^\s*defaults:/m);
|
|
const steps = ciCode.split(/^\s*- (?=name:|uses:|run:)/m);
|
|
for (const step of steps) {
|
|
if (/^\s*uses:|\n\s*uses:/.test(step)) expect(step).not.toMatch(/working-directory:/);
|
|
}
|
|
});
|
|
|
|
// `continue-on-error: true` turns a red step green without a trace in the
|
|
// commit status. Any literal spelling of the key counts: a block key or a
|
|
// list item, but also a flow mapping and a quoted key, which a pin anchored
|
|
// at the start of the line let through (#35), and a YAML anchor reused as a
|
|
// key, which only a match on the bare word catches. Unanchored and bare on
|
|
// purpose: the word in a `run:` script, a step name or a trailing comment
|
|
// fails the pin too, loudly. Only full comment lines are exempt, being
|
|
// stripped from `ciCode`. An escaped key (`"continue\u002don-error": true`)
|
|
// still passes: the pin reads text, not YAML.
|
|
const CONTINUE_ON_ERROR = /continue-on-error/;
|
|
|
|
test("never lets a failing step pass", () => {
|
|
expect(ciCode).not.toMatch(CONTINUE_ON_ERROR);
|
|
});
|
|
|
|
// The detector itself, through the same comment stripping as `ciCode`.
|
|
// Anchored at the start of the line again, it would miss the flow mapping,
|
|
// the quoted key and the anchor; with a colon required right after the word,
|
|
// the quoted key and the anchor; with a closing quote allowed before that
|
|
// colon, the anchor alone.
|
|
test.each([
|
|
["a block key", " - name: x\n run: npm test\n continue-on-error: true"],
|
|
["a list item", " - continue-on-error: true\n run: npm test"],
|
|
["a flow mapping", " - { name: x, run: false, continue-on-error: true }"],
|
|
["a quoted key", ' - name: x\n "continue-on-error": true'],
|
|
["an anchor reused as a key", " - name: x\n env: { STEP_KEY: &coe continue-on-error }\n *coe : true"],
|
|
])("the continue-on-error pin catches %s", (_, step) => {
|
|
expect(stripCommentLines(step)).toMatch(CONTINUE_ON_ERROR);
|
|
});
|
|
|
|
test("the continue-on-error pin ignores a full comment line quoting the key", () => {
|
|
expect(stripCommentLines(" # never add continue-on-error: true here")).not.toMatch(CONTINUE_ON_ERROR);
|
|
});
|
|
});
|
|
|
|
describe("what the boot check assumes of the Dockerfile", () => {
|
|
test("the image still drops to the node user, as the script does", () => {
|
|
expect(dockerfile).toMatch(/^\s*USER\s+node\s*$/m);
|
|
});
|
|
|
|
test("CMD is in exec form, the only one the script parses", () => {
|
|
const cmd = dockerfile.match(/^\s*CMD\s+(.+)$/m);
|
|
|
|
expect(cmd).not.toBeNull();
|
|
const argv = JSON.parse(cmd[1]);
|
|
expect(Array.isArray(argv)).toBe(true);
|
|
expect(argv.length).toBeGreaterThan(0);
|
|
});
|
|
});
|
|
|
|
// Spawns a command from the repository root with `env` over the test's own
|
|
// environment; a key set to undefined is dropped (spawn ignores undefined
|
|
// values). Async, so a listener in this process keeps answering meanwhile.
|
|
function run(command, args, env) {
|
|
return new Promise((resolve, reject) => {
|
|
const started = Date.now();
|
|
const child = spawn(command, args, { cwd: ROOT, env: { ...process.env, ...env } });
|
|
let stdout = "";
|
|
let stderr = "";
|
|
child.stdout.on("data", (d) => (stdout += d));
|
|
child.stderr.on("data", (d) => (stderr += d));
|
|
child.on("error", reject);
|
|
child.on("close", (code) => resolve({ code, stdout, stderr, ms: Date.now() - started }));
|
|
});
|
|
}
|
|
|
|
const runBootScript = (env) => run("sh", ["scripts/boot-from-copy.sh"], env);
|
|
|
|
// A port nothing listens on. The probe closes before the script starts: should
|
|
// something take the port in between, the script's own check refuses it and
|
|
// the test fails loudly.
|
|
async function freePort() {
|
|
const probe = http.createServer();
|
|
await new Promise((resolve) => probe.listen(0, "127.0.0.1", resolve));
|
|
const { port } = probe.address();
|
|
await new Promise((resolve) => probe.close(resolve));
|
|
return port;
|
|
}
|
|
|
|
describe("boot-from-copy.sh", () => {
|
|
// A listener already on the port: without the pre-check, the wait loop could
|
|
// reach this neighbour and assert on a server it never started.
|
|
test("refuses a port something else already holds", async () => {
|
|
const neighbour = http.createServer((req, res) => res.end("not the API"));
|
|
await new Promise((resolve) => neighbour.listen(0, "127.0.0.1", resolve));
|
|
try {
|
|
const { port } = neighbour.address();
|
|
const { code, stderr } = await runBootScript({ BOOT_PORT: String(port) });
|
|
|
|
expect(code).toBe(1);
|
|
expect(stderr).toMatch(new RegExp(`port ${port} is already taken`));
|
|
} finally {
|
|
await new Promise((resolve) => neighbour.close(resolve));
|
|
}
|
|
}, 15000);
|
|
|
|
// Every curl of the script targets the loopback, and a proxy from the
|
|
// environment would answer in the server's place. Before #35 only the port
|
|
// check had --noproxy: the wait loop and the assertions reached the proxy.
|
|
test("never sends a request through a proxy from the environment", async () => {
|
|
// Answers everything, as a real proxy would, and records what reaches it.
|
|
const proxied = [];
|
|
const proxy = http.createServer((req, res) => {
|
|
proxied.push(`${req.method} ${req.url}`);
|
|
res.end("answered by the proxy");
|
|
});
|
|
await new Promise((resolve) => proxy.listen(0, "127.0.0.1", resolve));
|
|
try {
|
|
const proxyUrl = `http://127.0.0.1:${proxy.address().port}`;
|
|
// ALL_PROXY as well, so that unsetting http_proxy alone would not pass.
|
|
// no_proxy leaves the loopback out, as an inherited one may: curl reads
|
|
// it before NO_PROXY, so a script exporting NO_PROXY alone would still go
|
|
// through the proxy. NO_PROXY is dropped, so that nothing the test itself
|
|
// inherited decides the outcome.
|
|
const env = {
|
|
http_proxy: proxyUrl,
|
|
ALL_PROXY: proxyUrl,
|
|
NO_PROXY: undefined,
|
|
no_proxy: "example.invalid",
|
|
};
|
|
const port = await freePort();
|
|
|
|
// The proxy is live: a bare curl with the same environment reaches it.
|
|
// Without this, "nothing reached the proxy" could pass for nothing.
|
|
await run("curl", ["-s", "-o", "/dev/null", "--max-time", "5", `http://127.0.0.1:${port}/`], env);
|
|
expect(proxied).toEqual([`GET http://127.0.0.1:${port}/`]);
|
|
proxied.length = 0;
|
|
|
|
const { code, stderr } = await runBootScript({ ...env, BOOT_PORT: String(port) });
|
|
|
|
expect(proxied).toEqual([]);
|
|
expect(code, stderr).toBe(0);
|
|
} finally {
|
|
await new Promise((resolve) => proxy.close(resolve));
|
|
}
|
|
}, 20000);
|
|
});
|
|
|
|
// The socket-level pass boots its own servers on 3099 and 3098, fixed ports:
|
|
// these cases fail if something else holds them, and must not overlap (vitest
|
|
// runs the tests of a file one after the other). BASE_URL is dropped from the
|
|
// environment each time: vite sets it to "/" in the test process, and the
|
|
// script reads a variable of that name.
|
|
describe("test-curl.sh", () => {
|
|
// True once nothing accepts a connection on the port, false after 3 s of
|
|
// trying: a server killed by the script's trap takes a moment to let go.
|
|
async function released(port) {
|
|
for (let i = 0; i < 30; i++) {
|
|
const open = await new Promise((resolve) => {
|
|
const socket = net.connect(port, "127.0.0.1");
|
|
socket.once("connect", () => {
|
|
socket.destroy();
|
|
resolve(true);
|
|
});
|
|
socket.once("error", () => resolve(false));
|
|
});
|
|
if (!open) return true;
|
|
await new Promise((resolve) => setTimeout(resolve, 100));
|
|
}
|
|
return false;
|
|
}
|
|
|
|
const runSocketPass = (env) => run("bash", ["test-curl.sh"], { BASE_URL: undefined, ...env });
|
|
|
|
// Same exposure as the boot check, 18 curl lines of it (#41). Before the
|
|
// export, a dead proxy made the script exit 7 with no output, and a live one
|
|
// would have answered every case in the API's place.
|
|
test("never sends a request through a proxy from the environment", async () => {
|
|
const proxied = [];
|
|
const proxy = http.createServer((req, res) => {
|
|
proxied.push(`${req.method} ${req.url}`);
|
|
res.end("answered by the proxy");
|
|
});
|
|
await new Promise((resolve) => proxy.listen(0, "127.0.0.1", resolve));
|
|
try {
|
|
const proxyUrl = `http://127.0.0.1:${proxy.address().port}`;
|
|
// Same environment as the boot check's case above, for the same reasons.
|
|
// FORCE_COLOR on top: node would colour the numbers the cases compare.
|
|
const env = {
|
|
http_proxy: proxyUrl,
|
|
ALL_PROXY: proxyUrl,
|
|
NO_PROXY: undefined,
|
|
no_proxy: "example.invalid",
|
|
FORCE_COLOR: "3",
|
|
};
|
|
|
|
// The proxy is live: a bare curl with the same environment reaches it.
|
|
const port = await freePort();
|
|
await run("curl", ["-s", "-o", "/dev/null", "--max-time", "5", `http://127.0.0.1:${port}/`], env);
|
|
expect(proxied).toEqual([`GET http://127.0.0.1:${port}/`]);
|
|
proxied.length = 0;
|
|
|
|
const { code, stdout, stderr } = await runSocketPass(env);
|
|
|
|
expect(proxied).toEqual([]);
|
|
expect(code, stdout + stderr).toBe(0);
|
|
// Every case ran against the API: a count, so a script cut short fails.
|
|
expect(stdout).toMatch(/=== Results: 45 passed, 0 failed ===/);
|
|
} finally {
|
|
await new Promise((resolve) => proxy.close(resolve));
|
|
}
|
|
}, 30000);
|
|
|
|
// A real neighbour on one of the two ports, as a second `npm test` or a
|
|
// stray server would be. Without the check up front the wait loop reaches the
|
|
// neighbour before the script's own server has died of EADDRINUSE, and the
|
|
// cases run against it (review of PR #45). Nothing must start, on either port.
|
|
test.each([
|
|
[3099, "http://localhost:3099"],
|
|
[3098, "http://localhost:3098"],
|
|
])("refuses to run when something else holds %i", async (port, url) => {
|
|
const asked = [];
|
|
const neighbour = http.createServer((req, res) => {
|
|
asked.push(req.url);
|
|
res.end("neighbour");
|
|
});
|
|
await new Promise((resolve, reject) => neighbour.once("error", reject).listen(port, resolve));
|
|
try {
|
|
const { code, stdout, stderr } = await runSocketPass({});
|
|
|
|
expect(stderr).toContain(`test-curl.sh: ${url} is already taken (curl exit 0)`);
|
|
expect(code).toBe(1);
|
|
expect(stdout).not.toMatch(/PASS|FAIL/);
|
|
// The neighbour saw the probe and nothing else: no case reached it.
|
|
expect(asked).toEqual(["/"]);
|
|
} finally {
|
|
await new Promise((resolve) => neighbour.close(resolve));
|
|
}
|
|
// The other port was free: no server of the script was left on it.
|
|
expect(await released(3099)).toBe(true);
|
|
expect(await released(3098)).toBe(true);
|
|
}, 20000);
|
|
|
|
// The rule is "anything but connection refused", not "an HTTP answer": an
|
|
// occupant that speaks no HTTP holds the port just as well.
|
|
test.each([
|
|
["closes at once", (socket) => socket.destroy(), /\(curl exit (52|56)\)/],
|
|
["accepts and stalls", () => {}, /\(curl exit 28\)/],
|
|
])("refuses to run when a listener that %s holds 3099", async (_, onConnection, exitCode) => {
|
|
const sockets = new Set();
|
|
const occupant = net.createServer((socket) => {
|
|
sockets.add(socket);
|
|
socket.on("error", () => {});
|
|
onConnection(socket);
|
|
});
|
|
await new Promise((resolve, reject) => occupant.once("error", reject).listen(3099, resolve));
|
|
try {
|
|
const { code, stdout, stderr } = await runSocketPass({});
|
|
|
|
expect(stderr).toContain("test-curl.sh: http://localhost:3099 is already taken");
|
|
expect(stderr).toMatch(exitCode);
|
|
expect(code).toBe(1);
|
|
expect(stdout).not.toMatch(/PASS|FAIL/);
|
|
} finally {
|
|
for (const socket of sockets) socket.destroy();
|
|
await new Promise((resolve) => occupant.close(resolve));
|
|
}
|
|
expect(await released(3098)).toBe(true);
|
|
}, 20000);
|
|
|
|
// A server that stays alive and never listens: only the deadline can end the
|
|
// wait. The fake `node` sleeps for the port named; the script's trap kills it.
|
|
test("gives up after 5 s on a server that is alive and never answers", async () => {
|
|
const bin = fs.mkdtempSync(path.join(os.tmpdir(), "vps-health-fake-node-"));
|
|
try {
|
|
fs.writeFileSync(
|
|
path.join(bin, "node"),
|
|
`#!/bin/sh\n[ "$PORT" = "3099" ] && exec sleep 30\nexec "${process.execPath}" "$@"\n`,
|
|
{ mode: 0o755 },
|
|
);
|
|
const { code, stdout, stderr, ms } = await runSocketPass({
|
|
PATH: `${bin}${path.delimiter}${process.env.PATH}`,
|
|
});
|
|
|
|
expect(stderr).toContain("test-curl.sh: no server answered at http://localhost:3099 within 5 s");
|
|
expect(code).toBe(1);
|
|
expect(stdout).not.toMatch(/PASS|FAIL/);
|
|
expect(ms).toBeGreaterThan(4000);
|
|
expect(ms).toBeLessThan(10000);
|
|
expect(await released(3098)).toBe(true);
|
|
} finally {
|
|
fs.rmSync(bin, { recursive: true, force: true });
|
|
}
|
|
}, 20000);
|
|
|
|
// A fake `node` first on the PATH stands in for a server that fails to start:
|
|
// it exits at once for the port named, and hands over to the real node
|
|
// otherwise. The script must name the URL instead of dying silently, for the
|
|
// main instance as for the one booted without HOSTS_INGEST_TOKEN.
|
|
test.each([
|
|
["3099", "http://localhost:3099"],
|
|
["3098", "http://localhost:3098"],
|
|
])("says so, and exits 1, when its server on %s does not start", async (port, url) => {
|
|
const bin = fs.mkdtempSync(path.join(os.tmpdir(), "vps-health-fake-node-"));
|
|
try {
|
|
fs.writeFileSync(
|
|
path.join(bin, "node"),
|
|
`#!/bin/sh\n[ "$PORT" = "${port}" ] && exit 1\nexec "${process.execPath}" "$@"\n`,
|
|
{ mode: 0o755 },
|
|
);
|
|
const { code, stdout, stderr, ms } = await runSocketPass({
|
|
PATH: `${bin}${path.delimiter}${process.env.PATH}`,
|
|
});
|
|
|
|
expect(stderr).toContain(`test-curl.sh: the server for ${url} exited before answering`);
|
|
expect(code).toBe(1);
|
|
expect(stdout).not.toMatch(/PASS|FAIL/);
|
|
// At once, not at the end of the 5 s wait: a message that did not stop
|
|
// the run would be repeated until the deadline.
|
|
expect(ms).toBeLessThan(3000);
|
|
// The server that did start is not left behind on its port.
|
|
expect(await released(3099)).toBe(true);
|
|
expect(await released(3098)).toBe(true);
|
|
} finally {
|
|
fs.rmSync(bin, { recursive: true, force: true });
|
|
}
|
|
}, 20000);
|
|
});
|
|
|
|
// The boot check only calls /health, so it proves the existence of the modules
|
|
// loaded at startup, nothing more. A require inside a route handler would load
|
|
// only when that route runs and escape it. Heuristic stand-in for "loaded at
|
|
// startup": every require starts at column 0 (a top-level statement), and there
|
|
// is no dynamic import. Comment lines are skipped, so an explanation quoting
|
|
// require( cannot break the pin.
|
|
//
|
|
// The pin reads literal calls only, a space before the parenthesis included
|
|
// (`require (`, `import (`). Known holes, among others, left open (#35):
|
|
// - a function written whole on a column-0 line, such as
|
|
// `const lazy = () => require("x");` or a one-line
|
|
// `function lazy() { return require("x"); }`;
|
|
// - a require( wrapped onto a column-0 line inside a function, which has the
|
|
// very shape of the top-level `} = require("node:fs");` that closes
|
|
// index.js's multi-line import: telling them apart takes a parser;
|
|
// - any indirection, such as an alias of require.
|
|
// None of them fits the style of this repository.
|
|
describe("runtime modules are all loaded at startup", () => {
|
|
const copyLine = dockerfile.match(/^\s*COPY\s+(.+)$/m);
|
|
const sources = copyLine ? copyLine[1].trim().split(/\s+/).slice(0, -1) : [];
|
|
const runtimeJs = sources.filter((f) => f.endsWith(".js"));
|
|
|
|
// True when the line may load a module after startup: an indented require,
|
|
// or a dynamic import at any column. Comment lines never count.
|
|
function loadsLate(line) {
|
|
if (/^\s*(\/\/|\/?\*)/.test(line)) return false;
|
|
return (/\brequire\s*\(/.test(line) && /^\s/.test(line)) || /\bimport\s*\(/.test(line);
|
|
}
|
|
|
|
test("the COPY line lists runtime modules", () => {
|
|
expect(runtimeJs.length).toBeGreaterThan(0);
|
|
});
|
|
|
|
test.each(runtimeJs)("%s requires only at module level", (file) => {
|
|
const lines = fs.readFileSync(path.join(ROOT, file), "utf-8").split("\n");
|
|
const offenders = [];
|
|
lines.forEach((line, i) => {
|
|
if (loadsLate(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`);
|
|
});
|
|
|
|
expect(offenders).toEqual([]);
|
|
});
|
|
|
|
// The detector itself. No runtime file breaks the pin today, so without these
|
|
// rows it could weaken, and the two holes closed in #35 reopen, with every
|
|
// test green.
|
|
test.each([
|
|
["an indented require", ' const x = require("./x");'],
|
|
["an indented require with a space before the parenthesis", ' const x = require ("./x");'],
|
|
["a dynamic import, even at column 0", 'const x = import("./x");'],
|
|
["a dynamic import with a space before the parenthesis", 'const x = import ("./x");'],
|
|
])("the load-late detector flags %s", (_, line) => {
|
|
expect(loadsLate(line)).toBe(true);
|
|
});
|
|
|
|
test.each([
|
|
["a top-level require", 'const x = require("./x");'],
|
|
["a comment line quoting require", ' // require("./x") here would load late'],
|
|
])("the load-late detector lets %s through", (_, line) => {
|
|
expect(loadsLate(line)).toBe(false);
|
|
});
|
|
});
|