Repository navigation
test: round-trip the PostgreSQL regression corpus with a known-failures ratchet - #366
pyramation wants to merge 2 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 4 medium 💬 Inline comments (4)
🧹 Nitpicks (1) — 🟢 1 low
The change introduces a corpus pipeline built from PostgreSQL's regression test SQL: a new
Reviewed commit: b05c71d |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds regression-corpus fixture generation and ratcheted known-failure tests for the deparser and plpgsql-deparser, with a handful of tooling robustness gaps to address.
Key findings
- 🟡 Partial UPDATE_KNOWN_FAILURES run wipes the ratchet file — regress-corpus.test.ts:39
- 🟡 Paren counter goes negative and swallows later statements — make-regress-fixtures.ts:43
- 🟡 Fallback regex split corrupts dollar-quoted strings — make-regress-fixtures.ts:64
- 🟡
globundeclared in deparser package — make-regress-fixtures.ts:14
| } catch { | ||
| // e.g. strings.sql has intentionally invalid escapes the scanner rejects | ||
| pieces = sql.split(/;[ \t]*\r?\n/); | ||
| } | ||
| return pieces | ||
| .map((s) => s.replace(/^(\s*--[^\n]*\n|\s+)+/, '').trim()) | ||
| .filter((s) => s.length > 0); |
There was a problem hiding this comment.
🟡 bug · medium
Fallback regex split corrupts dollar-quoted strings
When scanSync rejects a file (e.g. strings.sql with intentionally invalid escapes), splitStatements falls back to sql.split(/;[ \t]*\r? /) (make-regress-fixtures.ts:66), which is quote- and dollar-quote-blind. Any dollar-quoted body ($$...; ...$$, $_$...$_$) or a string literal containing ; is cut in half; each fragment then fails parseSync and is silently counted as 'skipped' (make-regress-fixtures.ts:92), so whole functions vanish from the corpus with no warning. The fragments also shift the per-file index i, so subsequent keys contain different statements than the generator's numbering implies.
📋 Prompt for AI Agents
In packages/deparser/scripts/make-regress-fixtures.ts, in splitStatements (lines 60-71), make the catch-block fallback dollar-quote aware: before sql.split(/;[ \t]*\r?\n/), replace every dollar-quoted span (regex /($[A-Za-z_]$)[\s\S]?\1/g) with a placeholder of identical byte length, split, then restore — or record the file as skipped with a log line instead of using the regex fallback. Otherwise fragments of a split dollar-quoted body fail parseSync and are silently dropped from the regress corpus, hiding entire functions from the fixture set.
There was a problem hiding this comment.
The fallback only runs for files the scanner rejects outright (just strings.sql at this pin). A piece is kept only if it parses as a full statement on its own, so a dollar-quoted body split in the middle gets dropped and never stored corrupted. Left as is.
There was a problem hiding this comment.
The fallback sql.split(/;[ \t]*\r?\n/) is still present unchanged in the current diff (lines 63-67 of the catch block). The argument that 'a split fragment gets dropped and never stored corrupted' does not address the finding: the problem is that whole statements (e.g. entire PL/pgSQL functions with dollar-quoted bodies) are silently lost from the fixture corpus, and the index i is shifted for all subsequent statements in that file, corrupting their keys. The fix requires either making the fallback dollar-quote-aware (placeholder-swap approach) or logging and skipping the file entirely — neither is present here.
| */ | ||
| import { execFileSync } from 'child_process'; | ||
| import * as fs from 'fs'; | ||
| import { sync as globSync } from 'glob'; |
There was a problem hiding this comment.
🟡 bug · medium
glob undeclared in deparser package
packages/deparser/scripts/make-regress-fixtures.ts:14 imports { sync as globSync } from 'glob', but glob is only declared in the root package.json and packages/pgsql-cli/package.json — not in packages/deparser/package.json. Under pnpm's default isolated node_modules layout, the new fixtures:regress script fails at import time with "Cannot find module 'glob'", so the corpus cannot be regenerated from a fresh clone.
📋 Prompt for AI Agents
In packages/deparser/package.json, add a devDependencies entry for glob matching the version already used at the repo root (^11.0.0), because packages/deparser/scripts/make-regress-fixtures.ts line 14 imports globSync from glob and pnpm will not resolve an undeclared transitive dependency from a package's own scripts.
There was a problem hiding this comment.
glob comes from the root package.json (^11.0.0) and is hoisted. The existing make-fixtures.ts, make-fixtures-ast.ts and make-fixtures-sql.ts scripts import it the same way, so I kept the convention.
There was a problem hiding this comment.
The candidate diff for 'packages/deparser/package.json' only shows the addition of the 'fixtures:regress' script entry — it does not include a 'glob' entry under 'devDependencies'. The fix described (adding 'glob': '^11.0.0' to devDependencies) is not visible in any of the evidence provided. The user's claim that hoisting from the root covers this is unreliable under pnpm's isolated node_modules layout, and the other scripts cited as precedent have the same latent issue. The original finding remains unresolved.
… ones; clamp scanner paren depth
|
Folded into #371 |
Summary
Runs every statement of the SQL that libpg_query vendors from PostgreSQL through parse → deparse → parse, inside the existing
pgsql-deparserandplpgsql-deparsertest jobs. There is no new CI job. A deparser change that breaks any of the 38k statements now fails CI. Today only the hand-picked kitchen-sink statements are checked.Corpus.
__fixtures__/regress/statements.json.gz(714 KB) holds 38,269 unique parseable statements from 400 files:test/sql/postgres_regresstest/sql/plpgsql_regresstest/sql/deparse*test/plpgsql_samples.sqlThey are taken from libpg_query at
b0f5f33, thex-publish.libpgQueryCommitof the pinnedlibpg-query@18.2.1. Keys matchgenerated.json(postgres_regress/arrays-12.sql). To regenerate:make-regress-fixtures.tssplits each file on top-level;tokens fromscanSync, so quoting and comments are handled andCREATE RULE (…;…)/BEGIN ATOMIC … ENDstay whole. It then drops duplicate and unparseable statements (psql meta-commands, intentional errors).Ratchet.
__tests__/regress/regress-corpus.test.ts(deparser) has one test per source file and callsTestUtils.expectAstMatch, both pretty and non-pretty. It compares the failing keys withknown-failures.deparser.json: 594 statements in 47 files.plpgsql-deparser/__tests__/regress-corpus.test.tsdoes the same for PL/pgSQL functions and DO blocks againstknown-failures.plpgsql-deparser.json(72 statements).A new failure fails the test, and so does a listed statement that now passes, which forces the list to shrink as bugs are fixed. To refresh after a deliberate change:
The workflow is documented in
.agents/skills/testing-fixtures/SKILL.md.Cost: ~20 s in the deparser job and ~3 s in the plpgsql-deparser job. Fixture size is ~0.75 MB in total.
Evidence
TruncateStmtemittingCONTINUE IDENTITYforRESTART IDENTITY):✕ postgres_regress/truncate.sql,Tests: 1 failed, 399 passed.JsonFuncExprinside aRangeFunction(JSON_TABLE / JSON_QUERY in FROM)ALTER OPERATOR FAMILY … ADD FUNCTION n (types) f(args)argument orderarrays.sqlLink to Devin session: https://app.devin.ai/sessions/91c28df261ac4b318f39d1c7242d008c
Open in Devin Desktop: https://app.devin.ai/desktop/session/91c28df261ac4b318f39d1c7242d008c?variant=devin
Requested by: @pyramation