From 247e4781561f3b52e5297abddc191b7f0bb7a4ae Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 14:18:32 -0500 Subject: [PATCH 1/7] 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 c4cb279..63cae6e 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -28,10 +28,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 e994e1b920fb1d44a80aa76818fc8384f6bc6eab Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 17:07:09 -0500 Subject: [PATCH 2/7] 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 262f6bc1960dd057cbbd172817afaa315419d9f7 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 4 Aug 2026 17:20:11 -0500 Subject: [PATCH 3/7] 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 63cae6e..37530df 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -53,10 +53,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: ``` @@ -65,9 +68,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 From ae91d20de384909031fe1f711771a4b7a490ec96 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 16:20:52 -0500 Subject: [PATCH 4/7] Move all-mode installation into test/install/load.sql; catalog-based dependency check test/install/load.sql is now the single place that installs the extension in every TEST_LOAD_SOURCE mode (fresh/update/existing), not just update/existing. This follows from deleting test/build/install.sql (rebase fallout from the add-test-build branch): test/build exists only to run scripts "bare" for better error context with throwaway results, so packaging/dependency checks belong in test/install/load.sql, which is meant to commit and persist. test/sql/base.sql and test/sql/pgtap.sql no longer branch on load mode or install anything themselves -- test/helpers/create_extension.sql (the install-or-skip wrapper they used) is deleted outright. This also means base.out/pgtap.out no longer need per-mode alternate expected files (test/expected/{base,pgtap}_1.out), since existing mode now produces the same output as fresh/update. pgtap.sql's dependency check moved from "try to CREATE EXTENSION test_factory_pgtap and see if it drags test_factory in" (only valid when pgtap.sql itself does the installing) to inspecting pg_depend directly, which works regardless of when/how installation happened. Confirmed against a live database that extension-requires-extension edges use deptype 'n' (normal), not 'e' (DEPENDENCY_EXTENSION, which instead means "this object belongs to this extension"). Fresh mode's CASCADE install surfaced a real ordering bug: it would cascade-install pgtap into the ambient search_path (public) before tap_setup.sql got a chance to create it in schema "tap", so the latter's CREATE EXTENSION IF NOT EXISTS silently no-op'd and every pgTAP-based test failed with "function no_plan() does not exist". Fixed by having load.sql create the tap schema and install pgtap into it before installing test_factory_pgtap in any mode. Verified fresh, update, and existing (against a manually pre-populated database) all pass with the same expected output. --- test/CLAUDE.md | 79 ++++++++++++---------- test/expected/base.out | 36 +++++----- test/expected/base_1.out | 25 ------- test/expected/pgtap.out | 12 ++-- test/expected/pgtap_1.out | 14 ---- test/helpers/create.sql | 19 ++---- test/helpers/create_extension.sql | 24 ------- test/install/load.sql | 106 ++++++++++++++++++++++-------- test/sql/base.sql | 3 +- test/sql/pgtap.sql | 79 +++++++++------------- 10 files changed, 183 insertions(+), 214 deletions(-) delete mode 100644 test/expected/base_1.out delete mode 100644 test/expected/pgtap_1.out delete mode 100644 test/helpers/create_extension.sql diff --git a/test/CLAUDE.md b/test/CLAUDE.md index 37530df..b536df9 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -28,9 +28,6 @@ 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; 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 @@ -39,27 +36,28 @@ The test_factory extension uses **pgTAP** (PostgreSQL's unit testing framework) `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`. +into every test file. It is the *only* place that knows how the extension +gets onto the system -- `test/sql/base.sql` and `test/sql/pgtap.sql` always +assume both extensions are already installed, in every mode, and are +otherwise identical regardless of which mode ran. `TEST_LOAD_SOURCE` +(default `fresh`) picks how the extension gets to its target state: + +- **fresh** (default) - drops both extensions first (so a re-run against a + non-fresh dev DB starts clean), lands `pgtap` in a dedicated `tap` schema, + then `CREATE EXTENSION test_factory_pgtap CASCADE` (proving + `test_factory_pgtap.control`'s `requires` line actually pulls + `test_factory` in). +- **update** - `CREATE EXTENSION test_factory VERSION :from` then + `ALTER EXTENSION UPDATE` (`TEST_UPDATE_FROM`/`TEST_UPDATE_TO` make vars), + then installs `test_factory_pgtap` at current (it has only ever shipped + one version, so there's no update path of its own to exercise yet -- 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. 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.) + proves it; never drops/creates/updates anything. Run against a real pre-existing install with: ``` @@ -67,11 +65,10 @@ extension gets to its target state: EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no ``` - `existing` mode legitimately produces different (but equally valid) - 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). +All three modes leave the system in the same observable end state (both +extensions installed, `pgtap` in schema `tap`), so `base.sql`/`pgtap.sql` +and their expected output are shared across all of them -- no +per-mode alternate expected files. ### Dependency Guard @@ -89,20 +86,24 @@ upgraded/updated objects this mode exists to test. ## Test Coverage Analysis ### Core Functionality Tests (`base.sql`) -1. **Extension Setup** - Creates extension and test tables -2. **Data Registration** - Tests `tf.register()` with multiple test sets -3. **Basic Retrieval** - Tests `tf.get()` returns correct data -4. **Dependency Resolution** - Tests automatic creation of dependent data (customer → invoice) -5. **Caching Behavior** - Verifies data consistency across multiple `tf.get()` calls -6. **Table Independence** - Tests that cached data persists after source table changes -7. **Function-based Test Data** - Tests using functions as test data sources +Assumes both extensions are already installed (by `test/install/load.sql`, +in every mode) and test tables not yet created: +1. **Data Registration** - Tests `tf.register()` with multiple test sets +2. **Basic Retrieval** - Tests `tf.get()` returns correct data +3. **Dependency Resolution** - Tests automatic creation of dependent data (customer → invoice) +4. **Caching Behavior** - Verifies data consistency across multiple `tf.get()` calls +5. **Table Independence** - Tests that cached data persists after source table changes +6. **Function-based Test Data** - Tests using functions as test data sources ### Security Tests (`create.sql`) -- **Role Management** - Validates proper role restoration after installation - **Security Definer Functions** - Ensures all privileged functions use `search_path=pg_catalog` - **Permission Isolation** - Tests with unprivileged `test_role` - **Temp Table Cleanup** - Verifies temporary installation objects are removed +Role-restore verification (does `CREATE EXTENSION` correctly restore the +calling role?) lives in `test/install/load.sql` instead, where `CREATE +EXTENSION` actually runs. + ### Raw SQL Syntax Tests (`test/build/syntax.sql`) - Runs `sql/test_factory.sql` and `sql/test_factory_pgtap.sql` (the actual source files, not the generated `sql/*--VERSION.sql` copies) directly via @@ -111,8 +112,18 @@ upgraded/updated objects this mode exists to test. - 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. +- This is the *only* thing `test/build` is for: running extension scripts + "bare" for better error context. Its results are always thrown away + (unlike `test/install`, which is intended to commit and persist). + Packaging checks (dependency declarations, clean install) belong in + `test/install/load.sql` instead. ### pgTAP Integration Tests (`pgtap.sql`) +- **Dependency Enforcement** - Confirms `test_factory_pgtap.control`'s + `requires = 'pgtap, test_factory'` line is real and enforced by Postgres, + via a `pg_depend` extension-requires-extension edge (`deptype = 'n'`), + not just documentation. Checking final catalog state this way works + uniformly in every `TEST_LOAD_SOURCE` mode. - **tf.tap() Function** - Tests pgTAP wrapper functionality - **Error Handling** - Tests proper error reporting for invalid inputs diff --git a/test/expected/base.out b/test/expected/base.out index 57f115a..75a9c3a 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,24 +1,22 @@ \set ECHO none -Creating extension test_factory 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 - 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 +ok 5 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 6 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 7 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 8 - Security definer function _tf.table_create has search_path=pg_catalog +ok 9 - Security definer function _tf.get has search_path=pg_catalog +ok 10 - customer table is empty +ok 11 - invoice table is empty +ok 12 - invoice factory output +ok 13 - invoice table content +ok 14 - customer table content +ok 15 - invoice factory second call +ok 16 - invoice table content stayed constant +ok 17 - customer table content stayed constant +ok 18 - Test function factory +ok 19 - customer table has new row +ok 20 - truncate invoice +ok 21 - invoice factory get remains the same after truncate diff --git a/test/expected/base_1.out b/test/expected/base_1.out deleted file mode 100644 index 0076bd0..0000000 --- a/test/expected/base_1.out +++ /dev/null @@ -1,25 +0,0 @@ -\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.out b/test/expected/pgtap.out index 5f552dc..2e529f2 100644 --- a/test/expected/pgtap.out +++ b/test/expected/pgtap.out @@ -1,11 +1,9 @@ \set ECHO none -Creating extension test_factory -Creating extension test_factory_pgtap -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 1 - test_factory_pgtap depends on test_factory (control file requires is real and enforced) +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 - 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 diff --git a/test/expected/pgtap_1.out b/test/expected/pgtap_1.out deleted file mode 100644 index e7c2b20..0000000 --- a/test/expected/pgtap_1.out +++ /dev/null @@ -1,14 +0,0 @@ -\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 87b0f15..75e5f6e 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -83,21 +83,10 @@ 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 +-- Role-restore verification (does CREATE EXTENSION correctly restore the +-- calling role?) now lives in test/install/load.sql, where CREATE +-- EXTENSION actually runs in every mode -- there's nothing to check here +-- anymore now that this file no longer runs it itself. SELECT cmp_ok( proconfig diff --git a/test/helpers/create_extension.sql b/test/helpers/create_extension.sql deleted file mode 100644 index 8dbe0f9..0000000 --- a/test/helpers/create_extension.sql +++ /dev/null @@ -1,24 +0,0 @@ -\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. - */ -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/install/load.sql b/test/install/load.sql index f2fa03f..3bbe914 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -4,20 +4,21 @@ * 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). + * one creating its own install from scratch. This is the ONLY place that + * knows how the extension gets onto the system, in every mode -- the + * regular test files (test/sql/base.sql, test/sql/pgtap.sql) always assume + * both extensions are already installed. 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. + * fresh (default) - CREATE EXTENSION test_factory_pgtap CASCADE. + * update - CREATE EXTENSION VERSION :from, then ALTER EXTENSION UPDATE, + * then install test_factory_pgtap too. * existing - extension is already installed (a real pg_upgrade target, or * an out-of-band update) -- assert-only, never drop/create. */ @@ -110,10 +111,16 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset * 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. + * Only planted in existing mode, not fresh/update: the guard's whole + * point is protecting the real upgraded/updated objects this mode exists + * to test, and fresh/update modes have nothing irreplaceable to protect + * (a re-run recreates everything from scratch there anyway). Note the + * guard only blocks a non-CASCADE drop -- it can't stop a logic bug that + * misroutes into the fresh/update branch below, since that branch's own + * drop-first reset uses CASCADE. Real protection against that specific + * failure mode is the is_existing check itself being correct, not this + * guard; this guard's job is narrower (an accidental *non-cascade* drop + * elsewhere while existing mode's state is live). */ CREATE SCHEMA IF NOT EXISTS test_factory_drop_guard; CREATE OR REPLACE VIEW test_factory_drop_guard.guard AS @@ -167,6 +174,29 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; + /* + * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap + * installs, not after. test_factory_pgtap.control declares pgtap as a + * requirement too, so the CREATE EXTENSION ... CASCADE (or plain CREATE + * EXTENSION, in update mode) below would otherwise cascade-install pgtap + * itself into whatever schema this session's ambient search_path + * resolves to (public, by default) -- and since that satisfies "pgtap is + * already installed", the main suite's own tap_setup.sql (which does + * CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap) then skips creating it + * in "tap" at all, leaving every pgTAP-based test file failing with + * "function no_plan() does not exist" (hit this for real writing this + * file). IF NOT EXISTS on both statements: harmless no-op on a rerun + * where a previous pass already did this and the drop-first reset above + * didn't touch it (cascading test_factory_pgtap's drop doesn't remove + * pgtap -- pgtap is its dependency, not the other way around). + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before either branch below runs CREATE EXTENSION, so the + -- role-restore proof after \endif covers whichever one actually ran. + SELECT current_user AS role_before_install \gset + SELECT :'load_mode' = 'update' AS is_update \gset \if :is_update @@ -178,21 +208,45 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset ALTER EXTENSION test_factory UPDATE; \endif SET client_min_messages = WARNING; + -- test_factory_pgtap has only ever shipped one version, so there's no + -- update path of its own to exercise -- just install it at current so + -- the rest of the suite can assume it's present, same as fresh mode. + CREATE EXTENSION test_factory_pgtap; + + \else + + /* + * fresh: install for real, right here, uniformly with update/existing -- + * so test/sql/base.sql and test/sql/pgtap.sql can assume both + * extensions are already present in every mode, instead of each mode + * needing its own install-or-skip dance (what test/helpers/ + * create_extension.sql used to do; deleted along with this change). + * CASCADE proves test_factory_pgtap.control's "requires = 'pgtap, + * test_factory'" line actually pulls test_factory in -- + * test/sql/pgtap.sql separately proves the dependency is *enforced* + * (via pg_depend), not just that cascade happens to work. + */ + CREATE EXTENSION test_factory_pgtap CASCADE; \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. + * Prove CREATE EXTENSION restored the calling role (whichever branch + * above actually ran it) -- a real security property, not a formality: + * test_factory.sql's install script does its own work as + * test_factory__owner via SET LOCAL ROLE, saving/restoring the original + * role around it. Compared from OUTSIDE (current_user before vs after), + * not by inspecting the internal GUC test_factory.sql saves it to -- + * that GUC is transaction-scoped (set_config(..., true)) and would + * already be gone by the time a later statement in this autocommit + * session could read it. */ + SELECT current_user AS role_after_install \gset + SELECT :'role_before_install' = :'role_after_install' AS role_was_restored \gset + \if :role_was_restored + \else + DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; + \endif \endif diff --git a/test/sql/base.sql b/test/sql/base.sql index f52bf4a..f41978c 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -1,8 +1,7 @@ \set ECHO none \i test/helpers/setup.sql -\set extension_name test_factory -\i test/helpers/create_extension.sql +-- test/install/load.sql already installed the extension, in every mode. -- NOTE: This runs some tests itself \i test/helpers/create.sql diff --git a/test/sql/pgtap.sql b/test/sql/pgtap.sql index 397e96e..468d63b 100644 --- a/test/sql/pgtap.sql +++ b/test/sql/pgtap.sql @@ -1,57 +1,42 @@ \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 +SET search_path = tap; /* - * 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. + * Prove test_factory_pgtap.control's "requires = 'pgtap, test_factory'" + * line is real and enforced by Postgres, not just documentation -- via a + * pg_depend extension-requires-extension edge, which CREATE EXTENSION + * always records for anything in `requires`, regardless of whether CASCADE + * was needed to satisfy it or the dependency was already present. Works + * uniformly in every TEST_LOAD_SOURCE mode, since it only inspects final + * state -- unlike attempting a bare, doomed CREATE EXTENSION + * test_factory_pgtap (the previous approach here), which only worked when + * this file could assume test_factory_pgtap wasn't installed yet; that + * assumption stopped holding once test/install/load.sql started installing + * both extensions in every mode, not just update/existing. + * + * deptype = 'n' (normal), NOT 'e': confirmed directly against a live + * database (`SELECT deptype, ... FROM pg_depend ...`) rather than assumed + * -- 'e' (DEPENDENCY_EXTENSION) is for "this object belongs to this + * extension" (e.g. a function belongs to test_factory), a completely + * different relationship from "this extension requires that extension", + * which pg_depend records as an ordinary 'n' dependency between the two + * pg_extension rows. */ -\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' +SELECT ok( + EXISTS ( + SELECT 1 + FROM pg_depend d + JOIN pg_extension req ON d.objid = req.oid AND d.classid = 'pg_extension'::regclass + JOIN pg_extension dep ON d.refobjid = dep.oid AND d.refclassid = 'pg_extension'::regclass + WHERE req.extname = 'test_factory_pgtap' + AND dep.extname = 'test_factory' + AND d.deptype = 'n' + ) + , 'test_factory_pgtap depends on test_factory (control file requires is real and enforced)' ); -\else - -\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 - -- NOTE: This runs some tests itself. It also changes search_path \i test/helpers/create.sql @@ -65,8 +50,6 @@ SELECT throws_ok( , 'Ensure we get sane error for a non-existent table' ); -\endif - ROLLBACK; -- vi: expandtab ts=2 sw=2 From 0176a3bc9a5ca8604db184350fe455e2351566cb Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 18:42:02 -0500 Subject: [PATCH 5/7] Fix sql-lint comment-stacked-dashes findings in create.sql and load.sql Never ran `make lint` locally after the foundation redesign, so these two multi-line -- comment blocks (added by that redesign) went unnoticed until CI caught them. Converted both to /* */ per the linter's rule. --- test/helpers/create.sql | 10 ++++++---- test/install/load.sql | 8 +++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 75e5f6e..8ff7b7f 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -83,10 +83,12 @@ SELECT hasnt_table( , 'Ensure original_role temp table was dropped' ); --- Role-restore verification (does CREATE EXTENSION correctly restore the --- calling role?) now lives in test/install/load.sql, where CREATE --- EXTENSION actually runs in every mode -- there's nothing to check here --- anymore now that this file no longer runs it itself. +/* + * Role-restore verification (does CREATE EXTENSION correctly restore the + * calling role?) now lives in test/install/load.sql, where CREATE + * EXTENSION actually runs in every mode -- there's nothing to check here + * anymore now that this file no longer runs it itself. + */ SELECT cmp_ok( proconfig diff --git a/test/install/load.sql b/test/install/load.sql index 3bbe914..b40a4bc 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -208,9 +208,11 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset ALTER EXTENSION test_factory UPDATE; \endif SET client_min_messages = WARNING; - -- test_factory_pgtap has only ever shipped one version, so there's no - -- update path of its own to exercise -- just install it at current so - -- the rest of the suite can assume it's present, same as fresh mode. + /* + * test_factory_pgtap has only ever shipped one version, so there's no + * update path of its own to exercise -- just install it at current so + * the rest of the suite can assume it's present, same as fresh mode. + */ CREATE EXTENSION test_factory_pgtap; \else From be9c35d66c7957a9d5ac7e573150c8245544f4af Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 14:03:15 -0500 Subject: [PATCH 6/7] load.sql: combine the 4 config-reading \gset calls into 1; \gset on its own line The four test_factory.test_load_mode/test_update_from/test_update_to reads and the has_update_to derived flag were 4 independent round-trips for no reason -- one row, one \gset. Also moved every remaining \gset in this file onto its own line, for consistency. --- test/install/load.sql | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/test/install/load.sql b/test/install/load.sql index b40a4bc..5c37e13 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -32,10 +32,12 @@ SET client_min_messages = WARNING; * 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 +SELECT + current_setting('test_factory.test_load_mode') AS load_mode + , current_setting('test_factory.test_update_from') AS update_from + , current_setting('test_factory.test_update_to') AS update_to + , current_setting('test_factory.test_update_to') <> '' AS has_update_to +\gset DO $$ BEGIN @@ -50,7 +52,8 @@ END $$; * 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 +SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_role +\gset \if :need_role CREATE ROLE :test_role; \endif @@ -60,7 +63,8 @@ CREATE ROLE :test_role; * expression -- compute it via SQL first (\if :load_mode = 'existing' would * silently misparse instead of erroring). */ -SELECT :'load_mode' = 'existing' AS is_existing \gset +SELECT :'load_mode' = 'existing' AS is_existing +\gset \if :is_existing /* @@ -195,9 +199,11 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset -- Captured before either branch below runs CREATE EXTENSION, so the -- role-restore proof after \endif covers whichever one actually ran. - SELECT current_user AS role_before_install \gset + SELECT current_user AS role_before_install + \gset - SELECT :'load_mode' = 'update' AS is_update \gset + SELECT :'load_mode' = 'update' AS is_update + \gset \if :is_update CREATE EXTENSION test_factory VERSION :'update_from'; @@ -243,8 +249,10 @@ SELECT :'load_mode' = 'existing' AS is_existing \gset * already be gone by the time a later statement in this autocommit * session could read it. */ - SELECT current_user AS role_after_install \gset - SELECT :'role_before_install' = :'role_after_install' AS role_was_restored \gset + SELECT current_user AS role_after_install + \gset + SELECT :'role_before_install' = :'role_after_install' AS role_was_restored + \gset \if :role_was_restored \else DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; From 04e71aaa93f0295c5cf400a28bd9b2f2c80b4fbc Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 15:46:39 -0500 Subject: [PATCH 7/7] Add psql \if-block naming convention; shorten pgtap.sql comment; apply to load.sql New CLAUDE.md rule: any \if/\else/\endif block spanning more than ~a dozen lines needs a short block-naming comment on each control statement, including \endif, so a reader landing on \else/\endif alone can tell which \if it belongs to. Applied it to load.sql's two long blocks (existing-vs-fresh/update, update-vs-fresh install). The comment must go on its own line above the control statement, not trailing on the same line: unlike SQL statements, \if/\else/\endif don't strip a trailing -- as a comment. \if parses the whole rest of the line as its boolean expression (a trailing comment breaks parsing outright, caught by re-running the suite after the first attempt used trailing comments -- test/install/load.out picked up literal "unrecognized value ... Boolean expected" and "extra argument ... ignored" lines). Documented this gotcha in the new CLAUDE.md rule so it isn't rediscovered the same way again. Also shortened test/sql/pgtap.sql's dependency-check comment -- kept only the one genuinely non-obvious fact (deptype 'n' vs 'e'), dropped the now-irrelevant history of the previous bare-CREATE-EXTENSION approach. --- CLAUDE.md | 17 +++++++++++++++++ test/install/load.sql | 6 ++++++ test/sql/pgtap.sql | 23 ++++------------------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c476d0d..9d060fc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,6 +6,23 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co After pushing to a branch with an open PR, monitor CI using `gh pr checks --watch` in a background subagent until all jobs pass or a failure is confirmed. Investigate and fix failures immediately rather than leaving them for the user to notice. +## psql Script Conventions + +Any `\if`/`\elsif`/`\else`/`\endif` block spanning more than ~a dozen lines needs a short comment naming the block on each of its control statements (including `\endif`), so a reader scrolling past `\else`/`\endif` on their own can tell at a glance which `\if` they belong to without scrolling back up. Put the comment on its own line immediately above the control statement, NOT trailing on the same line -- unlike SQL statements, psql's `\if`/`\else`/`\endif` don't treat a trailing `--` as a comment to strip: `\if` parses the entire rest of the line as its boolean expression (so a trailing comment breaks parsing outright), and `\else`/`\endif` parse it as an "extra argument" that gets ignored but still prints a warning into the actual output. Example: + +```sql +-- existing-mode install +\if :is_existing + ... +-- existing-mode install +\else + ... +-- existing-mode install +\endif +``` + +Short `\if` blocks don't need this -- the `\if` is still visible on screen alongside its `\else`/`\endif`. + ## Project Overview 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. diff --git a/test/install/load.sql b/test/install/load.sql index 5c37e13..1ad130c 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -65,6 +65,7 @@ CREATE ROLE :test_role; */ SELECT :'load_mode' = 'existing' AS is_existing \gset +-- existing-vs-fresh/update \if :is_existing /* @@ -165,6 +166,7 @@ SELECT :'load_mode' = 'existing' AS is_existing END; END $$; +-- existing-vs-fresh/update \else /* @@ -204,6 +206,7 @@ SELECT :'load_mode' = 'existing' AS is_existing SELECT :'load_mode' = 'update' AS is_update \gset + -- update-vs-fresh install \if :is_update CREATE EXTENSION test_factory VERSION :'update_from'; @@ -221,6 +224,7 @@ SELECT :'load_mode' = 'existing' AS is_existing */ CREATE EXTENSION test_factory_pgtap; + -- update-vs-fresh install \else /* @@ -236,6 +240,7 @@ SELECT :'load_mode' = 'existing' AS is_existing */ CREATE EXTENSION test_factory_pgtap CASCADE; + -- update-vs-fresh install \endif /* @@ -258,6 +263,7 @@ SELECT :'load_mode' = 'existing' AS is_existing DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; \endif +-- existing-vs-fresh/update \endif SET client_min_messages = NOTICE; diff --git a/test/sql/pgtap.sql b/test/sql/pgtap.sql index 468d63b..f0ecbcb 100644 --- a/test/sql/pgtap.sql +++ b/test/sql/pgtap.sql @@ -4,25 +4,10 @@ SET search_path = tap; /* - * Prove test_factory_pgtap.control's "requires = 'pgtap, test_factory'" - * line is real and enforced by Postgres, not just documentation -- via a - * pg_depend extension-requires-extension edge, which CREATE EXTENSION - * always records for anything in `requires`, regardless of whether CASCADE - * was needed to satisfy it or the dependency was already present. Works - * uniformly in every TEST_LOAD_SOURCE mode, since it only inspects final - * state -- unlike attempting a bare, doomed CREATE EXTENSION - * test_factory_pgtap (the previous approach here), which only worked when - * this file could assume test_factory_pgtap wasn't installed yet; that - * assumption stopped holding once test/install/load.sql started installing - * both extensions in every mode, not just update/existing. - * - * deptype = 'n' (normal), NOT 'e': confirmed directly against a live - * database (`SELECT deptype, ... FROM pg_depend ...`) rather than assumed - * -- 'e' (DEPENDENCY_EXTENSION) is for "this object belongs to this - * extension" (e.g. a function belongs to test_factory), a completely - * different relationship from "this extension requires that extension", - * which pg_depend records as an ordinary 'n' dependency between the two - * pg_extension rows. + * Confirms test_factory_pgtap.control's "requires = 'pgtap, test_factory'" + * is actually enforced by Postgres, via a pg_depend extension-requires- + * extension edge -- deptype 'n' (normal), NOT 'e' (DEPENDENCY_EXTENSION, + * which instead means "this object belongs to this extension"). */ SELECT ok( EXISTS (