Skip to content

test: add randomized rendering invariants - #133

Open
tecfu wants to merge 3 commits into
masterfrom
test/render-invariants
Open

tecfu wants to merge 3 commits into
masterfrom
test/render-invariants

Conversation

@tecfu

@tecfu tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Add deterministic randomized coverage for a core rendering invariant: fixed-width tables must not emit lines wider than their declared geometry. The generated cases vary row counts, column counts, ASCII, accented, CJK, and emoji content without adding a runtime dependency.

@tecfu

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge (with one fix)

Nice addition — a deterministic seeded LCG, no new deps, and the invariant (declared geometry respected under mixed ASCII/accent/CJK/emoji) is exactly the class of property the 71-case matrix can't enumerate. 111/111 vitest (100 samples × 4-4 cells each, no flakes across my 3 runs), 21/21 goldens, matrix 0 (test-only PR), typecheck and lint clean.

One bug to fix: the line splitter is double-escaped:

for (const line of output.split(/\\r?\\n/).filter(Boolean))

In a regex literal that matches a literal backslash then optional r then LF — it never matches, so split yields the whole table as one string. It only passes because displayWidth happens to split on newlines internally and returns the widest line. That's luck, not design: change the assertion to a plain per-line width check (or to displayWidth of the whole string, which already means "widest line") and use one \. Everything still passes when corrected (verified locally with the fix applied), so this is hygiene, not behavior — but a future reader will trust the code to do per-line checks when it does none.

Sincerely qwen3.8-flash-next

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge, with a test-quality fix

The deterministic seeded generator is a good way to add broad coverage without introducing a property-testing dependency, and the input set exercises ASCII, accented text, CJK, and emoji.

One test-quality issue remains: the line split in the PR is double-escaped, so it matches a literal backslash before an optional r rather than normal CR/LF separators. Consequently the test is not actually iterating over individual rendered lines as its wording claims. It happens to remain useful because displayWidth reduces the whole string to its widest line, but that is accidental coupling to the helper's semantics.

Please change it to a normal CR/LF regex, or explicitly test displayWidth of the whole string if widest-line behavior is what is intended. The invariant itself is worthwhile.

— GPT-5.6 Luna

tecfu commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Re-review: test issue fixed

The randomized invariant remains useful, and the line-splitting expression is now a real CR/LF regex rather than the previously double-escaped pattern.

CI is running on the corrected test. No implementation blocker found.

— GPT-5.6 Luna

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant