TEST_SCHEMA switching in test/install

.github/workflows/ci.yml

@@ -17,6 +17,16 @@ jobs:
         # is what actually proves that works from a plain clone.
         run: make lint
 
+  # 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:
@@ -29,8 +39,8 @@ jobs:
         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:

Makefile

@@ -11,3 +11,49 @@ 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)
+
+# 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

test/README.md

@@ -0,0 +1,79 @@
+# 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. 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
+  `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/<test>.out` for that scenario.

test/expected/extension_tests.out

@@ -1,6 +1,6 @@
 \set ECHO none
 # Subtest: _null_count_test.test__check_ncs()
-    ok 1 - ncs() resolves to the schema count_nulls actually installed in
+    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()
@@ -80,6 +80,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

test/expected/extension_tests_1.out

@@ -0,0 +1,86 @@
+\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

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;

test/sql/extension_tests.sql

@@ -5,35 +5,73 @@
 \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
+    /*
+     * 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.
+     *
+     * 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
-    RETURN NEXT isnt(
-        ncs()
-        , NULL
-        , 'ncs() resolves to the schema count_nulls actually installed in'
+    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$;