test: strict CREATE TABLE, demo schema pins, grid message coverage (#50) - #51
Merged
Merged
Conversation
Two bugs shipped through a 283-test suite. Both were structural blind spots in how the tests were written, not bad luck: - Nearly every assertion is `toContain` on rendered text, which can prove a thing is present but never that something extra is absent. A phantom column named "2" therefore sailed through every LIST and LIST STRUCTURE check. - Four of twelve ClientMessage types had no test at all. grid-edit wrote straight to SQLite unvalidated because the grid tests only opened the grid and pressed Escape. Root cause first: parseCreate used to absorb any token it did not understand and invent a column from it. It now rejects a malformed column list — missing comma, missing type, unclosed paren, a third type argument — naming the offending column and creating nothing. Then the coverage those bugs needed: - DemoSchemas.test.ts pins the exact column list of every table the demos create, and asserts no column name is a bare number. - GridMessages.test.ts drives grid-edit, grid-delete, grid-new-row and grid-refresh, asserting the effect on the database. - CreateTableParse.test.ts covers the strict grammar both ways. Writing those tests immediately found two more bugs, fixed here: - Index metadata was keyed by table name alone, so opening PEOPLE in one database silently activated an index defined on a different database's PEOPLE — pointing record order at a column that need not exist there. Now keyed by (db, table, tag). Existing definitions are adopted into the database that owns the table; ambiguous ones are dropped and must be recreated with INDEX ON. - A bare `INPUT "..." TO var` at the REPL discarded the typed value: form-submit only stored values when a continuation existed, which never happens for a single statement. Also removes the input-request/input-response message types (declared but never sent or handled), teaches the New table wizard to emit NUM(p,s), and adds `npm run coverage` so untested modules stop hiding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #50.
Why
Two bugs shipped through a 283-test suite (both found while building #45). Neither was bad luck — the suite was structurally incapable of seeing them:
toContainproves presence, never absence. Nearly every assertion in the repo greps rendered text for a substring. A phantom column named2therefore passed everyLIST/LIST STRUCTUREcheck, because the value each test looked for was still there.ClientMessagetypes had zero tests.grid-editwrote straight to SQLite with no validation and nobody noticed, because the grid tests only opened the grid, read some text, and pressed Escape.ColumnMetaStoreleak shipped in TIME column type #43 with seven passing tests and two green CI jobs. All seven used a single database.What this does
Root cause: a strict
CREATE TABLEgrammarparseCreateused to callident()— "take the next token, whatever it is" — so a stray)became a type and a stray,became a column name. That is precisely howNUM(8,2)produced a phantom column. It now rejects a malformed column list, names the offending column, and creates nothing:Covers missing comma, missing type, unclosed paren, unclosed qualifier, a non-numeric qualifier, and a third type argument. Every valid form still parses (bare table, no-qualifier types,
CHAR(40),NUM(6),TIME(15),NUM(8,2), trailing comma).The coverage those bugs needed
tests/DemoSchemas.test.ts— golden, exact column lists for all seven tables the demos create, verified both through the parser and against the real SQLite table, plus a guard that no column name is a bare number.tests/GridMessages.test.ts— drivesgrid-edit,grid-delete,grid-new-row,grid-refreshand asserts the database effect.tests/CreateTableParse.test.ts— the strict grammar, both directions.npm run coverage(vitest + v8, reporting only, no thresholds). Currently ~57% statements; the browser UI is e2e-only.Two more bugs, found by writing those tests
Index metadata leaked across databases.
indexes/active_indexeswere keyed by table name alone, soUSE PEOPLEin one database silently activated an index defined on a different database'sPEOPLE— pointing the record order at a column that need not exist there, and breakingBROWSE/LIST. (This is the same defect I fixed inColumnMetaStorein BROWSE per-cell validation hook #45; the issue listed it as out of scope, but it blocked the demo-schema pins, so it's fixed here.) Now keyed by(db, table, tag).Migration: index definitions aren't re-derivable (only
INDEX ONcreates them), so rather than discard them, each legacy row is adopted into the one database that actually owns a table of that name. Rows whose owner is ambiguous (same table name in two databases) or gone are dropped and must be recreated withINDEX ON. The underlying SQLite index objects are untouched. Covered bytests/IndexStoreMigration.test.ts, including the ambiguous case and idempotency.A bare
INPUT "…" TO varat the REPL silently discarded the value.form-submitonly stored the submitted values when a continuation existed — which never happens for a single statement. It worked inside a program only because a following statement happened to create one. Values a form collects are now always stored. I verified the new e2e catches it by reverting the fix:Expected "Ada", Received "".Also
input-request/input-responseWS message types — declared in the protocol, never sent or handled by anything.INPUTgoes throughform-open/form-submit.NUM(p,s)is a real qualifier now, so the New table wizard accepts a width (8) or a precision,scale pair (8,2), and rejects a scale ≥ precision. Driven by a new e2e that asserts the created table has exactly one column. The strict-parser change adds no command, so it needs no sidebar action; its error path is covered in the REPL instead.Test plan
Suites run serially (they share
data/).npx tsc --noEmitclean.npm test— 358/358 vitest (was 316), 25 files.npx playwright test— 83/83 (was 79). Newtests/schema-errors.spec.ts(3): malformedCREATE TABLEerrors in the REPL and creates nothing;NUM(8,2)yields exactly three columns; a bareINPUTstores its value. Plus theNUM(p,s)wizard case inassistant.spec.ts.INPUTe2e fails against the unfixed code, so it isn't decorative.Docs
CHANGELOG.md(Added / Fixed / Changed),README.md(strictCREATE TABLEnote),CLAUDE.md— including a new Test discipline section recording what the suite could not see, so the next person doesn't rebuild the same blind spot.