From 5325216144272c2f91485f30a9313ca5caebaae5 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 14:18:32 -0500 Subject: [PATCH 1/3] Add test/install foundation for update+upgrade testing (fresh/update/existing) Implements the load-mode switch, dependency guard, and single-source-of-truth role name from the advanced update+upgrade testing pattern (modeled on Postgres-Extensions/cat_tools PR #16 and what has landed on its master since), scoped to what test_factory can actually exercise today: - test/install/load.sql (pgxntool's PGXNTOOL_ENABLE_TEST_INSTALL) now owns getting the extension to its target state via TEST_LOAD_SOURCE= fresh|update|existing, propagated as GUCs (test_factory.test_load_mode etc), validated at both make-parse-time and read time. - test/helpers/create_extension.sql skips CREATE EXTENSION when load.sql already installed it (update/existing), while keeping fresh mode's original no-IF-NOT-EXISTS behavior (hard error on stale state) unchanged. - test/sql/install.sql and the install-ordering half of test/sql/pgtap.sql are skipped under existing mode: their own non-CASCADE DROP EXTENSION is a deliberate fresh-mode-only test that would otherwise trip the guard. - Dependency guard (existing mode only): a view depending on tf.tap(text,text) blocks a stray non-CASCADE DROP EXTENSION test_factory_pgtap; test_factory itself already has a natural guard for free via test_factory_pgtap's own `requires` clause, which load.sql also proves still holds. - test/roles.sql is now the single source of truth for the test_role name, \i'd via test/helpers/deps.sql and test/install/load.sql. - Alternate expected output (test/expected/{base,install,pgtap}_1.out) for the existing-mode leg, since it legitimately produces different (but equally valid) output -- generated from real `existing`-mode runs, no raw "not ok" TAP lines in either leg. Verified against both PG12 and PG17: fresh (default), TEST_LOAD_SOURCE=update (currently a no-op -- see comment in load.sql for why no CI job drives it yet), and TEST_LOAD_SOURCE=existing against a real pre-populated database (the make test ... --use-existing recipe). Fresh mode's expected output is byte-for-byte unchanged from before this change. Skipped/deferred (see PR description): TEST_SCHEMA (test_factory is non-relocatable with hardcoded schema names, so the ambient-search_path failure mode it protects against can't occur here); an update-path CI job (no second version has ever shipped); the bridge-update/multi-origin machinery from PR #16 (cat_tools-specific technical debt, not applicable). Co-Authored-By: Claude Sonnet 5 --- Makefile | 26 ++++ test/CLAUDE.md | 55 ++++++++- test/expected/base_1.out | 25 ++++ test/expected/pgtap_1.out | 14 +++ test/helpers/create.sql | 18 ++- test/helpers/create_extension.sql | 17 ++- test/helpers/deps.sql | 1 + test/install/load.out | 1 + test/install/load.sql | 193 ++++++++++++++++++++++++++++++ test/roles.sql | 8 ++ test/sql/pgtap.sql | 34 ++++++ 11 files changed, 385 insertions(+), 7 deletions(-) create mode 100644 test/expected/base_1.out create mode 100644 test/expected/pgtap_1.out create mode 100644 test/install/load.out create mode 100644 test/install/load.sql create mode 100644 test/roles.sql diff --git a/Makefile b/Makefile index 515b394..c8787b5 100644 --- a/Makefile +++ b/Makefile @@ -5,6 +5,32 @@ include pgxntool/base.mk # instead of silently disabling this check. PGXNTOOL_ENABLE_TEST_BUILD = yes +PGXNTOOL_ENABLE_TEST_INSTALL = yes + +# ------------------------------------------------------------------------------ +# TEST_LOAD_SOURCE: how test/install/load.sql gets the extension to its +# target state (fresh/update/existing). See test/install/load.sql for what +# each mode actually does. +# ------------------------------------------------------------------------------ +TEST_LOAD_SOURCE ?= fresh +ifeq ($(filter $(TEST_LOAD_SOURCE),fresh update existing),) +$(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)') +endif + +# Only meaningful in 'update' mode; empty TO means "update to current". +TEST_UPDATE_FROM ?= 0.5.0 +TEST_UPDATE_TO ?= + +# Export unconditionally -- load.sql must never treat "absent" as "fresh". +export PGOPTIONS := $(PGOPTIONS) -c test_factory.test_load_mode=$(TEST_LOAD_SOURCE) -c test_factory.test_update_from=$(TEST_UPDATE_FROM) -c test_factory.test_update_to=$(TEST_UPDATE_TO) + +# make test-update: convenience wrapper. Must re-invoke $(MAKE) (not just +# depend on test) so the parse-time TEST_LOAD_SOURCE validation above +# re-evaluates for the child invocation. +.PHONY: test-update +test-update: + $(MAKE) test TEST_LOAD_SOURCE=update + # 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 12ea604..1b07c98 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -26,10 +26,61 @@ The test_factory extension uses **pgTAP** (PostgreSQL's unit testing framework) ### Test Helpers - `test/helpers/setup.sql` - Test environment initialization and pgTAP setup - `test/helpers/create.sql` - Test data registration and security validation -- `test/helpers/create_extension.sql` - Extension creation wrapper -- `test/helpers/deps.sql` - Test dependency management +- `test/helpers/create_extension.sql` - Extension creation wrapper; skips + `CREATE EXTENSION` when `test/install/load.sql` already installed it + (`test_load_mode` is not `fresh`) +- `test/helpers/deps.sql` - Test dependency management (`\i`'s `test/roles.sql`) +- `test/roles.sql` - Single source of truth for test-only role names - Other helper files for role management and pgTAP integration +## Load Modes (`TEST_LOAD_SOURCE`) + +`test/install/load.sql` runs once, committed, before the regular test files +(pgxntool's `PGXNTOOL_ENABLE_TEST_INSTALL` feature), so its state survives +into every test file. `TEST_LOAD_SOURCE` (default `fresh`) picks how the +extension gets to its target state: + +- **fresh** (default) - `load.sql` does nothing extra; `test/sql/*.sql` + install the extension themselves, exactly as before this feature existed. +- **update** - `load.sql` does `CREATE EXTENSION test_factory VERSION + :from` then `ALTER EXTENSION UPDATE` (`TEST_UPDATE_FROM`/`TEST_UPDATE_TO` + make vars). test_factory has only ever shipped one version (0.5.0), so + this is a no-op today -- the mechanism exists for when a second version + ships, but no CI job drives it yet. `make test-update` is a shorthand for + `make test TEST_LOAD_SOURCE=update`. +- **existing** - the extension is already installed (a real `pg_upgrade` + target, or an out-of-band update) -- `load.sql` only asserts it's present + at the current version, plants a dependency guard (see below), and + proves it. `test/sql/install.sql` and the install/dependency-order part of + `test/sql/pgtap.sql` are skipped in this mode (see the `\if :is_existing` + branches in those files) since they'd otherwise defeat the guard by doing + their own from-scratch drop/recreate. + + Run against a real pre-existing install with: + ``` + make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB= \ + EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no + ``` + + `existing` mode legitimately produces different (but equally valid) + output than `fresh`/`update` for `base`/`install`/`pgtap` (skipped + sections, a skipped role-restore check), so it has alternate expected + files: `test/expected/{base,install,pgtap}_1.out` (pg_regress's numbered + alternate-expected-file convention). + +### Dependency Guard + +Planted only in `existing` mode (see `load.sql`): a view in schema +`test_factory_drop_guard` depending on `tf.tap(text,text)` blocks a +non-CASCADE `DROP EXTENSION test_factory_pgtap`. `test_factory` itself +doesn't need an artificial guard -- `test_factory_pgtap`'s own control file +(`requires = 'pgtap, test_factory'`) already blocks a non-CASCADE +`DROP EXTENSION test_factory` as long as `test_factory_pgtap` is installed; +`load.sql` proves that natural protection too. The point of the guard: in +`existing` mode, nothing else stops a stray drop (or a logic bug that falls +through to the fresh/update branch) from silently destroying the real +upgraded/updated objects this mode exists to test. + ## Test Coverage Analysis ### Core Functionality Tests (`base.sql`) diff --git a/test/expected/base_1.out b/test/expected/base_1.out new file mode 100644 index 0000000..0076bd0 --- /dev/null +++ b/test/expected/base_1.out @@ -0,0 +1,25 @@ +\set ECHO none +Creating extension test_factory +test_factory already installed -- skipping CREATE EXTENSION (test_load_mode is not fresh) +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 # SKIP role-restore check only applies when this session ran CREATE EXTENSION itself (test_load_mode=fresh) +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 - customer table is empty +ok 12 - invoice table is empty +ok 13 - invoice factory output +ok 14 - invoice table content +ok 15 - customer table content +ok 16 - invoice factory second call +ok 17 - invoice table content stayed constant +ok 18 - customer table content stayed constant +ok 19 - Test function factory +ok 20 - customer table has new row +ok 21 - truncate invoice +ok 22 - invoice factory get remains the same after truncate diff --git a/test/expected/pgtap_1.out b/test/expected/pgtap_1.out new file mode 100644 index 0000000..e7c2b20 --- /dev/null +++ b/test/expected/pgtap_1.out @@ -0,0 +1,14 @@ +\set ECHO none +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 # SKIP role-restore check only applies when this session ran CREATE EXTENSION itself (test_load_mode=fresh) +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 - Ensure we get sane error for a non-existent table diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 9897f75..f320e1f 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -1,13 +1,15 @@ SET ROLE = DEFAULT; -CREATE ROLE test_role; -GRANT USAGE ON SCHEMA tap TO test_role; +-- test_role itself is created once, idempotently, by test/install/load.sql +-- (test/roles.sql is the single source of truth for the name; \i'd via +-- test/helpers/deps.sql). +GRANT USAGE ON SCHEMA tap TO :test_role; /* * DO NOT GRANT test_role TO test_factory__owner; the whole point test_role is * to check for security problems. */ -CREATE SCHEMA test AUTHORIZATION test_role; -SET ROLE = test_role; +CREATE SCHEMA test AUTHORIZATION :test_role; +SET ROLE = :test_role; SET search_path = test, tap; CREATE TABLE customer( @@ -79,11 +81,19 @@ SELECT hasnt_table( , 'Ensure original_role temp table was dropped' ); +-- Only meaningful when this session actually ran CREATE EXTENSION itself +-- (test_load_mode=fresh; the tables don't exist under update/existing, +-- where test/install/load.sql installed/updated the extension earlier). +SELECT to_regclass('pg_temp.pre_install_role') IS NOT NULL AS has_role_capture \gset +\if :has_role_capture SELECT is( (SELECT * FROM post_install_role) , (SELECT * FROM pre_install_role) , 'Ensure role is put back after install' ); +\else +SELECT skip('role-restore check only applies when this session ran CREATE EXTENSION itself (test_load_mode=fresh)', 1); +\endif SELECT cmp_ok( proconfig diff --git a/test/helpers/create_extension.sql b/test/helpers/create_extension.sql index 1fd2308..d35d776 100644 --- a/test/helpers/create_extension.sql +++ b/test/helpers/create_extension.sql @@ -1,7 +1,22 @@ \echo Creating extension :extension_name --- No IF NOT EXISTS because we'll be confused if we're not loading the new stuff +-- In 'update'/'existing' mode, test/install/load.sql already installed (and, +-- for 'update', updated) the extensions in its own earlier committed +-- session -- skip re-creating here instead of erroring or, worse, silently +-- replacing the state those modes exist to test. In 'fresh' mode (the only +-- mode test/install/load.sql leaves untouched), keep the original +-- behavior: no IF NOT EXISTS, so we're confused loudly if something's +-- already there instead of silently testing stale state. +SELECT + current_setting('test_factory.test_load_mode') <> 'fresh' + AND EXISTS (SELECT 1 FROM pg_extension WHERE extname = :'extension_name') + AS already_installed +\gset +\if :already_installed +\echo :extension_name already installed -- skipping CREATE EXTENSION (test_load_mode is not fresh) +\else CREATE TEMP TABLE pre_install_role AS SELECT current_user; GRANT SELECT ON pre_install_role TO public; -- In case role is different CREATE EXTENSION :extension_name; CREATE TEMP TABLE post_install_role AS SELECT current_user; GRANT SELECT ON post_install_role TO public; -- In case role is different +\endif diff --git a/test/helpers/deps.sql b/test/helpers/deps.sql index e69de29..455686f 100644 --- a/test/helpers/deps.sql +++ b/test/helpers/deps.sql @@ -0,0 +1 @@ +\i test/roles.sql diff --git a/test/install/load.out b/test/install/load.out new file mode 100644 index 0000000..25fdbb1 --- /dev/null +++ b/test/install/load.out @@ -0,0 +1 @@ +\set ECHO none diff --git a/test/install/load.sql b/test/install/load.sql new file mode 100644 index 0000000..e74812a --- /dev/null +++ b/test/install/load.sql @@ -0,0 +1,193 @@ +\set ECHO none +/* + * Foundation installer for the whole regression run. pgxntool's + * PGXNTOOL_ENABLE_TEST_INSTALL feature runs this file, committed, in its + * own pg_regress session before the regular test SQL files run -- so + * whatever it commits here survives into every test file, instead of each + * one creating its own install from scratch (which is still what the + * regular test files do in the default 'fresh' mode, unchanged). NOTE: this + * comment deliberately never spells out a bare wildcard glob right after a + * slash -- slash-star is a comment opener, and Postgres nests block + * comments, so an unbalanced extra opener earlier in a comment silently + * swallows the rest of the file instead of erroring where you'd notice (hit + * this for real while writing this file -- see the PR description). + * + * TEST_LOAD_SOURCE (make var -> test_factory.test_load_mode GUC, see + * Makefile) picks how the extension gets to its target state: + * fresh (default) - do nothing here; the regular test files still + * install it themselves, exactly as before this + * feature existed. + * update - CREATE EXTENSION VERSION :from, then ALTER EXTENSION UPDATE. + * existing - extension is already installed (a real pg_upgrade target, or + * an out-of-band update) -- assert-only, never drop/create. + */ +\i test/helpers/psql.sql +\i test/roles.sql + +SET client_min_messages = WARNING; + +-- Read unconditionally, without missing_ok: the Makefile always exports +-- test_factory.test_load_mode, so an unset GUC here means the harness +-- itself is broken, not "assume fresh". +SELECT current_setting('test_factory.test_load_mode') AS load_mode \gset +SELECT current_setting('test_factory.test_update_from') AS update_from \gset +SELECT current_setting('test_factory.test_update_to') AS update_to \gset +SELECT :'update_to' <> '' AS has_update_to \gset + +DO $$ +BEGIN + IF current_setting('test_factory.test_load_mode') NOT IN ('fresh', 'update', 'existing') THEN + RAISE EXCEPTION 'test_factory.test_load_mode must be fresh, update or existing, got %', current_setting('test_factory.test_load_mode'); + END IF; +END $$; + +-- test_role is test infrastructure, not part of what fresh/update/existing +-- describe -- (re)create it idempotently in every mode. A real pg_upgrade +-- target (existing mode) carries global objects like roles over, but don't +-- assume that; a from-scratch "existing" target might not have it. +SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_role \gset +\if :need_role +CREATE ROLE :test_role; +\endif + +-- psql's \if only accepts a plain boolean token, not a comparison +-- expression -- compute it via SQL first (\if :load_mode = 'existing' would +-- silently misparse instead of erroring). +SELECT :'load_mode' = 'existing' AS is_existing \gset +\if :is_existing + + -- existing: never drop/create/update -- only assert the extensions are + -- actually there, at the version this build considers current. Never + -- hardcode the expected version (see doc's CI dynamic-version-assertion + -- guidance); default_version comes from the same control file `make` + -- itself builds from. + DO $$ + DECLARE + v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'test_factory'); + v_default text := (SELECT default_version FROM pg_available_extensions WHERE name = 'test_factory'); + BEGIN + IF v_installed IS NULL THEN + RAISE EXCEPTION 'test_load_mode=existing but test_factory is not installed'; + END IF; + IF v_installed IS DISTINCT FROM v_default THEN + RAISE EXCEPTION 'test_factory installed at % but default_version is %', v_installed, v_default; + END IF; + END $$; + + DO $$ + DECLARE + v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'test_factory_pgtap'); + v_default text := (SELECT default_version FROM pg_available_extensions WHERE name = 'test_factory_pgtap'); + BEGIN + IF v_installed IS NULL THEN + RAISE EXCEPTION 'test_load_mode=existing but test_factory_pgtap is not installed'; + END IF; + IF v_installed IS DISTINCT FROM v_default THEN + RAISE EXCEPTION 'test_factory_pgtap installed at % but default_version is %', v_installed, v_default; + END IF; + END $$; + + /* + * Dependency guard (advanced-extension-testing checklist item 6): nothing + * else stops a stray CASCADE drop, or a logic bug that falls through to + * the fresh/update branch below, from silently destroying the real + * upgraded/updated objects this mode exists to test -- the suite would + * then quietly pass against a fresh reinstall instead. + * + * Only test_factory_pgtap needs an artificial guard here. test_factory + * already has a natural one for free: test_factory_pgtap's own control + * file (`requires = 'pgtap, test_factory'`) already makes a non-CASCADE + * DROP EXTENSION test_factory fail on its own, as long as + * test_factory_pgtap is still installed -- proved below too, so a future + * change that weakens that dependency doesn't go unnoticed. Nothing + * depends on test_factory_pgtap itself, so it gets an explicit guard. + * + * Only planted in existing mode, not fresh/update: test/sql/install.sql's + * own non-CASCADE DROP EXTENSION is a deliberate, self-contained test of + * drop/recreate -- that test is skipped under existing mode (see + * install.sql) precisely because it's incompatible with this guard. + */ + CREATE SCHEMA IF NOT EXISTS test_factory_drop_guard; + CREATE OR REPLACE VIEW test_factory_drop_guard.guard AS + SELECT 'tf.tap(text,text)'::regprocedure AS guarded_member; + + DO $$ + BEGIN + BEGIN + DROP EXTENSION test_factory_pgtap; + RAISE EXCEPTION 'dependency guard is not working: non-CASCADE DROP EXTENSION test_factory_pgtap succeeded'; + EXCEPTION + WHEN dependent_objects_still_exist THEN + NULL; -- expected: the guard view blocked it + END; + END $$; + + DO $$ + BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'test_factory_pgtap') THEN + RAISE EXCEPTION 'dependency guard proof left test_factory_pgtap dropped'; + END IF; + IF NOT EXISTS ( + SELECT 1 FROM pg_class + WHERE relname = 'guard' AND relnamespace = 'test_factory_drop_guard'::regnamespace + ) THEN + RAISE EXCEPTION 'dependency guard proof left the guard view itself missing'; + END IF; + END $$; + + DO $$ + BEGIN + BEGIN + DROP EXTENSION test_factory; + RAISE EXCEPTION 'test_factory''s natural drop-guard (test_factory_pgtap''s requires clause) is not working: non-CASCADE DROP EXTENSION test_factory succeeded'; + EXCEPTION + WHEN dependent_objects_still_exist THEN + NULL; -- expected + END; + END $$; + +\else + + /* + * fresh/update: drop-first reset, so a re-run against a non-fresh DB + * (e.g. local dev) starts from a known state. test_factory's own role + * bootstrapping (CREATE ROLE test_factory__owner, guarded by WHEN + * duplicate_object in sql/test_factory.sql) already tolerates being + * re-run, so unlike pgxntool's own drop-first example there's no + * separate role-drop step needed here. + */ + DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; + DROP EXTENSION IF EXISTS test_factory CASCADE; + + SELECT :'load_mode' = 'update' AS is_update \gset + \if :is_update + + CREATE EXTENSION test_factory VERSION :'update_from'; + SET client_min_messages = ERROR; -- suppress update-script deprecation NOTICEs + \if :has_update_to + ALTER EXTENSION test_factory UPDATE TO :'update_to'; + \else + ALTER EXTENSION test_factory UPDATE; + \endif + SET client_min_messages = WARNING; + + \endif + /* + * fresh: nothing else to do here -- the regular test files install the + * extension themselves, same as before this feature existed (see + * test/helpers/create_extension.sql). + * + * test_factory has only ever shipped one version (0.5.0), so 'update' + * mode above has no real historical update script to exercise yet -- + * CREATE EXTENSION VERSION '0.5.0' + ALTER EXTENSION UPDATE is a no-op + * today. The mechanism is built now per the advanced-extension-testing + * checklist (items 3-5); no CI job drives TEST_LOAD_SOURCE=update yet, + * since doing so wouldn't prove anything fresh-mode CI doesn't already + * cover. See the PR description for this reasoning. + */ + +\endif + +SET client_min_messages = NOTICE; + +-- vi: expandtab ts=2 sw=2 diff --git a/test/roles.sql b/test/roles.sql new file mode 100644 index 0000000..4f2f5f1 --- /dev/null +++ b/test/roles.sql @@ -0,0 +1,8 @@ +-- Single source of truth for test-only role names. Roles are global (not +-- schema- or session-scoped), so this can't be a table constant -- it's a +-- psql variable, \i'd from any session that needs to reference the name: +-- test/install/load.sql, and test/helpers/create.sql (via +-- test/helpers/deps.sql) for the test/sql/*.sql files. +\set test_role test_role + +-- vi: expandtab ts=2 sw=2 diff --git a/test/sql/pgtap.sql b/test/sql/pgtap.sql index 2abbeb6..135f032 100644 --- a/test/sql/pgtap.sql +++ b/test/sql/pgtap.sql @@ -1,6 +1,38 @@ \set ECHO none \i test/helpers/setup.sql +/* + * psql's \if only accepts a plain boolean token, not a comparison + * expression -- compute it via SQL first. + */ +SELECT current_setting('test_factory.test_load_mode') = 'existing' AS is_existing \gset + +\if :is_existing + +/* + * existing mode: both extensions are already installed (see + * test/install/load.sql) -- don't touch install ordering here; that's what + * the \else branch below tests. Bypass test/helpers/create_extension.sql + * entirely rather than let it no-op: it skips creating pre_install_role / + * post_install_role when the extension is already installed, and the + * \else branch's DROP TABLE calls on those would then error. Packaging / + * dependency-declaration checks now live in test/build/install.sql, not + * here (see add-test-build). Just exercise tf.tap() against what's + * already there. + */ +\i test/helpers/create.sql + +SELECT tf.tap( 'invoice' ); +SELECT tf.tap( 'invoice', 'base' ); +SELECT throws_ok( + $$SELECT tf.tap( '"non-existent table"' )$$ + , '42P01' + , 'relation "non-existent table" does not exist' + , 'Ensure we get sane error for a non-existent table' +); + +\else + \set extension_name test_factory \i test/helpers/create_extension.sql DROP TABLE pre_install_role; @@ -21,6 +53,8 @@ SELECT throws_ok( , 'Ensure we get sane error for a non-existent table' ); +\endif + ROLLBACK; -- vi: expandtab ts=2 sw=2 From a70aa4881e7d311b645f7dee3d03d2d3e50976b8 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 17:07:09 -0500 Subject: [PATCH 2/3] Convert multi-line SQL comments to /* */ style Several review comments added by the test/install foundation work used consecutive -- lines for what was really one continuous remark. This repo's convention (see the pre-existing test/helpers/create.sql and test/sql/install.sql) is to use C-style /* */ blocks for any comment spanning more than one line, reserving -- for single-line remarks. While converting test/roles.sql, reworded "test/sql/*.sql" to "*.sql files under test/sql/" -- the original phrasing contained a literal /* immediately after test/sql, which Postgres's nesting-aware block comment parser reads as an unwanted nested comment opener, leaving the enclosing comment unterminated. --- test/helpers/create.sql | 16 ++++++++----- test/helpers/create_extension.sql | 16 +++++++------ test/install/load.sql | 38 +++++++++++++++++++------------ test/roles.sql | 12 ++++++---- 4 files changed, 49 insertions(+), 33 deletions(-) diff --git a/test/helpers/create.sql b/test/helpers/create.sql index f320e1f..87b0f15 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -1,7 +1,9 @@ SET ROLE = DEFAULT; --- test_role itself is created once, idempotently, by test/install/load.sql --- (test/roles.sql is the single source of truth for the name; \i'd via --- test/helpers/deps.sql). +/* + * test_role itself is created once, idempotently, by test/install/load.sql + * (test/roles.sql is the single source of truth for the name; \i'd via + * test/helpers/deps.sql). + */ GRANT USAGE ON SCHEMA tap TO :test_role; /* * DO NOT GRANT test_role TO test_factory__owner; the whole point test_role is @@ -81,9 +83,11 @@ SELECT hasnt_table( , 'Ensure original_role temp table was dropped' ); --- Only meaningful when this session actually ran CREATE EXTENSION itself --- (test_load_mode=fresh; the tables don't exist under update/existing, --- where test/install/load.sql installed/updated the extension earlier). +/* + * Only meaningful when this session actually ran CREATE EXTENSION itself + * (test_load_mode=fresh; the tables don't exist under update/existing, + * where test/install/load.sql installed/updated the extension earlier). + */ SELECT to_regclass('pg_temp.pre_install_role') IS NOT NULL AS has_role_capture \gset \if :has_role_capture SELECT is( diff --git a/test/helpers/create_extension.sql b/test/helpers/create_extension.sql index d35d776..8dbe0f9 100644 --- a/test/helpers/create_extension.sql +++ b/test/helpers/create_extension.sql @@ -1,11 +1,13 @@ \echo Creating extension :extension_name --- In 'update'/'existing' mode, test/install/load.sql already installed (and, --- for 'update', updated) the extensions in its own earlier committed --- session -- skip re-creating here instead of erroring or, worse, silently --- replacing the state those modes exist to test. In 'fresh' mode (the only --- mode test/install/load.sql leaves untouched), keep the original --- behavior: no IF NOT EXISTS, so we're confused loudly if something's --- already there instead of silently testing stale state. +/* + * In 'update'/'existing' mode, test/install/load.sql already installed (and, + * for 'update', updated) the extensions in its own earlier committed + * session -- skip re-creating here instead of erroring or, worse, silently + * replacing the state those modes exist to test. In 'fresh' mode (the only + * mode test/install/load.sql leaves untouched), keep the original + * behavior: no IF NOT EXISTS, so we're confused loudly if something's + * already there instead of silently testing stale state. + */ SELECT current_setting('test_factory.test_load_mode') <> 'fresh' AND EXISTS (SELECT 1 FROM pg_extension WHERE extname = :'extension_name') diff --git a/test/install/load.sql b/test/install/load.sql index e74812a..f2fa03f 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -26,9 +26,11 @@ SET client_min_messages = WARNING; --- Read unconditionally, without missing_ok: the Makefile always exports --- test_factory.test_load_mode, so an unset GUC here means the harness --- itself is broken, not "assume fresh". +/* + * Read unconditionally, without missing_ok: the Makefile always exports + * test_factory.test_load_mode, so an unset GUC here means the harness + * itself is broken, not "assume fresh". + */ SELECT current_setting('test_factory.test_load_mode') AS load_mode \gset SELECT current_setting('test_factory.test_update_from') AS update_from \gset SELECT current_setting('test_factory.test_update_to') AS update_to \gset @@ -41,26 +43,32 @@ BEGIN END IF; END $$; --- test_role is test infrastructure, not part of what fresh/update/existing --- describe -- (re)create it idempotently in every mode. A real pg_upgrade --- target (existing mode) carries global objects like roles over, but don't --- assume that; a from-scratch "existing" target might not have it. +/* + * test_role is test infrastructure, not part of what fresh/update/existing + * describe -- (re)create it idempotently in every mode. A real pg_upgrade + * target (existing mode) carries global objects like roles over, but don't + * assume that; a from-scratch "existing" target might not have it. + */ SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_role \gset \if :need_role CREATE ROLE :test_role; \endif --- psql's \if only accepts a plain boolean token, not a comparison --- expression -- compute it via SQL first (\if :load_mode = 'existing' would --- silently misparse instead of erroring). +/* + * psql's \if only accepts a plain boolean token, not a comparison + * expression -- compute it via SQL first (\if :load_mode = 'existing' would + * silently misparse instead of erroring). + */ SELECT :'load_mode' = 'existing' AS is_existing \gset \if :is_existing - -- existing: never drop/create/update -- only assert the extensions are - -- actually there, at the version this build considers current. Never - -- hardcode the expected version (see doc's CI dynamic-version-assertion - -- guidance); default_version comes from the same control file `make` - -- itself builds from. + /* + * existing: never drop/create/update -- only assert the extensions are + * actually there, at the version this build considers current. Never + * hardcode the expected version (see doc's CI dynamic-version-assertion + * guidance); default_version comes from the same control file `make` + * itself builds from. + */ DO $$ DECLARE v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'test_factory'); diff --git a/test/roles.sql b/test/roles.sql index 4f2f5f1..a48f56f 100644 --- a/test/roles.sql +++ b/test/roles.sql @@ -1,8 +1,10 @@ --- Single source of truth for test-only role names. Roles are global (not --- schema- or session-scoped), so this can't be a table constant -- it's a --- psql variable, \i'd from any session that needs to reference the name: --- test/install/load.sql, and test/helpers/create.sql (via --- test/helpers/deps.sql) for the test/sql/*.sql files. +/* + * Single source of truth for test-only role names. Roles are global (not + * schema- or session-scoped), so this can't be a table constant -- it's a + * psql variable, \i'd from any session that needs to reference the name: + * test/install/load.sql, and test/helpers/create.sql (via + * test/helpers/deps.sql) for the *.sql files under test/sql/. + */ \set test_role test_role -- vi: expandtab ts=2 sw=2 From 9d7d76b0dc1a52ef2919e43d24e88bcb20944bbc Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 4 Aug 2026 17:20:11 -0500 Subject: [PATCH 3/3] Fix rebase fallout: stale install.sql doc refs, update-mode pgtap.sql crash test/CLAUDE.md still referenced test/sql/install.sql (deleted by add-test-build, now this branch's base) in the existing-mode section and the alternate-expected-file list; updated both to reflect that packaging/dependency-declaration checks now live in test/build/install.sql instead. Also fixes a real, pre-existing bug in test/sql/pgtap.sql surfaced by testing TEST_LOAD_SOURCE=update locally (not just the fresh/existing legs the rebase itself required): test/install/load.sql's update mode installs test_factory only, never test_factory_pgtap. pgtap.sql's non-existing-mode branch calls test/helpers/create_extension.sql for test_factory (a no-op when already installed, per its own already_installed check) and then unconditionally DROP TABLEs pre_install_role/post_install_role -- which that no-op never created, so the DROP errored under update mode. Guarded the same way test/helpers/create.sql already guards its own read of those tables (to_regclass(...) IS NOT NULL), rather than inventing a new pattern. --- test/CLAUDE.md | 17 ++++++++++------- test/sql/pgtap.sql | 12 ++++++++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/test/CLAUDE.md b/test/CLAUDE.md index 1b07c98..3afbe24 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -51,10 +51,13 @@ extension gets to its target state: - **existing** - the extension is already installed (a real `pg_upgrade` target, or an out-of-band update) -- `load.sql` only asserts it's present at the current version, plants a dependency guard (see below), and - proves it. `test/sql/install.sql` and the install/dependency-order part of - `test/sql/pgtap.sql` are skipped in this mode (see the `\if :is_existing` - branches in those files) since they'd otherwise defeat the guard by doing - their own from-scratch drop/recreate. + proves it. The install/dependency-order part of `test/sql/pgtap.sql` is + skipped in this mode (see the `\if :is_existing` branch in that file) + since it'd otherwise defeat the guard by doing its own from-scratch + drop/recreate. (Packaging/dependency-declaration checks used to live in + `test/sql/install.sql`; that file is gone -- see `test/build/install.sql` + below, which runs once via pg_regress's classic diffing rather than + per-`TEST_LOAD_SOURCE`-mode.) Run against a real pre-existing install with: ``` @@ -63,9 +66,9 @@ extension gets to its target state: ``` `existing` mode legitimately produces different (but equally valid) - output than `fresh`/`update` for `base`/`install`/`pgtap` (skipped - sections, a skipped role-restore check), so it has alternate expected - files: `test/expected/{base,install,pgtap}_1.out` (pg_regress's numbered + output than `fresh`/`update` for `base`/`pgtap` (skipped sections, a + skipped role-restore check), so it has alternate expected files: + `test/expected/{base,pgtap}_1.out` (pg_regress's numbered alternate-expected-file convention). ### Dependency Guard diff --git a/test/sql/pgtap.sql b/test/sql/pgtap.sql index 135f032..397e96e 100644 --- a/test/sql/pgtap.sql +++ b/test/sql/pgtap.sql @@ -35,8 +35,20 @@ SELECT throws_ok( \set extension_name test_factory \i test/helpers/create_extension.sql +/* + * update mode: load.sql already installed test_factory (never + * test_factory_pgtap -- see test/install/load.sql), so the \i above just + * skipped without creating pre_install_role/post_install_role (see + * test/helpers/create_extension.sql's already_installed check) -- an + * unconditional DROP TABLE here would error "does not exist". Guard the + * same way test/helpers/create.sql already does for its own role-restore + * check. + */ +SELECT to_regclass('pg_temp.pre_install_role') IS NOT NULL AS has_role_capture \gset +\if :has_role_capture DROP TABLE pre_install_role; DROP TABLE post_install_role; +\endif \set extension_name test_factory_pgtap \i test/helpers/create_extension.sql