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,61 @@ 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, then CREATE EXTENSION ... WITH SCHEMA that name -
+# including a name that requires SQL identifier quoting (mixed case -
+# unquoted would fold to lowercase), to exercise the suite's %I
+# schema-qualification rather than just its literal test data. Locally:
+# `make test TEST_SCHEMA=Quoted`.
+#
+# Installing into two schemas only proves that %I-qualification works if the
+# test session's search_path never includes count_nulls' own schema in at
+# least one of those legs - otherwise an extension full of unqualified,
+# resolve-by-accident references would pass every leg too (see
+# test/core/functions.sql's header and test__check_ncs in
+# test/sql/extension_tests.sql, which is what actually checks this). This
+# suite excludes it in BOTH legs, which is stronger than the minimum needed -
+# not a requirement in itself.
+#
+# Propagated as a GUC (count_nulls.test_schema), exported unconditionally via
+# PGOPTIONS - pg_regress doesn't forward make variables, but the psql
+# processes it spawns inherit the environment. Empty is a valid, deliberate
+# value (not an error) - read without missing_ok, so a truly unpropagated GUC
+# still fails loudly instead of looking identical to a deliberately empty one.
+TEST_SCHEMA ?=
+export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_schema=$(TEST_SCHEMA)
+
+# Every TEST_SCHEMA value the suite is tested against. A single source so
+# test-schema-all and CI (once collapsed - see the "why not a CI matrix"
+# 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
+# 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,94 @@
+# 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`, 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.
+
+**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
+real schema count_nulls landed in) but pass an *explicit*, schema-free
+description to every pgTAP call - overriding pgTAP's own auto-generated
+descriptions, which otherwise embed the schema. This is what keeps
+`test/expected/extension_tests.out` a single file that both TEST_SCHEMA
+legs pass against, instead of needing one file per schema value.
+
+**`test__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
+
+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()

test/install/load.sql

@@ -11,4 +11,52 @@
  * file failing loudly (aborting the session) if something's wrong, not
  * from a textual comparison - matching cat_tools' test/install/load.sql.
  */
-CREATE EXTENSION count_nulls;
+
+/*
+ * TEST_SCHEMA (the count_nulls.test_schema GUC, set via the Makefile):
+ * which schema to install count_nulls into. Empty (the default) means
+ * "don't target any schema at all" - lands wherever this session's own
+ * default search_path resolves (ordinarily 'public', since test/install
+ * runs in its own bare connection, not the in-suite session pgTAP's
+ * tap_setup.sql runs in - see phase 1's commit message for why that
+ * matters). Non-empty explicitly creates that schema and targets it via
+ * CREATE EXTENSION ... WITH SCHEMA below - this file never mutates its
+ * own search_path to do so, and not because a one-shot bare connection
+ * wouldn't care about a leftover mutation either way: mutating
+ * search_path before CREATE EXTENSION would let the install succeed via
+ * a coincidentally arranged search_path, masking the extension's own
+ * install script secretly depending on unqualified name resolution
+ * during install. WITH SCHEMA targets the schema directly without
+ * touching search_path at all, so a successful install actually proves
+ * the install script itself doesn't need search_path arranged any
+ * particular way - the same qualification-correctness principle behind
+ * the whole TEST_SCHEMA axis (see core/functions.sql's header and
+ * test__check_ncs in sql/extension_tests.sql), just applied to the
+ * install step itself rather than to post-install test assertions.
+ *
+ * Read without missing_ok: a genuinely unpropagated GUC must fail loudly,
+ * not be indistinguishable from a deliberately empty one.
+ */
+SELECT current_setting('count_nulls.test_schema') AS schema
+\gset
+SELECT :'schema' <> '' AS count_nulls_has_schema
+\gset
+
+/*
+ * A reusable ' WITH SCHEMA "..."' fragment (leading space included, empty
+ * when count_nulls_has_schema is false) so CREATE EXTENSION below can just
+ * append :with_schema_clause without repeating the has-schema branch.
+ * format() is used unqualified: it's pg_catalog, always resolvable
+ * regardless of search_path.
+ */
+SELECT CASE WHEN :'count_nulls_has_schema'
+              THEN format(' WITH SCHEMA %I', :'schema')
+              ELSE ''
+       END AS with_schema_clause
+\gset
+
+\if :count_nulls_has_schema
+CREATE SCHEMA IF NOT EXISTS :"schema";
+\endif
+
+CREATE EXTENSION count_nulls:with_schema_clause;

test/sql/extension_tests.sql

@@ -5,35 +5,82 @@
 \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).
+ * Excluding it from BOTH legs is stronger than strictly required: the thing
+ * that actually makes a multi-schema matrix meaningful is that AT LEAST ONE
+ * tested schema is verifiably off search_path (otherwise installing into two
+ * schemas that both happen to stay reachable would let an unqualified,
+ * resolve-by-accident reference pass every leg without ever being caught).
+ * Keeping both legs off search_path is a simpler, deliberately stricter
+ * choice here, not evidence that every leg must be - a hypothetical future
+ * leg that left its schema on search_path wouldn't invalidate this design,
+ * as long as at least one other leg still excludes it.
+ *
+ * schema_hint reads the count_nulls.test_schema GUC directly (the Makefile
+ * exports it via PGOPTIONS for the whole run - see test/install/load.sql,
+ * which installs into it) rather than via a psql variable relayed through
+ * test/deps.sql: nothing in this per-test session needs deps.sql to have
+ * set anything, since the GUC is readable from any session in the run.
+ * NULLIF turns the empty-TEST_SCHEMA case into NULL, and runtests() calls
+ * every test__* function with no arguments, so it always gets this default.
  */
-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$$
     );
+
+    /*
+     * Plain cleanup, not a TAP assertion - dropping the schema TEST_SCHEMA
+     * created isn't something this suite is testing, just tearing down
+     * what it created. Same output in every TEST_SCHEMA leg: when
+     * schema_hint is NULL (empty leg), there's nothing to drop, so this is
+     * a no-op; if the DROP SCHEMA itself ever failed, the unhandled
+     * exception aborts the run loudly on its own - no lives_ok() needed
+     * for that.
+     */
+    IF schema_hint IS NOT NULL THEN
+        EXECUTE format('DROP SCHEMA %I', schema_hint);
+    END IF;
 END
 $body$;