diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef7d11a..aa14bd4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,15 +35,9 @@ # through AWS pg_tle's database-backed catalog # instead of a filesystem .control file. # -# Every TEST_SCHEMA value (empty - no schema targeting at all - and -# 'Quoted', a name requiring SQL identifier quoting) is exercised too, via -# `make test-schema-all`'s in-Makefile loop rather than a CI matrix -# dimension - a schema name is just an input the same assertions run -# against, not a real environment difference, so crossing it into the -# matrix would only multiply job count for no added confidence (see the -# Makefile's TEST_SCHEMA_VALUES comment). Every leg passes against the SAME -# test/expected/extension_tests.out (see test/README.md for how the suite -# keeps its output schema-invariant). +# During testing we always install into a freshly, randomly generated +# schema (see test/install/load.sql; test/README.md covers how +# test/expected/extension_tests.out stays schema-invariant). # # `changes` is a cheap gate that lets the heavy jobs above skip themselves on # doc-only pushes, and also derives the shared PostgreSQL-major list those @@ -251,16 +245,10 @@ jobs: # UPDATEs to current (no pg_upgrade, same PostgreSQL) and reruns the # suite - a single job rather than a dedicated one, since a load mode is # just an input the same assertions run against, not a real environment - # difference (same reasoning as TEST_SCHEMA below), and the per-version - # container/checkout setup would otherwise be duplicated across two jobs - # with the same PG matrix. Every TEST_SCHEMA value (empty - no schema - # targeting at all - and 'Quoted', a name requiring SQL identifier - # quoting) is exercised too, via `make test-schema-all`'s in-Makefile - # loop rather than a CI matrix dimension - a schema name is just an - # input the same assertions run against, not a real environment - # difference, so crossing it into the matrix would only multiply job - # count for no added confidence (see the Makefile's TEST_SCHEMA_VALUES - # comment). Every leg passes against the SAME + # difference, and the per-version container/checkout setup would + # otherwise be duplicated across two jobs with the same PG matrix. Each + # install (both legs) always targets its own freshly, randomly generated + # schema (see test/install/load.sql), so both legs pass against the SAME # test/expected/extension_tests.out (see test/README.md for how the # suite keeps its output schema-invariant). test: @@ -284,8 +272,8 @@ jobs: run: pg-start ${{ matrix.pg }} - name: Check out the repo uses: actions/checkout@v4 - - name: Test on PostgreSQL ${{ matrix.pg }}, across every TEST_SCHEMA value - run: make test-schema-all + - name: Test on PostgreSQL ${{ matrix.pg }} + run: make test - name: Install count_nulls run: make install - name: Update 0.9.6 -> current and run the suite @@ -300,10 +288,8 @@ jobs: # A single old_pg/new_pg leg (old_pg from the changes job's floor_pg # output, new_pg from its newest_pg output - see that job's comment; NOT # a second/third hardcoded copy of either number), which via TWIN - # databases in the SAME cluster, migrated by a SINGLE pg_upgrade call - # (the same trick this file uses to cross TEST_SCHEMA without doubling - # matrix legs elsewhere), compares BOTH update-vs-pg_upgrade orderings a - # real user could hit: + # databases in the SAME cluster, migrated by a SINGLE pg_upgrade call, + # compares BOTH update-vs-pg_upgrade orderings a real user could hit: # upgrade_oldest_first -- stays at 0.9.6 through the # binary pg_upgrade, updated to current AFTER it (on the NEW # cluster). Proves pg_upgrade itself correctly preserves objects @@ -341,9 +327,6 @@ jobs: # functions over anyarray/json/jsonb, nothing version-sensitive to # break at a specific boundary. Revisit if count_nulls ever grows # something catalog-touching. - # - # Not yet crossed with TEST_SCHEMA (a later phase adds that, once it can - # do so for both this job and the test job's update leg together). pg-upgrade-test: needs: [changes] # Skipped outright (not just matrix-reduced like `test`) on a draft PR: @@ -391,8 +374,8 @@ jobs: # anywhere in this job cannot silently make the eventual # existing-mode run test a fresh install instead. run: | - bin/test_existing prepare-old upgrade_oldest_first "" 0.9.6 - bin/test_existing prepare-old upgrade_current_first "" 0.9.6 + bin/test_existing prepare-old upgrade_oldest_first 0.9.6 + bin/test_existing prepare-old upgrade_current_first 0.9.6 - name: Update to current before pg_upgrade (proves pg_upgrade preserves CURRENT objects) # Updates ONLY upgrade_current_first # (upgrade_oldest_first must NOT be touched here - it @@ -455,8 +438,8 @@ jobs: # test a fresh install instead of the migrated objects. Once per # database, since each holds an independent ordering's result. run: | - bin/test_existing run-suite upgrade_oldest_first "" - bin/test_existing run-suite upgrade_current_first "" + bin/test_existing run-suite upgrade_oldest_first + bin/test_existing run-suite upgrade_current_first pg-tle-test: needs: [changes] @@ -585,11 +568,11 @@ jobs: # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like - # "🐘 PostgreSQL 14 (schema none)" which would all need to be listed - # individually and updated whenever the matrix changes. This job passes if - # all others passed or were skipped (e.g. the heavy jobs gated off by the - # `changes` job on a docs-only push), and fails if any failed or were - # cancelled. + # "🐘 PostgreSQL 14" (one per supported major) which would all need to be + # listed individually and updated whenever the matrix changes. This job + # passes if all others passed or were skipped (e.g. the heavy jobs gated + # off by the `changes` job on a docs-only push), and fails if any failed + # or were cancelled. all-checks-passed: needs: [changes, lint, test, pg-upgrade-test, pg-tle-test] if: always() diff --git a/Makefile b/Makefile index 1199bb6..eaf793c 100644 --- a/Makefile +++ b/Makefile @@ -12,64 +12,6 @@ testdeps: $(wildcard test/*/*.sql) $(wildcard test/*.sql) # Be careful not to in LINT_TARGETS = sql/count_nulls.sql test/ include lint.mk -# TEST_SCHEMA selects which schema test/install/load.sql installs count_nulls -# into, for the WHOLE test run (every test file sees the SAME schema in a -# given run). -# -# Empty (the default): don't target any schema at all - count_nulls installs -# wherever the session's own default search_path already resolves. Non-empty: -# explicitly CREATE SCHEMA, then CREATE EXTENSION ... WITH SCHEMA that name - -# including a name that requires SQL identifier quoting (mixed case - -# unquoted would fold to lowercase), to exercise the suite's %I -# schema-qualification rather than just its literal test data. Locally: -# `make test TEST_SCHEMA=Quoted`. -# -# Installing into two schemas only proves that %I-qualification works if the -# test session's search_path never includes count_nulls' own schema in at -# least one of those legs - otherwise an extension full of unqualified, -# resolve-by-accident references would pass every leg too (see -# test/core/functions.sql's header and test__check_ncs in -# test/sql/extension_tests.sql, which is what actually checks this). This -# suite excludes it in BOTH legs, which is stronger than the minimum needed - -# not a requirement in itself. -# -# Propagated as a GUC (count_nulls.test_schema), exported unconditionally via -# PGOPTIONS - pg_regress doesn't forward make variables, but the psql -# processes it spawns inherit the environment. Empty is a valid, deliberate -# value (not an error) - read without missing_ok, so a truly unpropagated GUC -# still fails loudly instead of looking identical to a deliberately empty one. -TEST_SCHEMA ?= -export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_schema=$(TEST_SCHEMA) - -# Every TEST_SCHEMA value the suite is tested against. A single source so -# test-schema-all/test-update-schema-all and CI can't silently drift onto -# different sets. See the TEST_SCHEMA comment above for why exercising more -# than one value here is meaningful (search_path exclusion), not just -# "install into schema A vs schema B". -TEST_SCHEMA_VALUES = "" Quoted - -# TEST_SCHEMA is deliberately NOT a CI matrix dimension: unlike PostgreSQL -# major (a real environment difference - different binaries, different -# container) or pg_tle deployment (a real isolation boundary - must never -# share a runner with a filesystem install), a schema name is just an input -# value the SAME assertions run against in the SAME environment. Crossing it -# into the matrix would only multiply job count (container boot + checkout -# per leg) for zero additional confidence per dollar. Loop it inside make -# instead - the same pattern test-update already uses for the load-mode -# axis, generalized to a list via a shell loop. Sequential recursive $(MAKE) -# calls, deliberately NOT bare prerequisites (which Make can run -# concurrently under -j and would collide on the same throwaway test -# database). `exit 1` on the first failure so a later iteration can't hide -# an earlier one; each iteration is echoed so a failure's TEST_SCHEMA value -# is still directly attributable in the log even without a separate CI -# check name per value. -.PHONY: test-schema-all -test-schema-all: - @for schema in $(TEST_SCHEMA_VALUES); do \ - echo "=== TEST_SCHEMA=$$schema ==="; \ - $(MAKE) test TEST_SCHEMA="$$schema" || exit 1; \ - done - # TEST_LOAD_SOURCE selects how test/install/load.sql installs count_nulls # for the WHOLE test run: # - fresh (default): CREATE EXTENSION count_nulls (current version). @@ -87,8 +29,10 @@ test-schema-all: # "update" (this) is extension-level (ALTER EXTENSION UPDATE); "upgrade" is # cluster-level (pg_upgrade) - 'existing' is how that axis is exercised. # -# Propagated the same way as TEST_SCHEMA: via the count_nulls.test_load_mode -# GUC, exported unconditionally through PGOPTIONS, read without missing_ok. +# Propagated as a GUC (count_nulls.test_load_mode), exported unconditionally +# through PGOPTIONS - pg_regress doesn't forward make variables, but the +# psql processes it spawns inherit the environment. Read without missing_ok: +# a genuinely unpropagated GUC must fail loudly, not look like a valid value. 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)') diff --git a/bin/test_existing b/bin/test_existing index b43c987..32c26fd 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -32,14 +32,16 @@ # # USAGE: bin/test_existing [args] # -# prepare-old DB SCHEMA INSTALL_VERSION +# prepare-old DB INSTALL_VERSION # Old-cluster prep for pg-upgrade-test: create DB + extension at -# INSTALL_VERSION in SCHEMA, then plant + prove the dependency guard. +# INSTALL_VERSION (in a freshly, randomly generated schema - see +# test/helpers/create_test_schema.sql), then plant + prove the +# dependency guard. # # update DB [TO_VERSION] # ALTER EXTENSION count_nulls UPDATE [TO 'TO_VERSION'] (empty => current). # -# run-suite DB SCHEMA +# run-suite DB # Assert the current version, re-prove the guard, drop it, then run the # suite in existing mode (extension must be at the current version). # @@ -101,8 +103,8 @@ installed_version() { # Plant the guard and PROVE it blocks a non-CASCADE drop. Call right after # CREATE EXTENSION (and before any update/upgrade) so it persists through them. plant_guard() { - local db=$1 schema=$2 - psql -d "$db" -v ON_ERROR_STOP=1 -v schema="$schema" -f bin/test_existing.sql/plant_guard.sql + local db=$1 + psql -d "$db" -v ON_ERROR_STOP=1 -f bin/test_existing.sql/plant_guard.sql assert_drop_blocked "$db" } @@ -140,35 +142,29 @@ update_ext() { psql_do "$db" -c "ALTER EXTENSION count_nulls UPDATE $to" } -# CREATE EXTENSION count_nulls at VERSION, targeting SCHEMA - unless SCHEMA -# is empty, in which case it's created untouched, wherever the session's -# own default search_path resolves (ordinarily 'public'). A quoted empty -# identifier ("") is a real Postgres syntax error, so this can't just always -# emit `CREATE SCHEMA IF NOT EXISTS "$schema"` - the empty case has to skip -# that entirely, mirroring test/install/load.sql's own :count_nulls_has_schema -# branch. +# CREATE EXTENSION count_nulls at VERSION, into a freshly, randomly +# generated schema - shared with test/install/load.sql's own fresh/update +# installs via test/helpers/create_test_schema.sql (see that file for the +# full rationale). create_extension_in_schema() { - local db=$1 schema=$2 version=$3 sql="" - if [ -n "$schema" ]; then - sql="CREATE SCHEMA IF NOT EXISTS \"$schema\"; SET search_path = \"$schema\"; " - fi - psql_do "$db" -c "${sql}CREATE EXTENSION count_nulls VERSION '$version'" + local db=$1 version=$2 + psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" -f test/helpers/create_test_schema.sql } # --------------------------------------------------------------------------- # Subcommand implementations # --------------------------------------------------------------------------- -# prepare-old DB SCHEMA INSTALL_VERSION +# prepare-old DB INSTALL_VERSION # Old-cluster preparation for pg-upgrade-test: create the database and the -# extension at INSTALL_VERSION in SCHEMA, then plant + prove the guard. No +# extension at INSTALL_VERSION, then plant + prove the guard. No # bridge-update step first: count_nulls ships no SELECT-*-over-catalog # views, so it has no known pg_upgrade-unsafe old version to bridge past. prepare_old() { - local db=$1 schema=$2 install=$3 + local db=$1 install=$2 createdb "$db" - create_extension_in_schema "$db" "$schema" "$install" - plant_guard "$db" "$schema" + create_extension_in_schema "$db" "$install" + plant_guard "$db" } # Run the pgTAP suite against an already-populated database in existing mode. @@ -179,7 +175,7 @@ prepare_old() { # then runs the suite via --use-existing so pg_regress does NOT drop/recreate # the database. run_suite() { - local db=$1 schema=$2 + local db=$1 assert_version "$db" current assert_drop_blocked "$db" drop_guard "$db" @@ -188,14 +184,14 @@ run_suite() { # verify-results`) is a real gate as of pgxntool 2.3.0 - it now exits # non-zero on regression failures instead of always exiting 0 regardless # of pg_regress's result (see this repo's pgxntool 2.3.0 bump). - make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing + make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing } usage() { echo "usage: bin/test_existing [args]" >&2 - echo " prepare-old DB SCHEMA INSTALL_VERSION" >&2 + echo " prepare-old DB INSTALL_VERSION" >&2 echo " update DB [TO_VERSION]" >&2 - echo " run-suite DB SCHEMA" >&2 + echo " run-suite DB" >&2 exit 2 } diff --git a/bin/test_existing.sql/plant_guard.sql b/bin/test_existing.sql/plant_guard.sql index 1c3e84d..da3b77c 100644 --- a/bin/test_existing.sql/plant_guard.sql +++ b/bin/test_existing.sql/plant_guard.sql @@ -8,23 +8,17 @@ * otherwise fall through to a silent fresh reinstall and the job would * still report green). * - * Usage: psql -v ON_ERROR_STOP=1 -v schema= -f plant_guard.sql - * (empty schema means "wherever null_count already resolves unqualified" - - * i.e. count_nulls was installed without targeting a schema). + * count_nulls always installs into its own randomly generated schema (see + * test/helpers/create_test_schema.sql) - this session didn't create it, so + * it has no other way to know its name; test/helpers/find_test_schema.sql + * discovers it live via pg_namespace. + * + * Usage: psql -v ON_ERROR_STOP=1 -f plant_guard.sql */ \set ON_ERROR_STOP on -/* - * schema_prefix: either empty, or the quoted schema name followed by a - * literal '.' - so the view definition below is a single statement with a - * plain (unquoted) substitution, rather than branching the whole CREATE - * VIEW on whether a schema was given. quote_ident(), not :"schema" - - * :schema_prefix is pasted as-is (unquoted substitution), so it must - * already be valid, properly-quoted SQL text by the time it lands there. - */ -SELECT CASE WHEN :'schema' <> '' THEN quote_ident(:'schema') || '.' ELSE '' END AS schema_prefix -\gset +\i test/helpers/find_test_schema.sql CREATE SCHEMA IF NOT EXISTS count_nulls_drop_guard; CREATE OR REPLACE VIEW count_nulls_drop_guard.guard AS - SELECT :schema_prefix null_count(NULL::int, NULL::int) AS guarded_member; + SELECT :"test_schema".null_count(NULL::int, NULL::int) AS guarded_member; diff --git a/test/README.md b/test/README.md index 3ae3a96..b3b58e3 100644 --- a/test/README.md +++ b/test/README.md @@ -28,41 +28,54 @@ then invoke via `runtests()`. `test__*` functions of its own (`test__check_ncs`, asserting count_nulls landed where expected; `test__shutdown__drop_all`, asserting it can be cleanly dropped), then runs everything via `runtests()`. +- `helpers/create_test_schema.sql` — creates the freshly, randomly generated + schema and installs count_nulls into it (see "Schema targeting" below). + Shared by `install/load.sql`'s fresh/update modes and `bin/test_existing`'s + `prepare-old` - two separate call sites, one shared implementation. +- `helpers/find_test_schema.sql` — rediscovers the randomly generated schema + count_nulls was installed into (see "Schema targeting" below), for + sessions that didn't create it themselves. -## TEST_SCHEMA +## Schema targeting -A make var/GUC (`count_nulls.test_schema`, propagated the same way as any -other placeholder GUC: `make var` -> `PGOPTIONS -c ...` -> `current_setting()` -- pg_regress doesn't forward make variables, but the psql processes it -spawns inherit the environment) selecting which schema `install/load.sql` -installs count_nulls into: +`helpers/create_test_schema.sql` always installs count_nulls into its own +freshly, randomly generated schema - never a fixed name, and never no +schema at all. It's shared by `install/load.sql`'s fresh/update modes (`\i`'d +in the same psql session) and `bin/test_existing`'s `prepare-old` (a +separate, `-f`'d invocation) - the schema-targeting behavior described here +applies to both. The generated name +(`'count_nulls test schema ' || substr(md5(random()::text), 1, 12)`) has two +deliberate properties: -- Empty (default): no schema targeting at all - count_nulls lands wherever - the session's own default search_path resolves. Since `install/load.sql` - runs in its own bare connection (not the in-suite session pgTAP's own - `tap_setup.sql` runs in), that's `public`. -- Non-empty: explicitly `CREATE SCHEMA`, then `CREATE EXTENSION ... WITH - SCHEMA` that name - `install/load.sql` never mutates its own - search_path to do this. `TEST_SCHEMA=Quoted` locally exercises a name - requiring SQL identifier quoting (mixed case - unquoted would fold to - lowercase). +- A constant prefix (`count_nulls test schema `, with a trailing space) + that by itself already requires SQL identifier quoting - so every single + run exercises the suite's `%I`-qualification, not just a dedicated + "quoting" leg that could bitrot independently of a "plain" one. +- The same prefix doubles as a marker for stale-schema cleanup: before + generating a new name, `helpers/create_test_schema.sql` finds and drops + any already-existing schema matching the prefix (`nspname LIKE + 'count_nulls test schema %'`), so a schema left behind by a run that + crashed before reaching its own teardown doesn't accumulate run over run. -Both legs run in CI - genuinely different code paths, not one a redundant -special case of the other. +`helpers/create_test_schema.sql` targets the generated schema via `CREATE +EXTENSION ... WITH SCHEMA`, never by mutating its own search_path first. -**Why two legs prove anything.** Installing into two different schemas by -itself doesn't test whether count_nulls' own SQL correctly schema-qualifies -its internal references - if BOTH schemas happened to stay on the test -session's search_path (e.g. because the empty leg's `public` and the -`TEST_SCHEMA` leg's target were both reachable), an extension full of -unqualified, resolve-by-accident references would pass every leg too. What -actually matters is that at least ONE leg's install schema is verifiably -absent from search_path, so that leg's assertions only pass if `%I`-qualified -references are genuinely correct - checked by `test__check_ncs` in -`sql/extension_tests.sql`. This suite goes further and excludes the schema -from search_path in *every* leg, via the fixed `SET SEARCH_PATH` in -`core/functions.sql` - a stronger, deliberate choice, not the minimum -required. +**Cross-session discovery.** Some scripts/sessions (e.g. `bin/test_existing`'s +steps, each a fresh `psql -f ...` invocation with no memory of another +invocation's `\gset` variables) need the generated name without having +created it themselves. `helpers/find_test_schema.sql` looks it up live via +`pg_namespace`, hard-failing (not a pgTAP assertion - a genuinely broken +condition, like zero or more than one matching schema) if it can't find +exactly one, and sets `:"test_schema"` via `\gset` for the including script +to use. + +**Why this proves anything.** Because count_nulls' own schema is randomly +named, it can never coincidentally end up on the test session's +search_path - so `core/functions.sql`'s `%I`-qualified calls (via `ncs()`) +only pass if they're genuinely correct, never because count_nulls' schema +happened to be reachable unqualified. `test__check_ncs` in +`sql/extension_tests.sql` is what actually checks this, via the fixed `SET +SEARCH_PATH` in `core/functions.sql`. **Assertion descriptions deliberately never embed the schema name.** `core/functions.sql`'s assertions build the SQL they *execute* via `%I` @@ -70,17 +83,17 @@ qualification (through `ncs()`, so they're always correct no matter which real schema count_nulls landed in) but pass an *explicit*, schema-free description to every pgTAP call - overriding pgTAP's own auto-generated descriptions, which otherwise embed the schema. This is what keeps -`test/expected/extension_tests.out` a single file that both TEST_SCHEMA -legs pass against, instead of needing one file per schema value. +`test/expected/extension_tests.out` a single file that passes no matter +which randomly generated name count_nulls actually landed in. -**`test__shutdown__drop_all`'s schema drop is plain cleanup, not a TAP -assertion.** Dropping the schema TEST_SCHEMA created is a real, correct -behavioral difference between "there's a schema to clean up" (non-empty) -and "there isn't" (empty, nothing to drop) - but it's teardown, not -something this suite is testing, so it's plain `EXECUTE`'d SQL with no -`lives_ok()`/`skip()` branch. That keeps its TAP output identical in every -TEST_SCHEMA leg (one `ok` row either way), so `test/expected/extension_tests.out` -needs no numbered pg_regress alternate for this function. +**`test__shutdown__drop_all` only asserts the extension can be dropped - +it doesn't clean up the schema itself.** Dropping the schema count_nulls +was installed into isn't something this suite is testing, and +`helpers/create_test_schema.sql` already unconditionally drops any +leftover schema before the next run creates its own, so a second, per-run +drop here would only ever be redundant. Its output is identical on every +run (one `ok` row), so `test/expected/extension_tests.out` needs no +numbered pg_regress alternate for this function. ## Regenerating expected output diff --git a/test/core/functions.sql b/test/core/functions.sql index 33bc30b..4d06ffc 100644 --- a/test/core/functions.sql +++ b/test/core/functions.sql @@ -75,11 +75,12 @@ BEGIN /* * Explicit descriptions below (not pgTAP's auto-generated default, - * which schema-qualifies via ncs()): this suite may run against - * count_nulls installed in ANY schema (see TEST_SCHEMA in the - * Makefile), and the description text is exact-matched by pg_regress - * against a single committed expected-output file - it must stay - * IDENTICAL no matter which schema the extension actually landed in. + * which schema-qualifies via ncs()): this suite runs against + * count_nulls installed in a freshly, randomly generated schema (see + * "Schema targeting" in test/README.md), and the description text is + * exact-matched by pg_regress against a single committed + * expected-output file - it must stay IDENTICAL no matter which + * schema the extension actually landed in. * ncs() itself is still used to locate and call the real function; * only the visible description text drops it. */ @@ -140,7 +141,8 @@ CREATE FUNCTION pg_temp.test_trigger_raw( * Schema-free stand-in for exec, used ONLY in the visible description * below - exec itself (schema-qualified via ncs(), by callers) is what * actually runs. Keeps this suite's output identical no matter which - * schema count_nulls is installed in (see TEST_SCHEMA). + * randomly generated schema count_nulls is installed in (see "Schema + * targeting" in test/README.md). */ , exec_desc text diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql new file mode 100644 index 0000000..bd236e7 --- /dev/null +++ b/test/helpers/create_test_schema.sql @@ -0,0 +1,78 @@ +/* + * Creates a fresh, randomly named schema and installs count_nulls into it. + * Shared by test/install/load.sql (fresh/update modes - same psql session, + * `\set version` then `\i` this file) AND bin/test_existing's prepare-old + * (a SEPARATE invocation - `-v version=` on the command + * line). Unusual for a test/ file to also be invoked from bin/, but the + * creation logic is identical in both cases, so it lives here once instead + * of being duplicated. + * + * :version must always be set explicitly to either the literal string + * 'current' (no VERSION clause - installs whatever the current default is) + * or a real version string (targets that specific version) - matching the + * same 'current' sentinel bin/test_existing's assert_version()/ + * current_version() already use, for the same reason: an empty string is a + * HARD ERROR rather than a valid signal, so an accidentally-unpropagated + * :version fails loudly instead of silently installing 'current' when + * something else was actually intended. + * + * The guard below bridges :version into the DO block via a SET + a real + * GUC (like test/install/load.sql's count_nulls.test_load_mode) rather than + * referencing :'version' directly inside the DO $$ ... $$ body: psql does + * NOT interpolate variables inside dollar-quoted strings (confirmed + * directly - a bare :'version' inside a $$ ... $$ block reaches the server + * un-substituted and is a syntax error), only in plain top-level SQL text + * such as the version_clause SELECT below. + * + * The generated name's constant prefix (a literal trailing space included) + * already guarantees SQL identifier quoting is required before the random + * suffix is even appended - unlike a mixed-case-only name, which would + * only force quoting by coincidence of which characters the randomness + * happened to produce. + * + * Cleanup-before-create: a prior run that crashed before reaching its own + * teardown would otherwise leave its randomly-named schema behind forever, + * since nothing else knows that name to find and drop it later. Matching + * on the constant prefix finds and drops any such leftovers before + * generating this run's own name. See test/helpers/find_test_schema.sql + * for how later, separate sessions rediscover the name this creates. + */ +SET count_nulls.test_schema_version = :'version'; + +DO $$ +BEGIN + IF current_setting('count_nulls.test_schema_version') = '' THEN + RAISE EXCEPTION ':version must be set explicitly - use ''current'' to install whatever the current default is, never an empty string, so an accidentally-unpropagated value fails loudly instead of silently installing ''current'' when something else was actually intended'; + END IF; +END +$$; + +DO $$ +DECLARE + r record; +BEGIN + FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP + EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); + END LOOP; +END +$$; + +SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema +\gset + +CREATE SCHEMA :"schema"; + +/* + * WITH SCHEMA targets the schema directly without touching search_path at + * all, so a successful install actually proves the install script itself + * doesn't need search_path arranged any particular way - the same + * qualification-correctness principle behind randomizing the schema name + * in the first place. Mutating search_path before CREATE EXTENSION + * instead would let the install succeed via a coincidentally arranged + * search_path, masking the extension's own install script secretly + * depending on unqualified name resolution during install. + */ +SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause +\gset + +CREATE EXTENSION count_nulls WITH SCHEMA :"schema":version_clause; diff --git a/test/helpers/find_test_schema.sql b/test/helpers/find_test_schema.sql new file mode 100644 index 0000000..1b1f2c4 --- /dev/null +++ b/test/helpers/find_test_schema.sql @@ -0,0 +1,35 @@ +/* + * Discovers the schema count_nulls was installed into for this test run - + * for scripts/sessions that didn't create it themselves and have no other + * way to know its (randomly generated) name. See + * test/helpers/create_test_schema.sql for how/why the name is randomized. + * + * If an anonymous DO block could return/populate a value back to the + * calling psql session, `SELECT ... INTO STRICT` would be the more natural + * way to write this check - Postgres's own built-in "expect exactly one + * row" enforcement, instead of manually counting rows and raising a custom + * exception. It can't (confirmed directly: a `\gset` following a DO block + * just silently re-executes the block's query text and captures nothing), + * so validation and the actual :test_schema capture below have to be two + * separate statements regardless, and the DO block falls back to a + * hand-counted check. That check is a hard failure, not a pgTAP-style + * assertion - + * finding zero or more than one match means something is broken (a + * previous run's schema was never cleaned up, or this ran before + * installation happened) and must abort immediately rather than silently + * guessing. + */ +DO $$ +DECLARE + v_count int := (SELECT count(*) FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'); +BEGIN + IF v_count <> 1 THEN + RAISE EXCEPTION + 'expected exactly one schema matching ''count_nulls test schema %%'', found %' + , v_count; + END IF; +END +$$; + +SELECT nspname AS test_schema FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' +\gset diff --git a/test/install/load.sql b/test/install/load.sql index 41fa1a8..5a4f12c 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -12,53 +12,6 @@ * from a textual comparison - matching cat_tools' test/install/load.sql. */ -/* - * TEST_SCHEMA (the count_nulls.test_schema GUC, set via the Makefile): - * which schema to install count_nulls into. Empty (the default) means - * "don't target any schema at all" - lands wherever this session's own - * default search_path resolves (ordinarily 'public', since test/install - * runs in its own bare connection, not the in-suite session pgTAP's - * tap_setup.sql runs in - see phase 1's commit message for why that - * matters). Non-empty explicitly creates that schema and targets it via - * CREATE EXTENSION ... WITH SCHEMA below - this file never mutates its - * own search_path to do so, and not because a one-shot bare connection - * wouldn't care about a leftover mutation either way: mutating - * search_path before CREATE EXTENSION would let the install succeed via - * a coincidentally arranged search_path, masking the extension's own - * install script secretly depending on unqualified name resolution - * during install. WITH SCHEMA targets the schema directly without - * touching search_path at all, so a successful install actually proves - * the install script itself doesn't need search_path arranged any - * particular way - the same qualification-correctness principle behind - * the whole TEST_SCHEMA axis (see core/functions.sql's header and - * test__check_ncs in sql/extension_tests.sql), just applied to the - * install step itself rather than to post-install test assertions. - * - * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, - * not be indistinguishable from a deliberately empty one. - */ -SELECT current_setting('count_nulls.test_schema') AS schema -\gset -SELECT :'schema' <> '' AS count_nulls_has_schema -\gset - -/* - * A reusable ' WITH SCHEMA "..."' fragment (leading space included, empty - * when count_nulls_has_schema is false) so CREATE EXTENSION below can just - * append :with_schema_clause without repeating the has-schema branch. - * format() is used unqualified: it's pg_catalog, always resolvable - * regardless of search_path. - */ -SELECT CASE WHEN :'count_nulls_has_schema' - THEN format(' WITH SCHEMA %I', :'schema') - ELSE '' - END AS with_schema_clause -\gset - -\if :count_nulls_has_schema -CREATE SCHEMA IF NOT EXISTS :"schema"; -\endif - /* * Mode selection: 'fresh' installs the current version directly; 'update' * installs the oldest version we still ship a full script for (0.9.6) and @@ -68,7 +21,8 @@ CREATE SCHEMA IF NOT EXISTS :"schema"; * 'existing' asserts count_nulls is already installed (a real `pg_upgrade` * run, external to this invocation) and touches nothing. * - * Read without missing_ok, same reasoning as count_nulls.test_schema above. + * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, + * not be indistinguishable from a deliberately empty one. */ SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_test_load_mode , current_setting('count_nulls.test_load_mode') = 'update' AS count_nulls_update_mode @@ -106,8 +60,17 @@ BEGIN END IF; END $$; -\elif :count_nulls_update_mode -CREATE EXTENSION count_nulls:with_schema_clause VERSION '0.9.6'; +\else +/* + * fresh/update: creation (fresh, randomly named schema; install at + * :version or current) is shared with bin/test_existing's prepare-old via + * test/helpers/create_test_schema.sql - see that file for the full + * rationale. 'update' additionally installs the oldest version we still + * ship a full script for, then upgrades it in place. + */ +\if :count_nulls_update_mode +\set version '0.9.6' +\i test/helpers/create_test_schema.sql /* * Suppress the "already installed, no update" NOTICE class of messages any * update script might emit. @@ -116,5 +79,7 @@ SET client_min_messages = WARNING; ALTER EXTENSION count_nulls UPDATE; SET client_min_messages = NOTICE; \else -CREATE EXTENSION count_nulls:with_schema_clause; +\set version 'current' +\i test/helpers/create_test_schema.sql +\endif \endif diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 1cd1bcb..bc049bd 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -6,52 +6,35 @@ /* * This file leaves search_path as functions.sql set it (_null_count_test, - * tap) - with an explicit TEST_SCHEMA, that keeps count_nulls' own schema - * off search_path, so every check below only passes if functions.sql's + * tap), and count_nulls always installs into its own freshly, randomly + * generated schema (see test/install/load.sql) - which is never on that + * search_path. So every check below only passes if functions.sql's * %I-qualified calls (via ncs()) are actually correct, never relying on - * count_nulls' own schema being reachable unqualified. When TEST_SCHEMA is - * empty, count_nulls lands in 'public' (test/install/load.sql runs in its - * own bare connection, with no schema targeting - see phase 1's commit - * message), which is NOT on search_path here either. + * count_nulls' own schema being reachable unqualified. * - * Excluding it from BOTH legs is stronger than strictly required: the thing - * that actually makes a multi-schema matrix meaningful is that AT LEAST ONE - * tested schema is verifiably off search_path (otherwise installing into two - * schemas that both happen to stay reachable would let an unqualified, - * resolve-by-accident reference pass every leg without ever being caught). - * Keeping both legs off search_path is a simpler, deliberately stricter - * choice here, not evidence that every leg must be - a hypothetical future - * leg that left its schema on search_path wouldn't invalidate this design, - * as long as at least one other leg still excludes it. - * - * schema_hint reads the count_nulls.test_schema GUC directly (the Makefile - * exports it via PGOPTIONS for the whole run - see test/install/load.sql, - * which installs into it) rather than via a psql variable relayed through - * test/deps.sql: nothing in this per-test session needs deps.sql to have - * set anything, since the GUC is readable from any session in the run. - * NULLIF turns the empty-TEST_SCHEMA case into NULL, and runtests() calls - * every test__* function with no arguments, so it always gets this default. + * This also means test/expected/extension_tests.out stays identical run to + * run regardless of which random name the schema gets: none of the pgTAP + * assertion descriptions below embed the actual schema name (see + * test/README.md's "Assertion descriptions deliberately never embed the + * schema name" section for the full rationale), so no numbered pg_regress + * alternate is ever needed for it. */ CREATE FUNCTION _null_count_test.test__check_ncs( - schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ DECLARE /* - * We either expect count_nulls' own schema to be in search_path, or we - * don't - and in this file we never do, in either leg: functions.sql - * unconditionally sets search_path to exclude it (see the header - * comment above), whether that's the empty leg's 'public' or the - * TEST_SCHEMA leg's known target. s is still real, independently - * determined content (via ncs() when there's no fixed target, via - * schema_hint when there is), so the membership check below genuinely - * exercises functions.sql's %I-qualification and load.sql's schema - * targeting - it isn't a tautology. + * We never expect count_nulls' own schema to be in search_path in this + * file: functions.sql unconditionally sets search_path to exclude it + * (see the header comment above). s is still real, independently + * determined content (via ncs()), so the membership check below + * genuinely exercises functions.sql's %I-qualification and load.sql's + * schema targeting - it isn't a tautology. * * SEE ALSO: teardown__search_path_unchanged in test/core/functions.sql, * which guards against some OTHER test mutating search_path mid-suite (a * different risk than this check). */ - s CONSTANT name = coalesce(schema_hint, ncs()); + s CONSTANT name = ncs(); BEGIN RETURN NEXT is( current_schemas(true) @> array[s] @@ -61,26 +44,23 @@ BEGIN END $body$; +/* + * Just asserts the extension can be dropped - doesn't attempt any schema + * cleanup itself. test/helpers/create_test_schema.sql already + * unconditionally drops any leftover 'count_nulls test schema %' schema + * before creating a fresh one on every single invocation. A second + * per-run cleanup here would only ever be redundant duplicate cleanup + * logic and never the thing that actually matters. (It's also moot in + * practice: this file's functions run inside a transaction pgxntool's + * test harness rolls back, never commits, so an explicit DROP SCHEMA here + * would never persist past this run anyway.) + */ CREATE FUNCTION _null_count_test.test__shutdown__drop_all( - schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ BEGIN RETURN NEXT lives_ok( $$DROP EXTENSION count_nulls$$ ); - - /* - * Plain cleanup, not a TAP assertion - dropping the schema TEST_SCHEMA - * created isn't something this suite is testing, just tearing down - * what it created. Same output in every TEST_SCHEMA leg: when - * schema_hint is NULL (empty leg), there's nothing to drop, so this is - * a no-op; if the DROP SCHEMA itself ever failed, the unhandled - * exception aborts the run loudly on its own - no lives_ok() needed - * for that. - */ - IF schema_hint IS NOT NULL THEN - EXECUTE format('DROP SCHEMA %I', schema_hint); - END IF; END $body$;