From f0a3893f1514e021d5cd115ac56977e9d4c77558 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 14:37:01 -0500 Subject: [PATCH 1/6] 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 881c252ab78865f9f9fde3f24abf04cfb40d30b1 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 4 Aug 2026 13:39:18 -0500 Subject: [PATCH 2/6] 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 fc63155857c2b0eeef83e00e27396fcb0717c111 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 14:37:43 -0500 Subject: [PATCH 3/6] 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 1de001b25d84db8520b42673a39c100cdff55efe Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 17:48:42 -0500 Subject: [PATCH 4/6] 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 0fac6a5cb025444c32712410212b7a98466ea04a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 17:56:59 -0500 Subject: [PATCH 5/6] 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 32e3429ce6849004130cf0fd3cfc3087bdc5d4b8 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 5 Aug 2026 18:15:33 -0500 Subject: [PATCH 6/6] 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