From 18f53e85040ac44b385e34736390e513f427f02f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 14:37:01 -0500 Subject: [PATCH 01/10] Phase 2: TEST_SCHEMA switching in test/install test/install/load.sql now reads the count_nulls.test_schema GUC (set via the Makefile's TEST_SCHEMA var, propagated via PGOPTIONS) to decide whether to CREATE SCHEMA/SET search_path before installing - empty means no targeting at all, non-empty explicitly targets that schema, matching TEST_SCHEMA=Quoted locally. test/sql/extension_tests.sql's test__check_ncs and test__shutdown__drop_all gain a schema_hint parameter (defaulted from the same GUC) so they can assert against a known target when TEST_SCHEMA is non-empty, and skip the schema-drop when it's empty (nothing to drop). This relies on test/core/functions.sql already producing schema-invariant assertion descriptions (see the prior PR) so a single test/expected/extension_tests.out still matches both legs. One unavoidable, genuine exception: test__shutdown__drop_all drops the schema TEST_SCHEMA created - real, different, correct behavior between "there's a schema to clean up" and "there isn't". Handled via pg_regress's native numbered-alternate mechanism - test/expected/extension_tests_1.out, captured from a real TEST_SCHEMA=Quoted run - documented in test/README.md's scenario writeup. Crossed the `test` CI job with TEST_SCHEMA={"", Quoted}. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 12 +++- Makefile | 19 +++++++ test/README.md | 78 ++++++++++++++++++++++++++ test/expected/extension_tests.out | 8 ++- test/expected/extension_tests_1.out | 87 +++++++++++++++++++++++++++++ test/install/load.sql | 23 ++++++++ test/sql/extension_tests.sql | 71 +++++++++++++++++------ 7 files changed, 277 insertions(+), 21 deletions(-) create mode 100644 test/README.md create mode 100644 test/expected/extension_tests_1.out diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5111af6..0d32069 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,13 +17,23 @@ jobs: # is what actually proves that works from a plain clone. run: make lint + # Fresh install, across the PG matrix AND a schema matrix (TEST_SCHEMA, + # picked up from the environment by test/install/load.sql via the + # count_nulls.test_schema GUC - see the Makefile). Empty ('') runs WITHOUT + # specifying a schema at all; 'Quoted' runs WITH one explicitly specified, + # using a name that requires SQL identifier quoting. Both legs matter and + # both pass against the SAME test/expected/extension_tests.out (see + # test/README.md for how the suite keeps its output schema-invariant). test: strategy: matrix: pg: [18, 17, 16, 15, 14, 13, 12, 11, 10] - name: 🐘 PostgreSQL ${{ matrix.pg }} + schema: ["", Quoted] + name: 🐘 PostgreSQL ${{ matrix.pg }} (schema ${{ matrix.schema == '' && 'none' || matrix.schema }}) runs-on: ubuntu-latest container: pgxn/pgxn-tools + env: + TEST_SCHEMA: ${{ matrix.schema }} steps: - name: Start PostgreSQL ${{ matrix.pg }} run: pg-start ${{ matrix.pg }} diff --git a/Makefile b/Makefile index 4386cc6..81d99a1 100644 --- a/Makefile +++ b/Makefile @@ -11,3 +11,22 @@ testdeps: $(wildcard test/*/*.sql) $(wildcard test/*.sql) # Be careful not to in # files are generated/derived from; those aren't relinted (see linter's DESIGN.md). 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/SET search_path to that name first - 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`. +# +# 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) diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000..4d87210 --- /dev/null +++ b/test/README.md @@ -0,0 +1,78 @@ +# count_nulls test suite + +This suite is structured differently from most pgTAP-based extension tests: +rather than each `test/sql/*.sql` file writing its own independent +assertions, `core/functions.sql` defines a shared library of `test__*` +functions (pgTAP's `runtests()` naming convention) that test files `\i` and +then invoke via `runtests()`. + +## Layout + +- `install/load.sql` — installs count_nulls once, committed, before the main + `test/sql/` schedule (see pgxntool/README.asc's `test/install` section). + Its own output isn't tracked (see `install/.gitignore`) - correctness + comes from this file failing loudly if something's wrong, not from a + textual comparison. +- `deps.sql` — loaded by every test file (via `load.sql` -> + `pgxntool/setup.sql` -> `deps.sql`). No longer installs count_nulls + itself (that's `install/load.sql`'s job); only for genuine per-test + dependency statements. +- `core/functions.sql` — a shared helper, `\i`'d by `sql/extension_tests.sql`. + Defines `ncs()` (discovers, live, which schema count_nulls is actually + installed in - never trusts a hardcoded/passed-in value) plus a battery of + `test__*` functions covering function definitions, immutability/ + strictness, and behavior across `anyarray`/`json`/`jsonb` and both + trigger functions. +- `sql/extension_tests.sql` — `\i`'s `core/functions.sql`, adds two more + `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()`. + +## TEST_SCHEMA + +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: + +- 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`/`SET search_path` to that name + first. `TEST_SCHEMA=Quoted` locally exercises a name requiring SQL + identifier quoting (mixed case - unquoted would fold to lowercase). + +Both legs run in CI - genuinely different code paths, not one a redundant +special case of the other. + +**Assertion descriptions deliberately never embed the schema name.** +`core/functions.sql`'s assertions build the SQL they *execute* via `%I` +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. + +**One unavoidable, genuine exception**: `test__shutdown__drop_all` drops +the schema TEST_SCHEMA created - a real, correct behavioral difference (not +an artifact) between "there's a schema to clean up" (non-empty) and "there +isn't" (empty, nothing to drop). `test/expected/extension_tests_1.out` is +`pg_regress`'s native numbered-alternate mechanism for exactly this: the +default file expects a `SKIP` there, `_1.out` (captured from a real +`TEST_SCHEMA=Quoted` run, never hand-authored) expects the schema actually +dropped. `pg_regress` tries the default first, then each numbered +alternate in turn, and passes if any one matches. + +## Regenerating expected output + +Never hand-edit files under `expected/`. Regenerate via `make results` +(guarded by `make verify-results`, which refuses to copy while +`regression.diffs` shows real failures - use +`PGXNTOOL_ENABLE_VERIFY_RESULTS=no` to bypass that guard for a run you've +already reviewed and know is a legitimate, intentional change, not a way to +skip reviewing the diff). `make results` only ever writes the unsuffixed +default; alternates (`_1.out`, ...) have to be copied by hand from a real +`test/results/.out` for that scenario. diff --git a/test/expected/extension_tests.out b/test/expected/extension_tests.out index a0cab9d..83991f5 100644 --- a/test/expected/extension_tests.out +++ b/test/expected/extension_tests.out @@ -1,7 +1,8 @@ \set ECHO none # Subtest: _null_count_test.test__check_ncs() - ok 1 - ncs() resolves to the schema count_nulls actually installed in - 1..1 + ok 1 + ok 2 - count_nulls' schema should not be in search path + 1..2 ok 1 - _null_count_test.test__check_ncs # Subtest: _null_count_test.test__definition() ok 1 - ensure null_count({anyarray}) is not in search_path @@ -80,6 +81,7 @@ ok 2 - _null_count_test.test__definition ok 3 - _null_count_test.test__functionality # Subtest: _null_count_test.test__shutdown__drop_all() ok 1 - 1..1 + ok 2 # SKIP TEST_SCHEMA is empty - no dedicated schema to drop + 1..2 ok 4 - _null_count_test.test__shutdown__drop_all 1..4 diff --git a/test/expected/extension_tests_1.out b/test/expected/extension_tests_1.out new file mode 100644 index 0000000..62ae915 --- /dev/null +++ b/test/expected/extension_tests_1.out @@ -0,0 +1,87 @@ +\set ECHO none +# Subtest: _null_count_test.test__check_ncs() + ok 1 + ok 2 - count_nulls' schema should not be in search path + 1..2 +ok 1 - _null_count_test.test__check_ncs +# Subtest: _null_count_test.test__definition() + ok 1 - ensure null_count({anyarray}) is not in search_path + ok 2 - Function null_count(anyarray) should return int + ok 3 - Function null_count(anyarray) should not be strict + ok 4 - Function null_count(anyarray) should be IMMUTABLE + ok 5 - ensure null_count({json}) is not in search_path + ok 6 - Function null_count(json) should return int + ok 7 - Function null_count(json) should not be strict + ok 8 - Function null_count(json) should be IMMUTABLE + ok 9 - ensure null_count({jsonb}) is not in search_path + ok 10 - Function null_count(jsonb) should return int + ok 11 - Function null_count(jsonb) should not be strict + ok 12 - Function null_count(jsonb) should be IMMUTABLE + ok 13 - ensure not_null_count({anyarray}) is not in search_path + ok 14 - Function not_null_count(anyarray) should return int + ok 15 - Function not_null_count(anyarray) should not be strict + ok 16 - Function not_null_count(anyarray) should be IMMUTABLE + ok 17 - ensure not_null_count({json}) is not in search_path + ok 18 - Function not_null_count(json) should return int + ok 19 - Function not_null_count(json) should not be strict + ok 20 - Function not_null_count(json) should be IMMUTABLE + ok 21 - ensure not_null_count({jsonb}) is not in search_path + ok 22 - Function not_null_count(jsonb) should return int + ok 23 - Function not_null_count(jsonb) should not be strict + ok 24 - Function not_null_count(jsonb) should be IMMUTABLE + ok 25 - Function null_count_trigger() should return trigger + ok 26 - Function null_count_trigger() should not be strict + ok 27 - Function null_count_trigger() should be IMMUTABLE + ok 28 - Function not_null_count_trigger() should return trigger + ok 29 - Function not_null_count_trigger() should not be strict + ok 30 - Function not_null_count_trigger() should be IMMUTABLE + 1..30 +ok 2 - _null_count_test.test__definition +# Subtest: _null_count_test.test__functionality() + ok 1 - Test null_count(a, b, c) + ok 2 - Test null_count(json) + ok 3 - Test null_count(jsonb) + ok 4 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger( NULL ) + ok 5 - Test not_null_count_trigger( NULL ) + ok 6 - DROP TRIGGER "test trigger" + ok 7 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger( ) + ok 8 - Test not_null_count_trigger( ) + ok 9 - DROP TRIGGER "test trigger" + ok 10 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger( NULL ) + ok 11 - Test null_count_trigger( NULL ) + ok 12 - DROP TRIGGER "test trigger" + ok 13 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger( ) + ok 14 - Test null_count_trigger( ) + ok 15 - DROP TRIGGER "test trigger" + ok 16 - CREATE TRIGGER "null_BEFORE_error_message" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, 'error_message') + ok 17 - Test "null_BEFORE_error_message" + ok 18 - DROP TRIGGER "null_BEFORE_error_message" + ok 19 - CREATE TRIGGER "null_AFTER_error_message" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, 'error_message') + ok 20 - Test "null_AFTER_error_message" + ok 21 - DROP TRIGGER "null_AFTER_error_message" + ok 22 - CREATE TRIGGER "not_null_BEFORE_error_message" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, 'error_message') + ok 23 - Test "not_null_BEFORE_error_message" + ok 24 - DROP TRIGGER "not_null_BEFORE_error_message" + ok 25 - CREATE TRIGGER "not_null_AFTER_error_message" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, 'error_message') + ok 26 - Test "not_null_AFTER_error_message" + ok 27 - DROP TRIGGER "not_null_AFTER_error_message" + ok 28 - CREATE TRIGGER "null_BEFORE_" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, NULL) + ok 29 - Test "null_BEFORE_" + ok 30 - DROP TRIGGER "null_BEFORE_" + ok 31 - CREATE TRIGGER "null_AFTER_" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, NULL) + ok 32 - Test "null_AFTER_" + ok 33 - DROP TRIGGER "null_AFTER_" + ok 34 - CREATE TRIGGER "not_null_BEFORE_" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, NULL) + ok 35 - Test "not_null_BEFORE_" + ok 36 - DROP TRIGGER "not_null_BEFORE_" + ok 37 - CREATE TRIGGER "not_null_AFTER_" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, NULL) + ok 38 - Test "not_null_AFTER_" + ok 39 - DROP TRIGGER "not_null_AFTER_" + 1..39 +ok 3 - _null_count_test.test__functionality +# Subtest: _null_count_test.test__shutdown__drop_all() + ok 1 + ok 2 + 1..2 +ok 4 - _null_count_test.test__shutdown__drop_all +1..4 diff --git a/test/install/load.sql b/test/install/load.sql index 19883c0..031eafc 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -11,4 +11,27 @@ * file failing loudly (aborting the session) if something's wrong, not * 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 and targets that schema. + * + * 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 + +\if :count_nulls_has_schema +CREATE SCHEMA IF NOT EXISTS :"schema"; +SET search_path = :"schema"; +\endif + CREATE EXTENSION count_nulls; diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 0ea7590..80e1cac 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -5,35 +5,72 @@ \i test/core/functions.sql /* - * count_nulls is installed by test/install/load.sql with no schema - * targeting - it lands wherever the session's own search_path resolves at - * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on - * search_path first by test/pgxntool/tap_setup.sql). This just proves - * ncs() actually resolves to something real; a future TEST_SCHEMA switch - * (see pgxntool/README.asc's U&U section) would let this assert an exact, - * known location instead. + * 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 + * %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. * - * 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). + * 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. */ -CREATE FUNCTION _null_count_test.test__check_ncs -() RETURNS SETOF text LANGUAGE plpgsql AS $body$ +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 + /* + * When TEST_SCHEMA is non-empty we know exactly where count_nulls + * should be, so compare ncs() against that known value - a real + * assertion. When it's empty (schema_hint is NULL), there's no fixed + * expectation (it lands in 'public', an artifact of test/install's own + * bare connection - not something this test should hardcode), so fall + * back to ncs() itself: a no-op comparison that still exercises the + * call, without asserting a location this file has no business + * assuming. + */ + s CONSTANT name = COALESCE(schema_hint, ncs()); BEGIN - RETURN NEXT isnt( + RETURN NEXT is( ncs() - , NULL - , 'ncs() resolves to the schema count_nulls actually installed in' + , s + ); + RETURN NEXT is( + current_schemas(true) @> array[s] + , false + , 'count_nulls'' schema should not be in search path' ); END $body$; -CREATE FUNCTION _null_count_test.test__shutdown__drop_all -() RETURNS SETOF text LANGUAGE plpgsql AS $body$ +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$$ ); + + /* + * Only try to drop a schema when TEST_SCHEMA actually created one - + * when it's empty (schema_hint is NULL), count_nulls lives in 'public' + * (see test/install/load.sql), which this file has no business + * dropping. + */ + IF schema_hint IS NOT NULL THEN + RETURN NEXT lives_ok( + format('DROP SCHEMA %I', schema_hint) + ); + ELSE + RETURN NEXT skip('TEST_SCHEMA is empty - no dedicated schema to drop'); + END IF; END $body$; From 5fd17d28cfd1de97f5b40c54e9f5965391f8393d Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 4 Aug 2026 13:39:18 -0500 Subject: [PATCH 02/10] Collapse TEST_SCHEMA out of the CI matrix into a make-level loop Per a review pass against Postgres-Extensions/cat_tools' own experience (test-fixes.md item 6, found while syncing that repo to pgxntool 2.3.0): a schema name is just an input value the SAME assertions run against in the SAME environment, not a real isolation/environment boundary (unlike PostgreSQL major - different binaries, different container - or pg_tle deployment, which must never share a runner with a filesystem install). Crossing it into the CI matrix only multiplies job count for zero added confidence per dollar. Added test-schema-all: loops TEST_SCHEMA through every value via sequential recursive $(MAKE) calls (same pattern test-update already uses for the load-mode axis), exit 1 on the first failure so a later iteration can't mask an earlier one, each iteration echoed so a failure's TEST_SCHEMA value is still directly attributable without a separate CI check name per value. The `test` CI job now calls this once per PostgreSQL major instead of crossing pg x schema - job count for this job drops back to one per PG major. Verified locally against PG17: both TEST_SCHEMA values pass via `make test-schema-all`. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 26 +++++++++++++------------- Makefile | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d32069..4dbfdb7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,30 +17,30 @@ jobs: # is what actually proves that works from a plain clone. run: make lint - # Fresh install, across the PG matrix AND a schema matrix (TEST_SCHEMA, - # picked up from the environment by test/install/load.sql via the - # count_nulls.test_schema GUC - see the Makefile). Empty ('') runs WITHOUT - # specifying a schema at all; 'Quoted' runs WITH one explicitly specified, - # using a name that requires SQL identifier quoting. Both legs matter and - # both pass against the SAME test/expected/extension_tests.out (see - # test/README.md for how the suite keeps its output schema-invariant). + # Fresh install, across the 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). 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: strategy: matrix: pg: [18, 17, 16, 15, 14, 13, 12, 11, 10] - schema: ["", Quoted] - name: 🐘 PostgreSQL ${{ matrix.pg }} (schema ${{ matrix.schema == '' && 'none' || matrix.schema }}) + name: 🐘 PostgreSQL ${{ matrix.pg }} runs-on: ubuntu-latest container: pgxn/pgxn-tools - env: - TEST_SCHEMA: ${{ matrix.schema }} steps: - name: Start PostgreSQL ${{ matrix.pg }} run: pg-start ${{ matrix.pg }} - name: Check out the repo uses: actions/checkout@v4 - - name: Test on PostgreSQL ${{ matrix.pg }} - run: pg-build-test + - name: Test on PostgreSQL ${{ matrix.pg }}, across every TEST_SCHEMA value + run: make test-schema-all pg-tle-test: strategy: diff --git a/Makefile b/Makefile index 81d99a1..89e001d 100644 --- a/Makefile +++ b/Makefile @@ -30,3 +30,30 @@ include lint.mk # 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 and CI (once collapsed - see the "why not a CI matrix" +# note below) can't silently drift onto different sets. +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 From 4713b01f24ff70879fb2404528055f9b8431a12c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 14:37:43 -0500 Subject: [PATCH 03/10] test__check_ncs: single unconditional assertion, not two skip()'d branches Per review: we either expect the schema count_nulls is installed into 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 count_nulls' own schema (see the header comment above), whether that's the empty leg's 'public' or the TEST_SCHEMA leg's known target - so the membership check's expected value is unconditionally false, not conditional on schema_hint at all. Drops the previous skip()-based two-branch design (which only ran a real assertion in the TEST_SCHEMA leg and skipped it entirely in the empty leg) and the is(ncs(), s) check (a separate "did TEST_SCHEMA target correctly" concern, not a "is it in search_path" concern). s is still real, independently-determined content (via ncs() when there's no fixed target, via schema_hint when there is), so this isn't a tautology - it fails for real if functions.sql's search_path exclusion or load.sql's schema targeting ever breaks. Regenerated test/expected/extension_tests.out and _1.out via make results for both TEST_SCHEMA legs (empty, Quoted) - verified via make test-schema-all. Co-Authored-By: Claude Sonnet 5 --- test/expected/extension_tests.out | 5 ++--- test/expected/extension_tests_1.out | 5 ++--- test/sql/extension_tests.sql | 21 +++++++++------------ 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/test/expected/extension_tests.out b/test/expected/extension_tests.out index 83991f5..73e1a05 100644 --- a/test/expected/extension_tests.out +++ b/test/expected/extension_tests.out @@ -1,8 +1,7 @@ \set ECHO none # Subtest: _null_count_test.test__check_ncs() - ok 1 - ok 2 - count_nulls' schema should not be in search path - 1..2 + ok 1 - count_nulls' schema should not be in search path + 1..1 ok 1 - _null_count_test.test__check_ncs # Subtest: _null_count_test.test__definition() ok 1 - ensure null_count({anyarray}) is not in search_path diff --git a/test/expected/extension_tests_1.out b/test/expected/extension_tests_1.out index 62ae915..c43e443 100644 --- a/test/expected/extension_tests_1.out +++ b/test/expected/extension_tests_1.out @@ -1,8 +1,7 @@ \set ECHO none # Subtest: _null_count_test.test__check_ncs() - ok 1 - ok 2 - count_nulls' schema should not be in search path - 1..2 + ok 1 - count_nulls' schema should not be in search path + 1..1 ok 1 - _null_count_test.test__check_ncs # Subtest: _null_count_test.test__definition() ok 1 - ensure null_count({anyarray}) is not in search_path diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 80e1cac..3f0058f 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -27,21 +27,18 @@ CREATE FUNCTION _null_count_test.test__check_ncs( ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ DECLARE /* - * When TEST_SCHEMA is non-empty we know exactly where count_nulls - * should be, so compare ncs() against that known value - a real - * assertion. When it's empty (schema_hint is NULL), there's no fixed - * expectation (it lands in 'public', an artifact of test/install's own - * bare connection - not something this test should hardcode), so fall - * back to ncs() itself: a no-op comparison that still exercises the - * call, without asserting a location this file has no business - * assuming. + * 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. */ s CONSTANT name = COALESCE(schema_hint, ncs()); BEGIN - RETURN NEXT is( - ncs() - , s - ); RETURN NEXT is( current_schemas(true) @> array[s] , false From fdb46cbad97b93fc4ce8d1fe404b144bb46c294c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 17:48:42 -0500 Subject: [PATCH 04/10] Cross-reference test__check_ncs with teardown__search_path_unchanged Document why these two search_path checks aren't redundant: this one doesn't guard against some other test mutating search_path mid-suite. Co-Authored-By: Claude Sonnet 5 --- test/sql/extension_tests.sql | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 3f0058f..ce60b28 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -35,7 +35,9 @@ DECLARE * 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. + * targeting - it isn't a tautology. Doesn't guard against some OTHER + * test mutating search_path mid-suite - see + * teardown__search_path_unchanged in test/core/functions.sql for that. */ s CONSTANT name = COALESCE(schema_hint, ncs()); BEGIN From 475ed323048eb5dec439f62abddbb4017c6282be Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 17:56:59 -0500 Subject: [PATCH 05/10] Make the test__check_ncs/search_path cross-reference a SEE ALSO paragraph The prior cross-reference was folded into the end of an existing sentence, easy to miss when skimming the block; call it out as its own labeled paragraph instead. Co-Authored-By: Claude Sonnet 5 --- test/sql/extension_tests.sql | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index ce60b28..db06342 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -35,9 +35,11 @@ DECLARE * 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. Doesn't guard against some OTHER - * test mutating search_path mid-suite - see - * teardown__search_path_unchanged in test/core/functions.sql for that. + * 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()); BEGIN From 0b3a607eb5ed3e139cfc4bce846cd554a38d3b9d Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 18:15:33 -0500 Subject: [PATCH 06/10] test/README.md: point deps.sql entry at its own header comment deps.sql now carries a fuller explanation of why it's empty and kept; avoid duplicating that here. Co-Authored-By: Claude Sonnet 5 --- test/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/README.md b/test/README.md index 4d87210..48885fc 100644 --- a/test/README.md +++ b/test/README.md @@ -16,7 +16,8 @@ then invoke via `runtests()`. - `deps.sql` — loaded by every test file (via `load.sql` -> `pgxntool/setup.sql` -> `deps.sql`). No longer installs count_nulls itself (that's `install/load.sql`'s job); only for genuine per-test - dependency statements. + dependency statements. Currently empty - see its own header comment for + why it's kept that way rather than deleted. - `core/functions.sql` — a shared helper, `\i`'d by `sql/extension_tests.sql`. Defines `ncs()` (discovers, live, which schema count_nulls is actually installed in - never trusts a hardcoded/passed-in value) plus a battery of From 9a99aff3065213640bcb2df0fdab794673709749 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 14:02:39 -0500 Subject: [PATCH 07/10] test__shutdown__drop_all: plain cleanup instead of skip()'d assertion The schema drop was a lives_ok()/skip() branch purely to make the TAP output type-check across both TEST_SCHEMA legs, forcing a second pg_regress expected-output file (extension_tests_1.out) whose only difference was that one row. Dropping the schema is teardown, not a behavior under test, so make it plain EXECUTE'd SQL with no pgTAP-visible output - both legs now produce byte-identical TAP output, so the numbered alternate is gone and test/README.md's description of it is updated to match. --- test/README.md | 17 +++--- test/expected/extension_tests.out | 3 +- test/expected/extension_tests_1.out | 86 ----------------------------- test/sql/extension_tests.sql | 17 +++--- 4 files changed, 17 insertions(+), 106 deletions(-) delete mode 100644 test/expected/extension_tests_1.out diff --git a/test/README.md b/test/README.md index 48885fc..0d58c49 100644 --- a/test/README.md +++ b/test/README.md @@ -57,15 +57,14 @@ 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. -**One unavoidable, genuine exception**: `test__shutdown__drop_all` drops -the schema TEST_SCHEMA created - a real, correct behavioral difference (not -an artifact) between "there's a schema to clean up" (non-empty) and "there -isn't" (empty, nothing to drop). `test/expected/extension_tests_1.out` is -`pg_regress`'s native numbered-alternate mechanism for exactly this: the -default file expects a `SKIP` there, `_1.out` (captured from a real -`TEST_SCHEMA=Quoted` run, never hand-authored) expects the schema actually -dropped. `pg_regress` tries the default first, then each numbered -alternate in turn, and passes if any one matches. +**`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. ## Regenerating expected output diff --git a/test/expected/extension_tests.out b/test/expected/extension_tests.out index 73e1a05..8913db9 100644 --- a/test/expected/extension_tests.out +++ b/test/expected/extension_tests.out @@ -80,7 +80,6 @@ ok 2 - _null_count_test.test__definition ok 3 - _null_count_test.test__functionality # Subtest: _null_count_test.test__shutdown__drop_all() ok 1 - ok 2 # SKIP TEST_SCHEMA is empty - no dedicated schema to drop - 1..2 + 1..1 ok 4 - _null_count_test.test__shutdown__drop_all 1..4 diff --git a/test/expected/extension_tests_1.out b/test/expected/extension_tests_1.out deleted file mode 100644 index c43e443..0000000 --- a/test/expected/extension_tests_1.out +++ /dev/null @@ -1,86 +0,0 @@ -\set ECHO none -# Subtest: _null_count_test.test__check_ncs() - ok 1 - count_nulls' schema should not be in search path - 1..1 -ok 1 - _null_count_test.test__check_ncs -# Subtest: _null_count_test.test__definition() - ok 1 - ensure null_count({anyarray}) is not in search_path - ok 2 - Function null_count(anyarray) should return int - ok 3 - Function null_count(anyarray) should not be strict - ok 4 - Function null_count(anyarray) should be IMMUTABLE - ok 5 - ensure null_count({json}) is not in search_path - ok 6 - Function null_count(json) should return int - ok 7 - Function null_count(json) should not be strict - ok 8 - Function null_count(json) should be IMMUTABLE - ok 9 - ensure null_count({jsonb}) is not in search_path - ok 10 - Function null_count(jsonb) should return int - ok 11 - Function null_count(jsonb) should not be strict - ok 12 - Function null_count(jsonb) should be IMMUTABLE - ok 13 - ensure not_null_count({anyarray}) is not in search_path - ok 14 - Function not_null_count(anyarray) should return int - ok 15 - Function not_null_count(anyarray) should not be strict - ok 16 - Function not_null_count(anyarray) should be IMMUTABLE - ok 17 - ensure not_null_count({json}) is not in search_path - ok 18 - Function not_null_count(json) should return int - ok 19 - Function not_null_count(json) should not be strict - ok 20 - Function not_null_count(json) should be IMMUTABLE - ok 21 - ensure not_null_count({jsonb}) is not in search_path - ok 22 - Function not_null_count(jsonb) should return int - ok 23 - Function not_null_count(jsonb) should not be strict - ok 24 - Function not_null_count(jsonb) should be IMMUTABLE - ok 25 - Function null_count_trigger() should return trigger - ok 26 - Function null_count_trigger() should not be strict - ok 27 - Function null_count_trigger() should be IMMUTABLE - ok 28 - Function not_null_count_trigger() should return trigger - ok 29 - Function not_null_count_trigger() should not be strict - ok 30 - Function not_null_count_trigger() should be IMMUTABLE - 1..30 -ok 2 - _null_count_test.test__definition -# Subtest: _null_count_test.test__functionality() - ok 1 - Test null_count(a, b, c) - ok 2 - Test null_count(json) - ok 3 - Test null_count(jsonb) - ok 4 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger( NULL ) - ok 5 - Test not_null_count_trigger( NULL ) - ok 6 - DROP TRIGGER "test trigger" - ok 7 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger( ) - ok 8 - Test not_null_count_trigger( ) - ok 9 - DROP TRIGGER "test trigger" - ok 10 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger( NULL ) - ok 11 - Test null_count_trigger( NULL ) - ok 12 - DROP TRIGGER "test trigger" - ok 13 - CREATE TRIGGER "test trigger" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger( ) - ok 14 - Test null_count_trigger( ) - ok 15 - DROP TRIGGER "test trigger" - ok 16 - CREATE TRIGGER "null_BEFORE_error_message" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, 'error_message') - ok 17 - Test "null_BEFORE_error_message" - ok 18 - DROP TRIGGER "null_BEFORE_error_message" - ok 19 - CREATE TRIGGER "null_AFTER_error_message" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, 'error_message') - ok 20 - Test "null_AFTER_error_message" - ok 21 - DROP TRIGGER "null_AFTER_error_message" - ok 22 - CREATE TRIGGER "not_null_BEFORE_error_message" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, 'error_message') - ok 23 - Test "not_null_BEFORE_error_message" - ok 24 - DROP TRIGGER "not_null_BEFORE_error_message" - ok 25 - CREATE TRIGGER "not_null_AFTER_error_message" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, 'error_message') - ok 26 - Test "not_null_AFTER_error_message" - ok 27 - DROP TRIGGER "not_null_AFTER_error_message" - ok 28 - CREATE TRIGGER "null_BEFORE_" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, NULL) - ok 29 - Test "null_BEFORE_" - ok 30 - DROP TRIGGER "null_BEFORE_" - ok 31 - CREATE TRIGGER "null_AFTER_" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE null_count_trigger(1, NULL) - ok 32 - Test "null_AFTER_" - ok 33 - DROP TRIGGER "null_AFTER_" - ok 34 - CREATE TRIGGER "not_null_BEFORE_" BEFORE INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, NULL) - ok 35 - Test "not_null_BEFORE_" - ok 36 - DROP TRIGGER "not_null_BEFORE_" - ok 37 - CREATE TRIGGER "not_null_AFTER_" AFTER INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE not_null_count_trigger(1, NULL) - ok 38 - Test "not_null_AFTER_" - ok 39 - DROP TRIGGER "not_null_AFTER_" - 1..39 -ok 3 - _null_count_test.test__functionality -# Subtest: _null_count_test.test__shutdown__drop_all() - ok 1 - ok 2 - 1..2 -ok 4 - _null_count_test.test__shutdown__drop_all -1..4 diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index db06342..943afcb 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -60,17 +60,16 @@ BEGIN ); /* - * Only try to drop a schema when TEST_SCHEMA actually created one - - * when it's empty (schema_hint is NULL), count_nulls lives in 'public' - * (see test/install/load.sql), which this file has no business - * dropping. + * 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 - RETURN NEXT lives_ok( - format('DROP SCHEMA %I', schema_hint) - ); - ELSE - RETURN NEXT skip('TEST_SCHEMA is empty - no dedicated schema to drop'); + EXECUTE format('DROP SCHEMA %I', schema_hint); END IF; END $body$; From eb91937566aece33979798a924b2cd85bea8ea3c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 14:14:10 -0500 Subject: [PATCH 08/10] test: clarify why >1 TEST_SCHEMA leg is meaningful (search_path exclusion) Comments described the schema matrix mechanically (install into schema A vs schema B) without stating the actual load-bearing requirement: at least one leg must have its schema verifiably off search_path, or an extension full of unqualified references would pass every leg anyway. Clarify in the Makefile's TEST_SCHEMA/TEST_SCHEMA_VALUES comments, test/README.md's TEST_SCHEMA section, and extension_tests.sql's header that excluding search_path in every leg (this suite's actual behavior) is a stronger-than-necessary, deliberate choice, not evidence that every leg must exclude it. --- Makefile | 13 ++++++++++++- test/README.md | 14 ++++++++++++++ test/sql/extension_tests.sql | 10 ++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 89e001d..760b35f 100644 --- a/Makefile +++ b/Makefile @@ -23,6 +23,15 @@ include lint.mk # 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 @@ -33,7 +42,9 @@ 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 and CI (once collapsed - see the "why not a CI matrix" -# note below) can't silently drift onto different sets. +# note below) 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 diff --git a/test/README.md b/test/README.md index 0d58c49..34d5daa 100644 --- a/test/README.md +++ b/test/README.md @@ -48,6 +48,20 @@ installs count_nulls into: Both legs run in CI - genuinely different code paths, not one a redundant special case of the other. +**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. + **Assertion descriptions deliberately never embed the schema name.** `core/functions.sql`'s assertions build the SQL they *execute* via `%I` qualification (through `ncs()`, so they're always correct no matter which diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 943afcb..7a303bd 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -14,6 +14,16 @@ * own bare connection, with no schema targeting - see phase 1's commit * message), which is NOT on search_path here either. * + * 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 From e5746a75c026ed86b58c9492399bedf77fecf512 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 15:42:19 -0500 Subject: [PATCH 09/10] test/install/load.sql: target schema via WITH SCHEMA, not SET search_path CREATE EXTENSION ... WITH SCHEMA targets a schema directly with no session-level side effect, so this file's own bare connection never needs to mutate its own search_path to land count_nulls in TEST_SCHEMA. Compute the WITH SCHEMA clause once (right after count_nulls_has_schema) so both CREATE EXTENSION call sites can reuse it without duplicating the has-schema branch. --- Makefile | 9 +++++---- test/README.md | 8 +++++--- test/install/load.sql | 22 +++++++++++++++++++--- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index 760b35f..7a0a979 100644 --- a/Makefile +++ b/Makefile @@ -18,10 +18,11 @@ include lint.mk # # 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/SET search_path to that name first - 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`. +# 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 diff --git a/test/README.md b/test/README.md index 34d5daa..3ae3a96 100644 --- a/test/README.md +++ b/test/README.md @@ -41,9 +41,11 @@ installs count_nulls into: 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`/`SET search_path` to that name - first. `TEST_SCHEMA=Quoted` locally exercises a name requiring SQL - identifier quoting (mixed case - unquoted would fold to lowercase). +- 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). Both legs run in CI - genuinely different code paths, not one a redundant special case of the other. diff --git a/test/install/load.sql b/test/install/load.sql index 031eafc..7916805 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -19,7 +19,11 @@ * 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 and targets that schema. + * 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 (there'd be no point: WITH SCHEMA already + * targets the schema directly, and this is a one-shot bare connection + * with no later statement here that would need search_path set). * * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, * not be indistinguishable from a deliberately empty one. @@ -29,9 +33,21 @@ SELECT current_setting('count_nulls.test_schema') AS schema 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"; -SET search_path = :"schema"; \endif -CREATE EXTENSION count_nulls; +CREATE EXTENSION count_nulls:with_schema_clause; From 1b7408cc19431abf71aa1eaa704db87fa24241f4 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 6 Aug 2026 16:09:44 -0500 Subject: [PATCH 10/10] Fix wrong search_path rationale, function-call casing, and quote-escape style test/install/load.sql's TEST_SCHEMA comment claimed the file skips mutating search_path because a one-shot bare connection wouldn't need it set later - that's not why. The real reason: 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. WITH SCHEMA avoids that by targeting the schema directly without touching search_path at all - the same qualification-correctness principle the whole TEST_SCHEMA axis is built on, applied to the install step itself. Also lowercase NULLIF/COALESCE calls in test/sql/extension_tests.sql to match the rest of the suite's function-call casing convention, and switch the 'count_nulls'' schema...' description to dollar-quoting to avoid the doubled single-quote escape. --- test/install/load.sql | 15 ++++++++++++--- test/sql/extension_tests.sql | 8 ++++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/test/install/load.sql b/test/install/load.sql index 7916805..37db853 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -21,9 +21,18 @@ * 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 (there'd be no point: WITH SCHEMA already - * targets the schema directly, and this is a one-shot bare connection - * with no later statement here that would need search_path set). + * 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. diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 7a303bd..1cd1bcb 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -33,7 +33,7 @@ * every test__* function with no arguments, so it always gets this default. */ CREATE FUNCTION _null_count_test.test__check_ncs( - schema_hint name DEFAULT NULLIF(current_setting('count_nulls.test_schema'), '')::name + schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ DECLARE /* @@ -51,18 +51,18 @@ DECLARE * 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 = coalesce(schema_hint, ncs()); BEGIN RETURN NEXT is( current_schemas(true) @> array[s] , false - , 'count_nulls'' schema should not be in search path' + , $$count_nulls' schema should not be in search path$$ ); END $body$; CREATE FUNCTION _null_count_test.test__shutdown__drop_all( - schema_hint name DEFAULT NULLIF(current_setting('count_nulls.test_schema'), '')::name + schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ BEGIN RETURN NEXT lives_ok(