Add test/build: extension-metadata tests + raw SQL syntax checks - #33
Open
jnasbyupgrade wants to merge 8 commits into
Open
Add test/build: extension-metadata tests + raw SQL syntax checks#33jnasbyupgrade wants to merge 8 commits into
jnasbyupgrade wants to merge 8 commits into
Conversation
Moves install-mechanics testing out of test/sql/install.sql and the top of
test/sql/pgtap.sql (which tested extension packaging -- dependency
declarations, clean install/uninstall -- not test_factory's actual
registering/getting business logic) into test/build/install.sql, using
pgxntool's test/build feature. Quoting its own purpose comment in
pgxntool/base.mk:
Validates that extension SQL files are syntactically correct by running
files from test/build/ through pg_regress. This provides better error
messages than CREATE EXTENSION failures.
Also adds test/build/syntax.sql, which \i's the raw generated versioned SQL
files (sql/test_factory--0.5.0.sql, sql/test_factory_pgtap--0.1.0.sql)
directly rather than through CREATE EXTENSION, so a genuine syntax error
shows up immediately instead of being obscured by a generic CREATE EXTENSION
failure. Verified this works by deliberately introducing a typo locally and
confirming both new tests surfaced it clearly, then reverting.
Both new files hit known, harmless errors baked into their expected output
(see comments in each file for the full explanation):
- pg_extension_config_dump() always errors when its script is \i'd
directly instead of run via CREATE/ALTER EXTENSION.
- `SET ROLE ""` fails because the role-restore GUC set via
pg_catalog.set_config(..., true) is transaction-scoped, and plain \i
(autocommit) gives each statement its own implicit transaction.
Also dials VERBOSITY down to "default" (from psql.sql's "verbose") since
verbose mode's backend source LOCATION lines differ across PG major
versions/builds, which would break single-expected-file matching across the
PG12-17 CI matrix.
Enables PGXNTOOL_ENABLE_TEST_BUILD explicitly (matching this Makefile's
existing preference for explicit-over-implicit config) and adds a CI step
that runs `make test-build` directly: pgxn-tools' pg-build-test invokes
`make installcheck`, never `make test`, so test-build's dependency chain
(wired only into `test`) would otherwise silently never run in CI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pgxntool/run-test-build.sh (vendored) shells out to rsync to sync
test/build/*.sql into test/build/sql/, but the pgxn/pgxn-tools container
image doesn't ship rsync -- caught by the first real CI run on this PR
("rsync: command not found", pgxntool/base.mk:473: test-build, Error 127).
Not reproducible locally since this dev container already has rsync.
Worth a pgxntool-test issue since any project enabling
PGXNTOOL_ENABLE_TEST_BUILD on this same image would hit the identical gap;
not fixed here (that's vendored, not this repo's own script).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both raw SQL files print a query-result row for their
pg_catalog.set_config('..._role', current_user, true) call (the
original-role save at the top of each). current_user is whichever role
actually connects -- this dev container connects as root, CI's
pgxn/pgxn-tools connects as postgres -- so the expected output generated
locally didn't match CI's real run (confirmed by the first real CI run
after the rsync fix: "root" expected, "postgres" got, both PG13's own
failure and independently confirmed by a peer session's diagnosis of the
same run).
Fixed with `\o /dev/null`: discards normal query-result output for the rest
of the file, but NOTICE/WARNING/ERROR go through a separate stream `\o`
doesn't touch, so every error this test actually cares about -- including a
genuine future syntax error -- still shows up in the diff untouched.
Regenerated test/build/expected/syntax.out from a real run (never
hand-authored); confirmed clean on both PG12 and PG17.
Note: `make results` does not handle test/build's separate results/expected
directories (it only copies test/results/*.out -> test/expected/*.out) --
had to copy test/build/results/syntax.out -> test/build/expected/syntax.out
by hand, still from a real run, never typed by hand. Worth a pgxntool-test
issue: `make results` silently doesn't cover test-build's own output.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rim comments CI (.github/workflows/ci.yml): pg-build-test's own failure detection (`make installcheck || status=$?`) can never actually trip -- pgxntool marks `installcheck` .IGNORE, so `make installcheck` always exits 0 regardless of real regression failures (verified locally with a deliberately broken test). Confirmed from pg-build-test's actual source (pgxn/docker-pgxn-tools bin/pg-build-test): `make all; sudo make install; make installcheck || status=$?` -- it never calls `make test` either, so test-build was never reachable through it regardless. Replaced with a direct `make verify-results`, which inspects the real TAP output instead of trusting an exit code, and pulls in test-build via the same `test` dependency chain it already runs. Confirmed locally (4 repeated runs) that a single `make verify-results` with no separate `make install` first is deterministic once pg-build-test's own `MAKEFLAGS=-j $(nproc)` export is out of the picture -- that export (not anything the container sets globally) was the actual source of the install/installcheck race PR #23 found and worked around; not re-litigating that fix there, just noting root cause now confirmed. test/build/syntax.sql: wrapped the \i calls in BEGIN/ROLLBACK with ON_ERROR_ROLLBACK on, so nothing persists in the database whether this runs under pg_regress or ad hoc locally (previously: autocommit, no rollback, would leave a real mess in a developer's own database). Bonus: running the whole thing as one transaction is also more faithful to how CREATE EXTENSION actually behaves, so `current_setting('..._role')` now correctly survives to the end of each file -- the third known/expected error (SET ROLE "" from the role-restore code) is gone entirely, not just documented away. Regenerated expected output from a real run. Trimmed both test/build/*.sql files' comments substantially and added a one-line cross-reference between them (install.sql tests packaging via a real CREATE EXTENSION; syntax.sql tests raw SQL syntax by bypassing it) -- the previous version buried that distinction in multi-paragraph essays. CLAUDE.md: noted that this PGXN distribution ships two structurally similar extensions (test_factory, test_factory_pgtap) and that the test suite shares infrastructure between them accordingly. Verified on both PG12 and PG17: full make test passes, and a plain `psql -f test/build/syntax.sql` against a scratch database leaves zero test_factory objects behind afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit (95d8be4) removed this based on local testing that turned out to be invalid: a concurrent process in this dev container had installed PostgreSQL 18, silently shifting the default `pg_config` on PATH out from under an unrelated test, which produced a false "it works without the extra step" result. Re-tested properly (PG_CONFIG pinned to match the actual target cluster, extension deliberately removed from the system first) and found a REAL, pre-existing pgxntool bug, independent of anything in this PR: on a genuinely fresh system, `make test`/`make verify-results` runs the main suite's `installcheck` before `install` ever copies the extension's files into place. Root cause: `TEST_DEPS` textually ends up as `testdeps check-stale-expected test-build install installcheck`, but `check-stale-expected: installcheck` is its own direct dependency edge -- Make resolves that early (2nd in the list), running the main suite against a not-yet-installed extension, well before it ever reaches the literal `install installcheck` pair at the end. `test-build` avoids this by luck, not shared design: it declares its own `test-build: install`, which protects only itself. Filed as Postgres-Extensions/pgxntool-test#62 (not fixed in the vendored copy here). Also corrects the record on PR #23's similar-looking fix: that one attributed the same symptom to "ambient parallel make" from pg-build-test's `MAKEFLAGS=-j $(nproc)` export -- confirmed here, with a plain serial `make -n` dry run (no -j anywhere), that the real cause is this TEST_DEPS ordering issue instead. The fix (explicit `make install` first) happened to be correct either way. Reproduced 3/3 times with the extension deliberately removed from /usr/share/postgresql/*/extension first, and confirmed fixed 3/3 times, on both PG12 and PG17, with PG_CONFIG correctly pinned throughout this time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…change Rebasing onto sync-pgxntool-2.3.0 (built on current upstream master) pulls in upstream PR #20 ("Drop the original_role temp table from install"), which this branch's fork-based master predated. That commit only touched sql/*.sql, not test/, but it shifts every subsequent line of the auto-generated sql/test_factory--0.5.0.sql by a constant offset -- and test/build/syntax.sql's expected output embeds literal line numbers from psql's \i output. Regenerated from a real run (test/build has no `make results` equivalent to gate this the way test/expected/ does); confirmed the diff is a pure line-number shift, not a content change: verified locally that a deliberately broken test/build syntax error is still caught immediately and clearly.
pgxntool 2.3.0 (sync-pgxntool-2.3.0, this branch's new base) adds a real installcheck: install dependency edge to base.mk, fixing the ordering bug this workaround existed for (Postgres-Extensions/pgxntool-test#62). Reproduced the original failure once more on this rebased branch to confirm the fix holds standalone: deleted test_factory's installed files from /usr/share/postgresql/17/extension/, ran the now-simplified `make verify-results` alone with no prior `make install`, and it passed -- where the old base required the explicit install step first.
Same root cause as the syntax.out regeneration two commits back: these files were authored on this branch's original fork-based master, which predates upstream PR #24 (adds the sql-lint tool and its comment-stacked-dashes rule, 3+ consecutive -- lines must use /* */). Rebasing onto sync-pgxntool-2.3.0 makes the linter apply to them for the first time. Watch for the same nested-block-comment hazard PR #22's own comment conversion hit (see commit 6a0627d "Convert multi-line SQL comments to /* */ style"): syntax.sql's original wording included the literal substring "sql/*--*.sql", where the "/*" opens a second, unwanted nested comment (Postgres block comments nest) -- reworded to avoid any "/*"/"*/" substrings appearing inside comment bodies, not just at the intended block boundaries.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7 tasks
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.
Rebase note
Rebased onto
sync-pgxntool-2.3.0(bottom of a 4-branch stack restructuring this PR together with #22/#23; coordinator is handlinggh stack linkseparately). Two things fell out of the rebase:make installworkaround this branch's CI added beforemake verify-results-- pgxntool 2.3.0 (the new base) fixes the underlyinginstallcheck-before-installordering bug upstream (Postgres-Extensions/pgxntool-test#62), soverify-results's own dependency chain now guarantees the ordering itself. Re-reproduced the original bug on this rebased branch before removing the workaround (cleared/usr/share/postgresql/17/extension/'s test_factory files, ran the simplified step alone) to confirm the fix holds standalone.test/build/expected/syntax.out: the new base (built on current upstreammaster) already includes upstream PR Drop the original_role temp table from install (#12/#13) #20 ("Drop the original_role temp table from install"), which this branch's original fork-basedmasterpredated. That commit shifts every line of the auto-generatedsql/test_factory--0.5.0.sqlby a constant offset, andsyntax.sql's expected output embeds literal\iline numbers. Confirmed the diff was a pure line-number shift, not a content regression, before regenerating from a real run.Summary
pgxntool (vendored at 2.2.0, now 2.3.0 via
sync-pgxntool-2.3.0) has had aPGXNTOOL_ENABLE_TEST_BUILDfeature for a while, but this repo never had atest/builddirectory. Quotingpgxntool/base.mk's own purpose comment for that feature:This PR addresses a separate, pre-existing gap; it's now the second branch in the stack (base:
sync-pgxntool-2.3.0), independent in content from #22/#23.1. Move install-mechanics tests into
test/build/install.sqltest/sql/install.sqland the opening block oftest/sql/pgtap.sqltested extension packaging (dependency declarations, clean install/uninstall) rather than test_factory's actual business logic (registering/getting test data). That's exactly whattest/buildis for: plain SQL + pg_regress's classic expected-output diffing, in an isolated database with none of the main suite's pgTAP/role/schedule machinery.test/sql/install.sqlis deleted (nothing left to test there once this moved), and the leading dependency-check block intest/sql/pgtap.sqlis trimmed (the rest of that file — the part that actually exercisestf.tap()— is untouched).2. Add
test/build/syntax.sql: run the raw versioned SQL files directlyPer the repo owner's request: "add simple tests there that just try to run the .sql files. Such tests make it MUCH easier to debug something like a syntax problem in the code, since CREATE EXTENSION tends to completely obscure the real error." This
\i'ssql/test_factory--0.5.0.sqlandsql/test_factory_pgtap--0.1.0.sqldirectly (not viaCREATE EXTENSION).Known/expected errors baked into its expected output (documented in the file itself, so a future reader isn't confused about which diffs are "expected" vs "something's actually broken"):
pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSIONerrors — that function always errors when its containing script is\i'd directly instead of run viaCREATE/ALTER EXTENSION, regardless of whether the SQL is otherwise valid.SET ROLE ""/ "zero-length delimited identifier" error near the end of each file — both files save the caller's role viapg_catalog.set_config(..., true)(transaction-scoped) so they can restore it at the end. A realCREATE EXTENSIONruns its whole script as one transaction, so that works; run via plain\i(autocommit), each statement is its own implicit transaction, so the GUC's scope ends immediately andcurrent_setting()returns''by the time the restore runs. We deliberately do not wrap the\iin an explicitBEGIN/COMMITto "fix" this — verified locally that doing so makes the first error poison the whole transaction, aborting everything after it instead of just these two harmless statements.Verified locally (both PG12 and PG17) that a deliberately-introduced syntax typo is immediately and clearly surfaced by this test (
syntax error at or near "TYPPE"at the exact line), then reverted.Also had to dial
VERBOSITYdown frompsql.sql'sverbosetodefaultin both new files — verbose mode appends backend C sourceLOCATIONlines that differ across PG major versions/builds, which broke matching a single expected-output file across this repo's PG12–17 matrix.3. Wire it up
PGXNTOOL_ENABLE_TEST_BUILD = yesexplicit in the Makefile (avoids silently disabling the check iftest/build/'s contents were ever accidentally deleted).pg-build-testnever callsmake test, sotest-build(wired only intotest's dependency chain) would never run in CI. Replaced with a directmake verify-resultsstep (see rebase note above for the latest simplification of that step).Follow-up needed
Landing this means #22 will need a follow-up rebase to remove now-redundant mode-awareness it added to
install.sql/pgtap.sql(not doing that here — just flagging it; being handled as part of the same stack restructuring).Test plan
make testpasses on PG17 (including the newtest-buildstep)make testpasses on PG12 (including the newtest-buildstep)sql/test_factory--0.5.0.sql(typo'dCREATE TYPE→CREATE TYPPE) and confirmed bothtest/build/install.sqlandtest/build/syntax.sqlsurfaced it clearly and immediately; reverted before committingtest/build/expected/*.outgenerated from real runs (never hand-authored)test/build/expected/syntax.out's regenerated diff was a pure line-number shift, not a content regression🤖 Generated with Claude Code
Recreated from #26 (fork-headed) as an upstream-branch PR so it can be part of a formal GitHub stack (
gh stack link), which refuses fork PRs. Same content/commits, same CI results.