Merge pull request 'Fix duplicate ids, validate CLI input, harden load/save' (#2) from fix/review-findings into main
Reviewed-on: #2
This commit was merged in pull request #2.
This commit is contained in:
5 files changed
+400
-26
No files matched your search
@@ -0,0 +1,122 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, dirname } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const CLI = fileURLToPath(new URL("../src/cli.js", import.meta.url));
|
||||
|
||||
function run(file, ...args) {
|
||||
return spawnSync(process.execPath, [CLI, ...args], {
|
||||
env: { ...process.env, TODO_FILE: file },
|
||||
encoding: "utf8",
|
||||
});
|
||||
}
|
||||
|
||||
const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json");
|
||||
|
||||
test("ls with no file prints the empty message and writes nothing", () => {
|
||||
const file = tempFile();
|
||||
const res = run(file, "ls");
|
||||
assert.equal(res.status, 0);
|
||||
assert.equal(res.stdout.trim(), "Nothing to do.");
|
||||
assert.equal(existsSync(file), false);
|
||||
});
|
||||
|
||||
test("no command lists the todos", () => {
|
||||
const file = tempFile();
|
||||
run(file, "add", "a");
|
||||
const res = run(file);
|
||||
assert.equal(res.status, 0);
|
||||
assert.equal(res.stdout.trim(), "[ ] 1. a");
|
||||
});
|
||||
|
||||
test("add trims surrounding whitespace from the text", () => {
|
||||
const file = tempFile();
|
||||
run(file, "add", " buy", "milk ");
|
||||
assert.deepEqual(JSON.parse(readFileSync(file, "utf8")), [{ id: 1, text: "buy milk", done: false }]);
|
||||
});
|
||||
|
||||
test("add fails readably when no ids are left, without saving", () => {
|
||||
const file = tempFile();
|
||||
const before = JSON.stringify([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }]);
|
||||
writeFileSync(file, before);
|
||||
const res = run(file, "add", "b");
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, /^No ids left/);
|
||||
assert.equal(readFileSync(file, "utf8"), before);
|
||||
});
|
||||
|
||||
test("rm on a file with duplicate ids removes only the first of them", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }]));
|
||||
const res = run(file, "rm", "2");
|
||||
assert.equal(res.status, 0, res.stderr);
|
||||
assert.equal(res.stdout.trim(), "[ ] 3. c");
|
||||
});
|
||||
|
||||
test("add with no text fails without saving", () => {
|
||||
const file = tempFile();
|
||||
const res = run(file, "add", " ");
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, /Missing todo text/);
|
||||
assert.equal(existsSync(file), false);
|
||||
});
|
||||
|
||||
for (const [label, args, message] of [
|
||||
["missing id", [], /Invalid id: \(missing\)/],
|
||||
["non-numeric id", ["abc"], /Invalid id: abc/],
|
||||
["hex id", ["0x1"], /Invalid id: 0x1/],
|
||||
["exponent id", ["1e0"], /Invalid id: 1e0/],
|
||||
["decimal id", ["1.0"], /Invalid id: 1\.0/],
|
||||
["padded id", [" 1 "], /Invalid id: 1 /],
|
||||
["empty id", [""], /Invalid id: \n/],
|
||||
["unsafe id", ["99999999999999999999"], /Invalid id: 99999999999999999999/],
|
||||
["leading-zero id", ["01"], /Invalid id: 01/],
|
||||
["zero id", ["0"], /Invalid id: 0/],
|
||||
["unknown id", ["99"], /No todo with id 99/],
|
||||
]) {
|
||||
for (const cmd of ["done", "rm"]) {
|
||||
test(`${cmd} with ${label} fails and leaves the file unchanged`, () => {
|
||||
const file = tempFile();
|
||||
run(file, "add", "a");
|
||||
const before = readFileSync(file, "utf8");
|
||||
const res = run(file, cmd, ...args);
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, message);
|
||||
assert.equal(readFileSync(file, "utf8"), before);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
test("unknown command prints usage even when the file is corrupt", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, "{ not json");
|
||||
const res = run(file, "--help");
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, /Unknown command: --help\nUsage:/);
|
||||
});
|
||||
|
||||
test("a failed save gives a readable error instead of a stack trace", { skip: process.getuid?.() === 0 && "root ignores file permissions" }, () => {
|
||||
const file = tempFile();
|
||||
chmodSync(dirname(file), 0o500);
|
||||
try {
|
||||
const res = run(file, "add", "a");
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, /^Could not save /);
|
||||
assert.doesNotMatch(res.stderr, /at .*\.js:\d+/);
|
||||
} finally {
|
||||
chmodSync(dirname(file), 0o700);
|
||||
}
|
||||
});
|
||||
|
||||
test("a corrupt file gives a readable error instead of a stack trace", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, "{ not json");
|
||||
const res = run(file, "ls");
|
||||
assert.equal(res.status, 1);
|
||||
assert.match(res.stderr, /^Could not read /);
|
||||
assert.doesNotMatch(res.stderr, /at .*\.js:\d+/);
|
||||
});
|
||||
+145
-1
@@ -1,12 +1,29 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { add, complete, remove, format } from "../src/todo.js";
|
||||
import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, linkSync, lstatSync, statSync, chmodSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, dirname } from "node:path";
|
||||
import { load, save, add, has, complete, remove, format } from "../src/todo.js";
|
||||
|
||||
const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json");
|
||||
const isRoot = process.getuid?.() === 0;
|
||||
|
||||
test("add appends an open todo", () => {
|
||||
const todos = add([], "buy milk");
|
||||
assert.deepEqual(todos, [{ id: 1, text: "buy milk", done: false }]);
|
||||
});
|
||||
|
||||
test("add after a remove does not reuse an existing id", () => {
|
||||
const todos = add(remove(add(add([], "a"), "b"), 1), "c");
|
||||
assert.deepEqual(todos.map((t) => t.id), [2, 3]);
|
||||
});
|
||||
|
||||
test("has reports whether an id exists", () => {
|
||||
const todos = add([], "a");
|
||||
assert.equal(has(todos, 1), true);
|
||||
assert.equal(has(todos, 2), false);
|
||||
});
|
||||
|
||||
test("complete marks only the matching todo", () => {
|
||||
const todos = complete(add(add([], "a"), "b"), 2);
|
||||
assert.equal(todos[0].done, false);
|
||||
@@ -21,3 +38,130 @@ test("remove drops the matching todo", () => {
|
||||
test("format renders an empty list", () => {
|
||||
assert.equal(format([]), "Nothing to do.");
|
||||
});
|
||||
|
||||
test("load returns an empty list for a missing file", () => {
|
||||
assert.deepEqual(load(tempFile()), []);
|
||||
});
|
||||
|
||||
test("add refuses an id that load() would reject", () => {
|
||||
assert.throws(() => add([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }], "b"), /No ids left/);
|
||||
});
|
||||
|
||||
test("add handles a very large list", () => {
|
||||
const todos = Array.from({ length: 200_000 }, (_, i) => ({ id: i + 1, text: "x", done: false }));
|
||||
assert.equal(add(todos, "y").at(-1).id, 200_001);
|
||||
});
|
||||
|
||||
test("save then load round-trips and leaves no temp file", () => {
|
||||
const file = tempFile();
|
||||
const todos = add([], "a");
|
||||
save(file, todos);
|
||||
assert.deepEqual(load(file), todos);
|
||||
assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]);
|
||||
});
|
||||
|
||||
test("save writes through a symlink instead of replacing it", () => {
|
||||
const real = tempFile();
|
||||
const link = join(dirname(tempFile()), "link.json");
|
||||
save(real, []);
|
||||
symlinkSync(real, link);
|
||||
save(link, add([], "a"));
|
||||
assert.equal(lstatSync(link).isSymbolicLink(), true);
|
||||
assert.deepEqual(load(real).map((t) => t.text), ["a"]);
|
||||
});
|
||||
|
||||
test("load renumbers duplicate ids left by the old add() bug", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, JSON.stringify([
|
||||
{ id: 2, text: "b", done: false },
|
||||
{ id: 2, text: "c", done: true },
|
||||
{ id: 3, text: "d", done: false },
|
||||
]));
|
||||
assert.deepEqual(load(file).map((t) => [t.id, t.text, t.done]), [[2, "b", false], [4, "c", true], [3, "d", false]]);
|
||||
});
|
||||
|
||||
test("save writes in place when the file has other hard links", () => {
|
||||
const file = tempFile();
|
||||
const other = join(dirname(file), "other.json");
|
||||
save(file, []);
|
||||
linkSync(file, other);
|
||||
save(file, add([], "a"));
|
||||
assert.deepEqual(load(other).map((t) => t.text), ["a"]);
|
||||
});
|
||||
|
||||
test("save writes in place when only the file, not its directory, is writable", { skip: isRoot && "root ignores file permissions" }, () => {
|
||||
const file = tempFile();
|
||||
save(file, []);
|
||||
chmodSync(dirname(file), 0o500);
|
||||
try {
|
||||
save(file, add([], "a"));
|
||||
assert.deepEqual(load(file).map((t) => t.text), ["a"]);
|
||||
assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]);
|
||||
} finally {
|
||||
chmodSync(dirname(file), 0o700);
|
||||
}
|
||||
});
|
||||
|
||||
test("save through a dangling symlink creates the target and keeps the link", () => {
|
||||
const dir = dirname(tempFile());
|
||||
const link = join(dir, "link.json");
|
||||
symlinkSync("real.json", link);
|
||||
save(link, add([], "a"));
|
||||
assert.equal(lstatSync(link).isSymbolicLink(), true);
|
||||
assert.deepEqual(load(join(dir, "real.json")).map((t) => t.text), ["a"]);
|
||||
});
|
||||
|
||||
test("save refuses a read-only file", { skip: isRoot && "root ignores file permissions" }, () => {
|
||||
const file = tempFile();
|
||||
save(file, []);
|
||||
chmodSync(file, 0o444);
|
||||
assert.throws(() => save(file, add([], "a")), /Could not save .*todos\.json/);
|
||||
assert.deepEqual(load(file), []);
|
||||
});
|
||||
|
||||
test("save keeps the file's permissions", () => {
|
||||
const file = tempFile();
|
||||
save(file, []);
|
||||
chmodSync(file, 0o600);
|
||||
save(file, add([], "a"));
|
||||
assert.equal(statSync(file).mode & 0o777, 0o600);
|
||||
});
|
||||
|
||||
test("save reports a readable error and cleans up when it can't write", { skip: isRoot && "root ignores file permissions" }, () => {
|
||||
const file = tempFile();
|
||||
chmodSync(dirname(file), 0o500);
|
||||
try {
|
||||
assert.throws(() => save(file, []), /Could not save .*todos\.json/);
|
||||
assert.deepEqual(readdirSync(dirname(file)), []);
|
||||
} finally {
|
||||
chmodSync(dirname(file), 0o700);
|
||||
}
|
||||
});
|
||||
|
||||
test("load rejects invalid JSON with a readable error", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, "{ not json");
|
||||
assert.throws(() => load(file), /Could not read .*todos\.json/);
|
||||
});
|
||||
|
||||
test("load rejects JSON that is not a list", () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, "{}");
|
||||
assert.throws(() => load(file), /expected a list of todos/);
|
||||
});
|
||||
|
||||
for (const entry of [
|
||||
"null",
|
||||
"1",
|
||||
'{"text":"a","done":false}',
|
||||
'{"id":"1","text":"a","done":false}',
|
||||
'{"id":0,"text":"a","done":false}',
|
||||
'{"id":-1,"text":"a","done":false}',
|
||||
`{"id":${Number.MAX_SAFE_INTEGER},"text":"a","done":false}`,
|
||||
]) {
|
||||
test(`load rejects an invalid entry: ${entry}`, () => {
|
||||
const file = tempFile();
|
||||
writeFileSync(file, `[${entry}]`);
|
||||
assert.throws(() => load(file), /entry 1 is not a valid todo/);
|
||||
});
|
||||
}
|
||||
Reference in new issue
Block a user