From fb32a4971d90d82f0f0c01265a084f2df98bbc10 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 18:02:30 -0500 Subject: [PATCH 1/5] Add test/build: extension-metadata tests + raw SQL syntax checks 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 --- .github/workflows/ci.yml | 10 +++++ Makefile | 5 +++ test/CLAUDE.md | 21 +++++++-- test/build/expected/install.out | 8 ++++ test/build/expected/syntax.out | 30 +++++++++++++ test/build/install.sql | 77 +++++++++++++++++++++++++++++++++ test/build/syntax.sql | 69 +++++++++++++++++++++++++++++ test/expected/install.out | 9 ---- test/expected/pgtap.out | 25 +++++------ test/helpers/tap_setup.sql | 4 -- test/sql/install.sql | 30 ------------- test/sql/pgtap.sql | 9 ---- 12 files changed, 229 insertions(+), 68 deletions(-) create mode 100644 test/build/expected/install.out create mode 100644 test/build/expected/syntax.out create mode 100644 test/build/install.sql create mode 100644 test/build/syntax.sql delete mode 100644 test/expected/install.out delete mode 100644 test/sql/install.sql diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d4aaec9..44f8d8c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,3 +15,13 @@ jobs: uses: actions/checkout@v4 - name: Test on PostgreSQL ${{ matrix.pg }} run: pg-build-test + # pg-build-test (from pgxn-tools) runs `make all; sudo make install; + # make installcheck` -- it never invokes the `test` target, so + # pgxntool's test-build feature (wired only into `test`'s dependency + # chain, not `installcheck`) would otherwise never run in CI even + # though `PGXNTOOL_ENABLE_TEST_BUILD = yes` is set in the Makefile. + # Run it explicitly here so a SQL syntax error or a packaging/install + # regression actually fails CI, instead of only being caught by a + # developer who happens to run `make test` locally. + - name: Verify test/build (SQL syntax + install mechanics) + run: make test-build diff --git a/Makefile b/Makefile index 9bc4fd6..bc29d40 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,10 @@ include pgxntool/base.mk +# Explicit rather than relying on base.mk's auto-detection of test/build/*.sql +# files, so an accidental deletion of test/build/'s contents is a loud error +# instead of silently disabling this check. +PGXNTOOL_ENABLE_TEST_BUILD = yes + # Hook for test to ensure dependencies in control file are set correctly testdeps: check_control diff --git a/test/CLAUDE.md b/test/CLAUDE.md index ac12354..12ea604 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -10,8 +10,14 @@ The test_factory extension uses **pgTAP** (PostgreSQL's unit testing framework) ### Test Files - `test/sql/base.sql` - Core functionality tests (22 tests) -- `test/sql/install.sql` - Extension installation/uninstallation tests - `test/sql/pgtap.sql` - pgTAP integration and `tf.tap()` function tests +- `test/build/install.sql` - Extension packaging/install-mechanics checks + (dependency declarations, clean install/uninstall). Runs via pgxntool's + `test/build` feature (plain SQL + pg_regress diffing, no pgTAP) in an + isolated database, separate from the main pgTAP-based suite above. +- `test/build/syntax.sql` - Runs the raw, generated versioned SQL install + scripts (`sql/*--*.sql`) directly via `\i`, to catch SQL syntax errors with + a clearer error than a `CREATE EXTENSION` failure would give. ### Expected Results - `test/expected/*.out` - Expected test output for regression testing @@ -41,15 +47,24 @@ The test_factory extension uses **pgTAP** (PostgreSQL's unit testing framework) - **Permission Isolation** - Tests with unprivileged `test_role` - **Temp Table Cleanup** - Verifies temporary installation objects are removed -### Installation Tests (`install.sql`) +### Installation/Packaging Tests (`test/build/install.sql`) - **Dependency Validation** - Tests extension dependency requirements - **Clean Installation** - Tests CREATE EXTENSION without conflicts - **Clean Removal** - Tests DROP EXTENSION without orphaned objects +- Runs via pgxntool's `test/build` feature (plain SQL/pg_regress diffing, + not pgTAP), in an isolated database separate from the main suite below. + +### Raw SQL Syntax Tests (`test/build/syntax.sql`) +- Runs `sql/test_factory--*.sql` and `sql/test_factory_pgtap--*.sql` directly + via `\i` (not `CREATE EXTENSION`), so a genuine syntax error is reported + clearly instead of being obscured by a generic CREATE EXTENSION failure. +- See the comments in that file for the known/expected errors baked into its + expected output (`pg_extension_config_dump()` and `SET ROLE ""`), which are + artifacts of running the file outside of CREATE EXTENSION, not bugs. ### pgTAP Integration Tests (`pgtap.sql`) - **tf.tap() Function** - Tests pgTAP wrapper functionality - **Error Handling** - Tests proper error reporting for invalid inputs -- **Extension Dependencies** - Validates test_factory_pgtap requires test_factory ## Test Data Model diff --git a/test/build/expected/install.out b/test/build/expected/install.out new file mode 100644 index 0000000..dce592a --- /dev/null +++ b/test/build/expected/install.out @@ -0,0 +1,8 @@ +\set ECHO none +ERROR: required extension "test_factory" is not installed +HINT: Use CREATE EXTENSION ... CASCADE to install required extensions too. + regprocedure +------------------- + tf.tap(text,text) +(1 row) + diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out new file mode 100644 index 0000000..9bb1e4a --- /dev/null +++ b/test/build/expected/syntax.out @@ -0,0 +1,30 @@ +\set ECHO none +NOTICE: extension "test_factory_pgtap" does not exist, skipping +NOTICE: extension "test_factory" does not exist, skipping + set_config +------------ + root +(1 row) + +psql:sql/test_factory--0.5.0.sql:42: WARNING: SET LOCAL can only be used in transaction blocks +psql:sql/test_factory--0.5.0.sql:56: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory--0.5.0.sql:57: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory--0.5.0.sql:95: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory--0.5.0.sql:117: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory--0.5.0.sql:123: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory--0.5.0.sql:262: ERROR: zero-length delimited identifier at or near """" +LINE 1: SET ROLE "" + ^ +QUERY: SET ROLE "" +CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE + set_config +------------ + root +(1 row) + +psql:sql/test_factory_pgtap--0.1.0.sql:10: WARNING: SET LOCAL can only be used in transaction blocks +psql:sql/test_factory_pgtap--0.1.0.sql:39: ERROR: zero-length delimited identifier at or near """" +LINE 1: SET ROLE "" + ^ +QUERY: SET ROLE "" +CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE diff --git a/test/build/install.sql b/test/build/install.sql new file mode 100644 index 0000000..11eba23 --- /dev/null +++ b/test/build/install.sql @@ -0,0 +1,77 @@ +\set ECHO none +\i test/helpers/psql.sql + +/* + * Extension packaging/install-mechanics checks. + * + * These verify that the extension *packaging* is correct (control file + * dependency declarations, clean install/uninstall) -- not test_factory's + * actual business logic (registering/getting test data; that's covered by + * test/sql/base.sql and test/sql/pgtap.sql). This lives in test/build + * because that's exactly what pgxntool's test/build feature is for. Quoting + * pgxntool/base.mk's own comment on the purpose of test/build: + * + * 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. + * + * test/build runs in an isolated database with none of the main suite's + * test/helpers/setup.sql machinery (no pgTAP, no roles, no --dbname), so + * this file uses plain SQL and lets pg_regress's classic expected-output + * diffing do the comparison instead of pgTAP assertions. + * + * The one intentional error below (the bare CREATE EXTENSION) is expected to + * abort only its own statement, not the rest of this script, since psql runs + * each top-level statement as its own autocommit transaction by default. We + * still disable ON_ERROR_STOP (which test/helpers/psql.sql just turned on) + * to make that reliance explicit rather than depending on pg_regress's own + * default. + * + * We also dial VERBOSITY back down from psql.sql's "verbose" to "default": + * verbose mode appends a LOCATION line with the triggering backend C source + * file/line for every error, which differs across PG major versions (and + * even between builds of the same version) -- fine for interactive + * debugging, but it would make this file's expected output impossible to + * match across the PG12/PG17 matrix this repo tests against. + */ +\set ON_ERROR_STOP false +\set VERBOSITY default + +SET client_min_messages = WARNING; + +-- pgtap is a declared dependency of test_factory_pgtap too. Install it up +-- front so the bare-create failure below isolates specifically on +-- test_factory being missing, not on pgtap also being missing (test/build's +-- database doesn't have pgtap pre-installed the way the main suite's +-- test/helpers/setup.sql arranges). +CREATE EXTENSION IF NOT EXISTS pgtap; + +-- Start from a clean slate. Non-CASCADE so we'd notice if a previous step in +-- this file (or a run before it) left something installed unexpectedly. +DROP EXTENSION IF EXISTS test_factory_pgtap; +DROP EXTENSION IF EXISTS test_factory; + +/* + * A bare (non-CASCADE) CREATE EXTENSION for test_factory_pgtap must fail, + * since test_factory isn't installed. This proves + * test_factory_pgtap.control's "requires = 'pgtap, test_factory'" line is + * real and enforced by Postgres, not just documentation. + * IF YOU GET A "schema tf does not exist" ERROR HERE INSTEAD (i.e. the + * CREATE EXTENSION below succeeds), the dependency declaration is + * missing/broken -- check test_factory_pgtap.control. + */ +CREATE EXTENSION test_factory_pgtap; + +-- CASCADE should pull test_factory in automatically. +CREATE EXTENSION test_factory_pgtap CASCADE; + +-- Confirm tf.tap(text, text) exists after install. Casting to regprocedure +-- errors out (visibly, in the diff) if the function doesn't exist. +SELECT 'tf.tap(text, text)'::regprocedure; + +-- Clean removal: a bare (non-CASCADE) DROP must not error, proving nothing +-- else was left depending on either extension. +DROP EXTENSION test_factory_pgtap; +DROP EXTENSION test_factory; + +-- vi: expandtab ts=2 sw=2 diff --git a/test/build/syntax.sql b/test/build/syntax.sql new file mode 100644 index 0000000..83b8f7d --- /dev/null +++ b/test/build/syntax.sql @@ -0,0 +1,69 @@ +\set ECHO none +\i test/helpers/psql.sql + +/* + * Run the raw, generated versioned SQL install scripts directly (\i, NOT + * CREATE EXTENSION) against a fresh database. This is a deliberately simple + * test: its whole point is that a genuine SQL syntax error anywhere in + * either file shows up immediately and clearly, instead of being obscured + * behind a generic CREATE EXTENSION failure the way test/build/install.sql + * (or a normal `make install`) would report it. + * + * KNOWN / EXPECTED ERRORS BAKED INTO THIS TEST'S EXPECTED OUTPUT + * (verified locally; these are not bugs -- see explanation before each + * occurrence below): + * + * 1. Two "pg_extension_config_dump() can only be called from an SQL script + * executed by CREATE EXTENSION" errors (both from test_factory's file). + * That function always errors when its containing script is \i'd + * directly rather than run by CREATE/ALTER EXTENSION, regardless of + * whether the file's SQL is otherwise valid. + * + * 2. One "zero-length delimited identifier" error from `SET ROLE ""` near + * the end of EACH file. Both files save the caller's role in a GUC via + * `pg_catalog.set_config(..., true)` (the trailing `true` = is_local, + * i.e. transaction-scoped) so they can restore it before the script + * ends. A real CREATE EXTENSION runs its whole script as one transaction, + * so that works fine there. Run via plain \i in autocommit (no enclosing + * transaction), each top-level statement is its own implicit + * transaction, so the GUC's scope ends immediately -- by the time the + * restore-role code runs, current_setting() returns an empty string, and + * `SET ROLE ""` fails this way instead. + * + * We deliberately do NOT wrap the \i calls below in an explicit + * BEGIN/COMMIT to "fix" the above -- that would make the FIRST + * pg_extension_config_dump error poison the whole transaction, aborting + * every statement after it instead of just these known, harmless ones + * (verified locally: with an explicit transaction, every subsequent + * statement fails with "current transaction is aborted", which would hide + * real syntax errors instead of surfacing them). Relatedly, we disable + * ON_ERROR_STOP below (which test/helpers/psql.sql just turned on) so that + * these known errors don't stop psql from processing the rest of either + * file -- each statement already being its own autocommit transaction means + * a REAL syntax error later in either file still shows up as its own clear + * diff, it just doesn't halt the whole test. + * + * We also dial VERBOSITY back down from psql.sql's "verbose" to "default": + * verbose mode appends a LOCATION line with the triggering backend C source + * file/line for every error above, which differs across PG major versions + * (and even between builds of the same version) -- it would make this + * file's expected output impossible to match across the PG12/PG17 matrix + * this repo tests against. + */ +\set ON_ERROR_STOP false +\set VERBOSITY default + +-- Guard against test/build/install.sql (or a previous run) having left +-- either extension installed; keep this file runnable on its own regardless +-- of test-build's file-execution order. +DROP EXTENSION IF EXISTS test_factory_pgtap; +DROP EXTENSION IF EXISTS test_factory; + +-- test_factory must run first: test_factory_pgtap's raw file creates +-- objects in the "tf" schema, owned by the test_factory__owner role, both of +-- which only exist once test_factory's file has run. +\i sql/test_factory--0.5.0.sql + +\i sql/test_factory_pgtap--0.1.0.sql + +-- vi: expandtab ts=2 sw=2 diff --git a/test/expected/install.out b/test/expected/install.out deleted file mode 100644 index ae61611..0000000 --- a/test/expected/install.out +++ /dev/null @@ -1,9 +0,0 @@ -\set ECHO none -ok 1 - drop extension test_factory_pgtap -ok 2 - drop extension test_factory -ok 3 - Extension test_factory should not exist -ok 4 - Extension test_factory_pgtap should not exist -ok 5 - create extension -ok 6 - Function tf.tap(text, text) should exist -ok 7 - clean-up test_factory_pgtap -ok 8 - clean-up test_factory diff --git a/test/expected/pgtap.out b/test/expected/pgtap.out index b2b7706..5f552dc 100644 --- a/test/expected/pgtap.out +++ b/test/expected/pgtap.out @@ -1,17 +1,16 @@ \set ECHO none -ok 1 - Ensure test_factory is a dependency of test_factory_pgtap Creating extension test_factory Creating extension test_factory_pgtap -ok 2 - Register test customers -ok 3 - Create function customer__add -ok 4 - Register test invoices -ok 5 - Ensure original_role temp table was dropped -ok 6 - Ensure role is put back after install -ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 10 - Security definer function _tf.table_create has search_path=pg_catalog -ok 11 - Security definer function _tf.get has search_path=pg_catalog +ok 1 - Register test customers +ok 2 - Create function customer__add +ok 3 - Register test invoices +ok 4 - Ensure original_role temp table was dropped +ok 5 - Ensure role is put back after install +ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 7 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 8 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 9 - Security definer function _tf.table_create has search_path=pg_catalog +ok 10 - Security definer function _tf.get has search_path=pg_catalog +ok 11 - Get test data set "base" for table invoice ok 12 - Get test data set "base" for table invoice -ok 13 - Get test data set "base" for table invoice -ok 14 - Ensure we get sane error for a non-existent table +ok 13 - Ensure we get sane error for a non-existent table diff --git a/test/helpers/tap_setup.sql b/test/helpers/tap_setup.sql index c127096..b90844c 100644 --- a/test/helpers/tap_setup.sql +++ b/test/helpers/tap_setup.sql @@ -1,9 +1,5 @@ \i test/helpers/psql.sql -/* - * NOTE: if you get errors about things already existing it's because they've - * been left behind by test/sql/install.sql - */ SET client_min_messages = WARNING; CREATE SCHEMA IF NOT EXISTS tap; SET search_path = tap; diff --git a/test/sql/install.sql b/test/sql/install.sql deleted file mode 100644 index ae58bf3..0000000 --- a/test/sql/install.sql +++ /dev/null @@ -1,30 +0,0 @@ -\set ECHO none -\i test/helpers/setup.sql - -SET client_min_messages = WARNING; - -/* - * DO NOT use CASCADE here; we want this to fail if there's anything installed - * that depends on it. - */ -SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'drop extension test_factory_pgtap'); -SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'drop extension test_factory'); - -SELECT hasnt_extension( 'test_factory' ); -SELECT hasnt_extension( 'test_factory_pgtap' ); - -SELECT lives_ok($$CREATE EXTENSION test_factory_pgtap CASCADE$$, 'create extension'); -COMMIT; - -SELECT has_function('tf', 'tap', array['text','text']); - --- Cleanup -SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'clean-up test_factory_pgtap'); -SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'clean-up test_factory'); - -/* - * Arguably we should cleanup pgtap and the tap schema... - */ - --- vi: expandtab ts=2 sw=2 - diff --git a/test/sql/pgtap.sql b/test/sql/pgtap.sql index ba0c4b3..2abbeb6 100644 --- a/test/sql/pgtap.sql +++ b/test/sql/pgtap.sql @@ -1,15 +1,6 @@ \set ECHO none \i test/helpers/setup.sql -SET search_path = tap; --- IF YOU GET A "schema tf does not exist" error here then the dependency is missing! -SELECT throws_ok( - $$CREATE EXTENSION test_factory_pgtap$$ - , '42704' - , 'required extension "test_factory" is not installed' - , 'Ensure test_factory is a dependency of test_factory_pgtap' -); - \set extension_name test_factory \i test/helpers/create_extension.sql DROP TABLE pre_install_role; From 560eed3b067b354f46db8b7048dc57f66bd35f8f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 18:05:26 -0500 Subject: [PATCH 2/5] Install rsync before make test-build in CI 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 --- .github/workflows/ci.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 44f8d8c..3b17288 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,4 +24,12 @@ jobs: # regression actually fails CI, instead of only being caught by a # developer who happens to run `make test` locally. - name: Verify test/build (SQL syntax + install mechanics) - run: make test-build + # pgxntool/run-test-build.sh (vendored, not this repo's own script) + # shells out to rsync to sync test/build/*.sql into test/build/sql/ -- + # the pgxn/pgxn-tools image doesn't ship rsync, so install it first. + # Any pgxntool project enabling PGXNTOOL_ENABLE_TEST_BUILD on this + # image would hit the same gap; worth a pgxntool-test issue, not + # something to work around inside the vendored script itself. + run: | + apt-get install -y rsync + make test-build From 4d2e7c673e3794acea878ad07212612085ca0672 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 18:09:26 -0500 Subject: [PATCH 3/5] Fix environment-dependent expected output in test/build/syntax.sql 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 --- test/build/expected/syntax.out | 10 ---------- test/build/syntax.sql | 12 ++++++++++++ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index 9bb1e4a..c5024bb 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,11 +1,6 @@ \set ECHO none NOTICE: extension "test_factory_pgtap" does not exist, skipping NOTICE: extension "test_factory" does not exist, skipping - set_config ------------- - root -(1 row) - psql:sql/test_factory--0.5.0.sql:42: WARNING: SET LOCAL can only be used in transaction blocks psql:sql/test_factory--0.5.0.sql:56: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION psql:sql/test_factory--0.5.0.sql:57: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION @@ -17,11 +12,6 @@ LINE 1: SET ROLE "" ^ QUERY: SET ROLE "" CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE - set_config ------------- - root -(1 row) - psql:sql/test_factory_pgtap--0.1.0.sql:10: WARNING: SET LOCAL can only be used in transaction blocks psql:sql/test_factory_pgtap--0.1.0.sql:39: ERROR: zero-length delimited identifier at or near """" LINE 1: SET ROLE "" diff --git a/test/build/syntax.sql b/test/build/syntax.sql index 83b8f7d..e960fbd 100644 --- a/test/build/syntax.sql +++ b/test/build/syntax.sql @@ -49,9 +49,21 @@ * (and even between builds of the same version) -- it would make this * file's expected output impossible to match across the PG12/PG17 matrix * this repo tests against. + * + * Both files also print a row of query output 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, which is environment-dependent (this repo's own local + * testing connects as a different OS role than CI does) -- caught for real + * when this file's expected output, generated locally, didn't match CI. + * `\o /dev/null` discards normal query-result output for the rest of this + * file; NOTICE/WARNING/ERROR go to a separate stream psql doesn't route + * through `\o`, so every error this test actually cares about (including a + * genuine future syntax error) still shows up in the diff untouched. */ \set ON_ERROR_STOP false \set VERBOSITY default +\o /dev/null -- Guard against test/build/install.sql (or a previous run) having left -- either extension installed; keep this file runnable on its own regardless From 95d8be4fee6612fa8c27325c13853ebe399aa160 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 31 Jul 2026 15:06:25 -0500 Subject: [PATCH 4/5] Drop pg-build-test in CI, fix syntax.sql's mess/portability issues, trim 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 --- .github/workflows/ci.yml | 39 ++++++++------- CLAUDE.md | 2 + test/build/expected/syntax.out | 14 ------ test/build/install.sql | 77 +++++++++------------------- test/build/syntax.sql | 91 ++++++++-------------------------- 5 files changed, 68 insertions(+), 155 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3b17288..86e0304 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,23 +13,24 @@ jobs: run: pg-start ${{ matrix.pg }} - name: Check out the repo uses: actions/checkout@v4 + # Not pgxn-tools' own `pg-build-test` helper: it never invokes `make + # test` (it runs `make all; sudo make install; make installcheck` + # directly -- confirmed from its actual source, pgxn/docker-pgxn-tools + # bin/pg-build-test), so it would never reach test-build (wired in only + # via `test`'s own dependency chain). Worse, its 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). `make verify-results` inspects the actual + # TAP output instead of trusting an exit code, and pulls in test-build + # as part of the same `test` dependency chain it already runs. + # + # rsync first: pgxntool/run-test-build.sh (vendored, not this repo's + # own script) needs it to sync test/build/*.sql into test/build/sql/, + # and the pgxn/pgxn-tools image doesn't ship it. Any pgxntool project + # enabling PGXNTOOL_ENABLE_TEST_BUILD on this image would hit the same + # gap -- worth a pgxntool-test issue, not a vendored-script edit here. + - name: Install rsync + run: apt-get install -y rsync - name: Test on PostgreSQL ${{ matrix.pg }} - run: pg-build-test - # pg-build-test (from pgxn-tools) runs `make all; sudo make install; - # make installcheck` -- it never invokes the `test` target, so - # pgxntool's test-build feature (wired only into `test`'s dependency - # chain, not `installcheck`) would otherwise never run in CI even - # though `PGXNTOOL_ENABLE_TEST_BUILD = yes` is set in the Makefile. - # Run it explicitly here so a SQL syntax error or a packaging/install - # regression actually fails CI, instead of only being caught by a - # developer who happens to run `make test` locally. - - name: Verify test/build (SQL syntax + install mechanics) - # pgxntool/run-test-build.sh (vendored, not this repo's own script) - # shells out to rsync to sync test/build/*.sql into test/build/sql/ -- - # the pgxn/pgxn-tools image doesn't ship rsync, so install it first. - # Any pgxntool project enabling PGXNTOOL_ENABLE_TEST_BUILD on this - # image would hit the same gap; worth a pgxntool-test issue, not - # something to work around inside the vendored script itself. - run: | - apt-get install -y rsync - make test-build + run: make verify-results diff --git a/CLAUDE.md b/CLAUDE.md index 33b198c..c476d0d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,6 +10,8 @@ After pushing to a branch with an open PR, monitor CI using `gh pr checks --watc This is **test_factory**, a PostgreSQL extension that provides a framework for managing unit test data in databases. It solves the common problem of creating and maintaining test data by providing a system to register test data definitions once and retrieve them efficiently with automatic dependency resolution. +This PGXN distribution ships **two extensions**: `test_factory` (the core framework) and `test_factory_pgtap` (a thin pgTAP integration wrapper, `tf.tap()`, depending on `test_factory`). Because they're structurally so similar (same install/security-definer/role-restoration patterns, same packaging concerns), the test suite shares a lot of common infrastructure between them (`test/helpers/*`, `test/build/*`) rather than duplicating it per extension -- keep that in mind when adding tests for one and wondering whether the other needs the same treatment. + ## Build System & Development Commands This project uses PGXNtool for build management. Key commands: diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index c5024bb..ac4404c 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,20 +1,6 @@ \set ECHO none -NOTICE: extension "test_factory_pgtap" does not exist, skipping -NOTICE: extension "test_factory" does not exist, skipping -psql:sql/test_factory--0.5.0.sql:42: WARNING: SET LOCAL can only be used in transaction blocks psql:sql/test_factory--0.5.0.sql:56: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION psql:sql/test_factory--0.5.0.sql:57: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION psql:sql/test_factory--0.5.0.sql:95: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text psql:sql/test_factory--0.5.0.sql:117: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text psql:sql/test_factory--0.5.0.sql:123: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text -psql:sql/test_factory--0.5.0.sql:262: ERROR: zero-length delimited identifier at or near """" -LINE 1: SET ROLE "" - ^ -QUERY: SET ROLE "" -CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE -psql:sql/test_factory_pgtap--0.1.0.sql:10: WARNING: SET LOCAL can only be used in transaction blocks -psql:sql/test_factory_pgtap--0.1.0.sql:39: ERROR: zero-length delimited identifier at or near """" -LINE 1: SET ROLE "" - ^ -QUERY: SET ROLE "" -CONTEXT: PL/pgSQL function inline_code_block line 3 at EXECUTE diff --git a/test/build/install.sql b/test/build/install.sql index 11eba23..88e38e5 100644 --- a/test/build/install.sql +++ b/test/build/install.sql @@ -1,76 +1,47 @@ \set ECHO none \i test/helpers/psql.sql -/* - * Extension packaging/install-mechanics checks. - * - * These verify that the extension *packaging* is correct (control file - * dependency declarations, clean install/uninstall) -- not test_factory's - * actual business logic (registering/getting test data; that's covered by - * test/sql/base.sql and test/sql/pgtap.sql). This lives in test/build - * because that's exactly what pgxntool's test/build feature is for. Quoting - * pgxntool/base.mk's own comment on the purpose of test/build: - * - * 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. - * - * test/build runs in an isolated database with none of the main suite's - * test/helpers/setup.sql machinery (no pgTAP, no roles, no --dbname), so - * this file uses plain SQL and lets pg_regress's classic expected-output - * diffing do the comparison instead of pgTAP assertions. - * - * The one intentional error below (the bare CREATE EXTENSION) is expected to - * abort only its own statement, not the rest of this script, since psql runs - * each top-level statement as its own autocommit transaction by default. We - * still disable ON_ERROR_STOP (which test/helpers/psql.sql just turned on) - * to make that reliance explicit rather than depending on pg_regress's own - * default. - * - * We also dial VERBOSITY back down from psql.sql's "verbose" to "default": - * verbose mode appends a LOCATION line with the triggering backend C source - * file/line for every error, which differs across PG major versions (and - * even between builds of the same version) -- fine for interactive - * debugging, but it would make this file's expected output impossible to - * match across the PG12/PG17 matrix this repo tests against. - */ +-- Extension packaging checks (dependency declarations, clean +-- install/uninstall) -- not test_factory's business logic, which is +-- test/sql/base.sql and test/sql/pgtap.sql's job. Compare +-- test/build/syntax.sql: that one runs the raw SQL files directly to catch +-- syntax errors; this one goes through a real CREATE EXTENSION to catch +-- packaging errors instead. +-- +-- test/build runs in an isolated database with none of the main suite's +-- pgTAP/role setup, so this is plain SQL, relying on pg_regress's classic +-- expected-output diffing. ON_ERROR_STOP is off since the bare CREATE +-- EXTENSION below is expected to fail (each top-level statement is its own +-- autocommit transaction, so that doesn't touch anything after it). +-- VERBOSITY is knocked down from psql.sql's "verbose" to "default" since the +-- verbose LOCATION line differs across PG majors. \set ON_ERROR_STOP false \set VERBOSITY default SET client_min_messages = WARNING; --- pgtap is a declared dependency of test_factory_pgtap too. Install it up --- front so the bare-create failure below isolates specifically on --- test_factory being missing, not on pgtap also being missing (test/build's --- database doesn't have pgtap pre-installed the way the main suite's --- test/helpers/setup.sql arranges). +-- pgtap is test_factory_pgtap's other declared dependency; install it first +-- so the bare-create failure below isolates on test_factory specifically. CREATE EXTENSION IF NOT EXISTS pgtap; --- Start from a clean slate. Non-CASCADE so we'd notice if a previous step in --- this file (or a run before it) left something installed unexpectedly. +-- Clean slate; non-CASCADE so we'd notice if something was already there. DROP EXTENSION IF EXISTS test_factory_pgtap; DROP EXTENSION IF EXISTS test_factory; -/* - * A bare (non-CASCADE) CREATE EXTENSION for test_factory_pgtap must fail, - * since test_factory isn't installed. This proves - * test_factory_pgtap.control's "requires = 'pgtap, test_factory'" line is - * real and enforced by Postgres, not just documentation. - * IF YOU GET A "schema tf does not exist" ERROR HERE INSTEAD (i.e. the - * CREATE EXTENSION below succeeds), the dependency declaration is - * missing/broken -- check test_factory_pgtap.control. - */ +-- A bare (non-CASCADE) create must fail: test_factory isn't installed yet. +-- Proves test_factory_pgtap.control's "requires = 'pgtap, test_factory'" is +-- real and enforced, not just documentation. IF THIS SUCCEEDS INSTEAD (e.g. +-- with a "schema tf does not exist" error later), the dependency +-- declaration is missing/broken -- check test_factory_pgtap.control. CREATE EXTENSION test_factory_pgtap; -- CASCADE should pull test_factory in automatically. CREATE EXTENSION test_factory_pgtap CASCADE; --- Confirm tf.tap(text, text) exists after install. Casting to regprocedure --- errors out (visibly, in the diff) if the function doesn't exist. +-- tf.tap(text, text) should exist after install; the cast errors visibly if not. SELECT 'tf.tap(text, text)'::regprocedure; --- Clean removal: a bare (non-CASCADE) DROP must not error, proving nothing --- else was left depending on either extension. +-- Clean removal: a bare (non-CASCADE) drop must not error. DROP EXTENSION test_factory_pgtap; DROP EXTENSION test_factory; diff --git a/test/build/syntax.sql b/test/build/syntax.sql index e960fbd..889542f 100644 --- a/test/build/syntax.sql +++ b/test/build/syntax.sql @@ -1,81 +1,34 @@ \set ECHO none \i test/helpers/psql.sql -/* - * Run the raw, generated versioned SQL install scripts directly (\i, NOT - * CREATE EXTENSION) against a fresh database. This is a deliberately simple - * test: its whole point is that a genuine SQL syntax error anywhere in - * either file shows up immediately and clearly, instead of being obscured - * behind a generic CREATE EXTENSION failure the way test/build/install.sql - * (or a normal `make install`) would report it. - * - * KNOWN / EXPECTED ERRORS BAKED INTO THIS TEST'S EXPECTED OUTPUT - * (verified locally; these are not bugs -- see explanation before each - * occurrence below): - * - * 1. Two "pg_extension_config_dump() can only be called from an SQL script - * executed by CREATE EXTENSION" errors (both from test_factory's file). - * That function always errors when its containing script is \i'd - * directly rather than run by CREATE/ALTER EXTENSION, regardless of - * whether the file's SQL is otherwise valid. - * - * 2. One "zero-length delimited identifier" error from `SET ROLE ""` near - * the end of EACH file. Both files save the caller's role in a GUC via - * `pg_catalog.set_config(..., true)` (the trailing `true` = is_local, - * i.e. transaction-scoped) so they can restore it before the script - * ends. A real CREATE EXTENSION runs its whole script as one transaction, - * so that works fine there. Run via plain \i in autocommit (no enclosing - * transaction), each top-level statement is its own implicit - * transaction, so the GUC's scope ends immediately -- by the time the - * restore-role code runs, current_setting() returns an empty string, and - * `SET ROLE ""` fails this way instead. - * - * We deliberately do NOT wrap the \i calls below in an explicit - * BEGIN/COMMIT to "fix" the above -- that would make the FIRST - * pg_extension_config_dump error poison the whole transaction, aborting - * every statement after it instead of just these known, harmless ones - * (verified locally: with an explicit transaction, every subsequent - * statement fails with "current transaction is aborted", which would hide - * real syntax errors instead of surfacing them). Relatedly, we disable - * ON_ERROR_STOP below (which test/helpers/psql.sql just turned on) so that - * these known errors don't stop psql from processing the rest of either - * file -- each statement already being its own autocommit transaction means - * a REAL syntax error later in either file still shows up as its own clear - * diff, it just doesn't halt the whole test. - * - * We also dial VERBOSITY back down from psql.sql's "verbose" to "default": - * verbose mode appends a LOCATION line with the triggering backend C source - * file/line for every error above, which differs across PG major versions - * (and even between builds of the same version) -- it would make this - * file's expected output impossible to match across the PG12/PG17 matrix - * this repo tests against. - * - * Both files also print a row of query output 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, which is environment-dependent (this repo's own local - * testing connects as a different OS role than CI does) -- caught for real - * when this file's expected output, generated locally, didn't match CI. - * `\o /dev/null` discards normal query-result output for the rest of this - * file; NOTICE/WARNING/ERROR go to a separate stream psql doesn't route - * through `\o`, so every error this test actually cares about (including a - * genuine future syntax error) still shows up in the diff untouched. - */ +-- Runs sql/*--*.sql directly via \i (not CREATE EXTENSION), so a genuine +-- syntax error is reported clearly instead of hiding behind a generic +-- CREATE EXTENSION failure. Compare test/build/install.sql: that one tests +-- packaging (dependency declarations, clean install/uninstall) via a real +-- CREATE EXTENSION; this one tests raw SQL syntax by deliberately bypassing +-- it. +-- +-- Wrapped in one transaction, rolled back at the end, so nothing persists +-- whether this runs under pg_regress or ad hoc locally. ON_ERROR_ROLLBACK +-- savepoints each statement, so the two expected errors below +-- (pg_extension_config_dump() requires a real CREATE/ALTER EXTENSION +-- context; harmless here) don't abort the rest of the file -- a real syntax +-- error later still shows up on its own. VERBOSITY is knocked down from +-- psql.sql's "verbose" to "default" since the verbose LOCATION line differs +-- across PG majors. \o /dev/null hides the one row of query output each +-- file prints (current_user, which varies by environment) while leaving +-- NOTICE/WARNING/ERROR visible. \set ON_ERROR_STOP false +\set ON_ERROR_ROLLBACK on \set VERBOSITY default \o /dev/null --- Guard against test/build/install.sql (or a previous run) having left --- either extension installed; keep this file runnable on its own regardless --- of test-build's file-execution order. -DROP EXTENSION IF EXISTS test_factory_pgtap; -DROP EXTENSION IF EXISTS test_factory; +BEGIN; --- test_factory must run first: test_factory_pgtap's raw file creates --- objects in the "tf" schema, owned by the test_factory__owner role, both of --- which only exist once test_factory's file has run. +-- test_factory first: test_factory_pgtap's file needs its "tf" schema/role. \i sql/test_factory--0.5.0.sql - \i sql/test_factory_pgtap--0.1.0.sql +ROLLBACK; + -- vi: expandtab ts=2 sw=2 From 9a6cd78d462722a1de1ff9e8b4db03fd4d0c6699 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 31 Jul 2026 15:12:58 -0500 Subject: [PATCH 5/5] Restore explicit make install before make verify-results in CI 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 --- .github/workflows/ci.yml | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86e0304..556ed93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,5 +32,23 @@ jobs: # gap -- worth a pgxntool-test issue, not a vendored-script edit here. - name: Install rsync run: apt-get install -y rsync + # `make install` first, as its own separate invocation, is required on + # a genuinely fresh system (confirmed by deleting test_factory's + # installed files and re-running from scratch): pgxntool's own + # TEST_DEPS order is testdeps, check-stale-expected, test-build, + # install, installcheck -- and check-stale-expected has a direct + # `check-stale-expected: installcheck` dependency of its own, so Make + # satisfies "installcheck" (running the main suite against a + # not-yet-installed extension) to resolve THAT edge long before it + # ever reaches the literal "install installcheck" pair listed later in + # TEST_DEPS. test-build survives this by luck, not design symmetry: it + # separately declares `test-build: install`, which happens to install + # the extension in time for test-build's OWN nested installcheck, but + # does nothing for the main suite's installcheck resolved earlier via + # check-stale-expected. This is a real pgxntool base.mk ordering gap + # worth its own upstream issue -- not something to fix by editing the + # vendored copy here. - name: Test on PostgreSQL ${{ matrix.pg }} - run: make verify-results + run: | + make install + make verify-results