commit pi sessions
This commit is contained in:
liph
@@ -0,0 +1,289 @@
|
||||
# Test design: add a rate-limit
|
||||
|
||||
> Deliverable for the `p-test` step of `c-design-build` (subject: "add a rate-limit").
|
||||
> Matches the existing `test/` conventions: `node --test` + `node:assert/strict`,
|
||||
> small independent tests, no LLM, no real clock by default.
|
||||
|
||||
## 1. Existing conventions
|
||||
|
||||
| Aspect | Convention in `test/` | Implication for rate-limit tests |
|
||||
|---|---|---|
|
||||
| Framework | `node:test` + `node:assert/strict` | Use the same; no jest/vitest. |
|
||||
| File layout | `test/<feature>.test.ts` next to `support/` | New file: `test/rate-limit.test.ts`. |
|
||||
| Imports | `import { describe, it, before } from "node:test";` | Identical. |
|
||||
| Assertions | `assert.equal`, `assert.deepEqual`, `assert.match` | Use strict asserts; avoid `assert.ok(x === y)`. |
|
||||
| Module type | `"type": "module"` in `package.json` | Use ESM, `.ts` extension in import specifiers. |
|
||||
| Test discovery | `node --test --experimental-strip-types <file>` | Same one-liner; no jest config. |
|
||||
| Time / randomness | Not yet used; tests are static and synchronous | Inject a `now()` function so we can advance the clock deterministically. |
|
||||
| Fixtures | `test/fixtures/valid-prompt.md` is a one-off example | Reuse the same pattern only if a non-trivial fixture is needed; prefer inline data. |
|
||||
| Support helpers | `test/support/frontmatter.ts` exposes `parseFrontmatter`, `parseChain` | Add `test/support/rate-limit.ts` for the SUT + clock injection; keep helpers pure. |
|
||||
|
||||
## 2. Test cases to add
|
||||
|
||||
Grouped by behavior. Each test is one `it(...)` and asserts one thing.
|
||||
|
||||
### Happy path
|
||||
- `allowRequest` returns `true` for the first request from a fresh key.
|
||||
- A burst up to the configured `limit` inside the window all return `true`.
|
||||
- A single request after the window has elapsed returns `true` again (full reset on window expiry).
|
||||
- Per-key isolation: a request from key `A` does not consume a slot for key `B`.
|
||||
- The limiter is independent across two distinct limiter instances (no shared global state).
|
||||
- `getStats(key)` reports `used`, `limit`, `remaining`, and `resetAt` matching the last operation.
|
||||
|
||||
### Edge cases
|
||||
- `limit = 0`: every request is rejected (degenerate but defined).
|
||||
- `limit = 1`: second request in the same window is rejected, the first is allowed.
|
||||
- Window boundary: request at exactly `windowMs` after the first is treated as a fresh window (off-by-one). Use a `now` function that returns the boundary value explicitly.
|
||||
- Two consecutive requests straddling the window: first allowed, second allowed, counts reset.
|
||||
- Large `limit` (e.g. `1_000_000`): behaves like an effectively unbounded limiter; one test with a high value is enough.
|
||||
- Empty / whitespace key string: rejected up front with a typed error, never reaches the bucket logic.
|
||||
- `now` going backwards (clock skew): does not grant extra capacity; refuses to decrement below zero.
|
||||
- `now` going forwards by exactly `windowMs + 1`: bucket fully reset; one extra request is allowed.
|
||||
|
||||
### Error cases
|
||||
- Missing required options (`limit` or `windowMs`): throws `TypeError` with a message that includes the missing key.
|
||||
- `limit` is negative or non-integer: throws `RangeError` (or `TypeError` — pick one and assert it consistently).
|
||||
- `windowMs` is zero or negative: throws `RangeError`.
|
||||
- Unknown option key (e.g. `limt` typo) is rejected to catch silent config bugs; throws with the unknown key in the message.
|
||||
- A custom `now` that throws propagates the error (no swallowed exceptions).
|
||||
- Concurrency: 100 synchronous calls to `allowRequest` for the same key with `limit = 50` result in exactly 50 `true` and 50 `false`, no off-by-one, no double-allow.
|
||||
|
||||
## 3. Test code
|
||||
|
||||
Target file: `test/rate-limit.test.ts`. The SUT is not yet in this repo; the
|
||||
tests assume it is implemented at `src/rate-limit.ts` with this minimal
|
||||
contract (declared here so the tests are runnable as soon as the SUT lands):
|
||||
|
||||
```ts
|
||||
// test/rate-limit.test.ts
|
||||
|
||||
import { describe, it } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import {
|
||||
createRateLimiter,
|
||||
type RateLimiter,
|
||||
type RateLimiterOptions,
|
||||
} from "../src/rate-limit.ts";
|
||||
|
||||
// --- helpers -------------------------------------------------------------
|
||||
|
||||
function makeLimiter(
|
||||
overrides: Partial<RateLimiterOptions> = {},
|
||||
now: () => number = () => 0,
|
||||
): RateLimiter {
|
||||
return createRateLimiter(
|
||||
{
|
||||
limit: 3,
|
||||
windowMs: 1_000,
|
||||
...overrides,
|
||||
},
|
||||
now,
|
||||
);
|
||||
}
|
||||
|
||||
// --- happy path ----------------------------------------------------------
|
||||
|
||||
describe("rate-limit: happy path", () => {
|
||||
it("allows the first request from a fresh key", () => {
|
||||
const rl = makeLimiter();
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
});
|
||||
|
||||
it("allows a burst up to the configured limit inside the window", () => {
|
||||
const rl = makeLimiter({ limit: 3 });
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
});
|
||||
|
||||
it("rejects the (limit + 1)th request in the same window", () => {
|
||||
const rl = makeLimiter({ limit: 3 });
|
||||
for (let i = 0; i < 3; i++) rl.allowRequest("user:1");
|
||||
assert.equal(rl.allowRequest("user:1"), false);
|
||||
});
|
||||
|
||||
it("allows again after the window has fully elapsed", () => {
|
||||
let t = 0;
|
||||
const clock = () => t;
|
||||
const rl = makeLimiter({ limit: 1, windowMs: 1_000 }, clock);
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
assert.equal(rl.allowRequest("user:1"), false);
|
||||
t = 1_001; // strictly past the window
|
||||
assert.equal(rl.allowRequest("user:1"), true);
|
||||
});
|
||||
|
||||
it("isolates buckets per key", () => {
|
||||
const rl = makeLimiter({ limit: 1 });
|
||||
assert.equal(rl.allowRequest("a"), true);
|
||||
assert.equal(rl.allowRequest("a"), false);
|
||||
assert.equal(rl.allowRequest("b"), true);
|
||||
assert.equal(rl.allowRequest("b"), false);
|
||||
});
|
||||
|
||||
it("keeps two limiter instances independent", () => {
|
||||
const a = makeLimiter({ limit: 1 });
|
||||
const b = makeLimiter({ limit: 1 });
|
||||
assert.equal(a.allowRequest("k"), true);
|
||||
assert.equal(b.allowRequest("k"), true);
|
||||
});
|
||||
|
||||
it("getStats reports remaining and resetAt matching the bucket state", () => {
|
||||
let t = 0;
|
||||
const rl = makeLimiter({ limit: 2, windowMs: 1_000 }, () => t);
|
||||
rl.allowRequest("user:1");
|
||||
const stats = rl.getStats("user:1");
|
||||
assert.equal(stats.used, 1);
|
||||
assert.equal(stats.remaining, 1);
|
||||
assert.equal(stats.resetAt, 1_000);
|
||||
});
|
||||
});
|
||||
|
||||
// --- edge cases ----------------------------------------------------------
|
||||
|
||||
describe("rate-limit: edge cases", () => {
|
||||
it("limit = 0 rejects every request", () => {
|
||||
const rl = makeLimiter({ limit: 0 });
|
||||
assert.equal(rl.allowRequest("k"), false);
|
||||
});
|
||||
|
||||
it("limit = 1 allows exactly one request then rejects", () => {
|
||||
const rl = makeLimiter({ limit: 1 });
|
||||
assert.equal(rl.allowRequest("k"), true);
|
||||
assert.equal(rl.allowRequest("k"), false);
|
||||
});
|
||||
|
||||
it("treats a request at exactly windowMs as a fresh window (off-by-one)", () => {
|
||||
let t = 0;
|
||||
const rl = makeLimiter({ limit: 1, windowMs: 1_000 }, () => t);
|
||||
assert.equal(rl.allowRequest("k"), true);
|
||||
t = 1_000; // exactly at the boundary
|
||||
assert.equal(rl.allowRequest("k"), true);
|
||||
});
|
||||
|
||||
it("does not grant extra capacity when the clock goes backwards", () => {
|
||||
let t = 1_000;
|
||||
const rl = makeLimiter({ limit: 1, windowMs: 1_000 }, () => t);
|
||||
assert.equal(rl.allowRequest("k"), true);
|
||||
t = 500; // clock skew backwards
|
||||
assert.equal(rl.allowRequest("k"), false);
|
||||
assert.equal(rl.getStats("k").used, 1);
|
||||
});
|
||||
|
||||
it("rejects an empty or whitespace key", () => {
|
||||
const rl = makeLimiter();
|
||||
assert.throws(() => rl.allowRequest(""), /key/);
|
||||
assert.throws(() => rl.allowRequest(" "), /key/);
|
||||
});
|
||||
|
||||
it("handles a very large limit without overflowing", () => {
|
||||
const rl = makeLimiter({ limit: 1_000_000 });
|
||||
for (let i = 0; i < 10; i++) {
|
||||
assert.equal(rl.allowRequest("k"), true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// --- error cases ---------------------------------------------------------
|
||||
|
||||
describe("rate-limit: error cases", () => {
|
||||
it("throws when limit is missing", () => {
|
||||
// @ts-expect-error: limit intentionally omitted
|
||||
assert.throws(() => createRateLimiter({ windowMs: 1_000 }), /limit/);
|
||||
});
|
||||
|
||||
it("throws when windowMs is missing", () => {
|
||||
// @ts-expect-error: windowMs intentionally omitted
|
||||
assert.throws(() => createRateLimiter({ limit: 1 }), /windowMs/);
|
||||
});
|
||||
|
||||
it("throws on a negative limit", () => {
|
||||
assert.throws(() => makeLimiter({ limit: -1 }), /limit/);
|
||||
});
|
||||
|
||||
it("throws on a non-integer limit", () => {
|
||||
assert.throws(() => makeLimiter({ limit: 1.5 }), /limit/);
|
||||
});
|
||||
|
||||
it("throws on a zero or negative windowMs", () => {
|
||||
assert.throws(() => makeLimiter({ windowMs: 0 }), /windowMs/);
|
||||
assert.throws(() => makeLimiter({ windowMs: -1 }), /windowMs/);
|
||||
});
|
||||
|
||||
it("rejects an unknown option key", () => {
|
||||
assert.throws(
|
||||
() => makeLimiter({ limt: 3 } as unknown as Partial<RateLimiterOptions>),
|
||||
/limt/,
|
||||
);
|
||||
});
|
||||
|
||||
it("propagates errors from a custom now()", () => {
|
||||
const rl = createRateLimiter(
|
||||
{ limit: 1, windowMs: 1_000 },
|
||||
() => { throw new Error("clock broken"); },
|
||||
);
|
||||
assert.throws(() => rl.allowRequest("k"), /clock broken/);
|
||||
});
|
||||
|
||||
it("is exact under synchronous burst: exactly limit trues, then falses", () => {
|
||||
const rl = makeLimiter({ limit: 50 });
|
||||
let allowed = 0;
|
||||
let rejected = 0;
|
||||
for (let i = 0; i < 100; i++) {
|
||||
if (rl.allowRequest("k")) allowed++;
|
||||
else rejected++;
|
||||
}
|
||||
assert.equal(allowed, 50);
|
||||
assert.equal(rejected, 50);
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
## 4. How to run
|
||||
|
||||
From the repository root:
|
||||
|
||||
```bash
|
||||
# all tests in this directory
|
||||
cd test && npm test
|
||||
|
||||
# only the new rate-limit tests
|
||||
cd test && node --test --experimental-strip-types rate-limit.test.ts
|
||||
|
||||
# a single test by name pattern
|
||||
cd test && node --test --experimental-strip-types \
|
||||
--test-name-pattern="rejects the \\(limit \\+ 1\\)th" rate-limit.test.ts
|
||||
```
|
||||
|
||||
Notes:
|
||||
- `--experimental-strip-types` is required because the existing `test/package.json`
|
||||
already targets Node's built-in TS stripping; no separate tsconfig needed.
|
||||
- These tests are pure and synchronous, so the whole file should finish in
|
||||
well under 100 ms once the SUT lands.
|
||||
|
||||
## 5. Coverage gaps (flagged, not refactored in this pass)
|
||||
|
||||
1. **SUT does not exist yet.** `src/rate-limit.ts` is assumed. Until it
|
||||
lands with the contract declared at the top of section 3, the test file
|
||||
will fail to import. This is the single largest blocker.
|
||||
2. **Async / distributed cases are not covered.** The contract above is
|
||||
synchronous. If the production limiter is async (e.g. backed by Redis
|
||||
with `INCR` + `EXPIRE`), the burst test will need to be reworked; a
|
||||
`before`/`after` test that injects an in-memory store would be the
|
||||
smallest refactor.
|
||||
3. **Concurrency under a real event loop is not exercised.** A single
|
||||
synchronous `for` loop does not stress the microtask queue. If the SUT
|
||||
becomes async, add a `Promise.all` of N concurrent calls and assert
|
||||
that the count of `true` results equals `limit`.
|
||||
4. **No metrics / observability surface is assumed.** If the limiter is
|
||||
later expected to emit counters or events, that needs an interface
|
||||
change first; tests would follow in a follow-up.
|
||||
5. **No per-route or weighted limits.** A real production limiter often
|
||||
supports per-route buckets or weighted costs per request. Out of scope
|
||||
here; would require a contract change.
|
||||
6. **No clock injection in the SUT contract yet.** The design assumes
|
||||
`createRateLimiter(opts, now?)`. If the chosen implementation uses a
|
||||
module-level `Date.now()`, the clock-dependent tests in this file
|
||||
cannot be written deterministically; the SUT should be refactored to
|
||||
accept a `now` parameter (smallest viable change) before these tests
|
||||
are added.
|
||||
Reference in New Issue
Block a user