Phase 2: TEST_SCHEMA switching in test/install + schema-invariant assertions

.github/workflows/ci.yml

@@ -5,13 +5,23 @@ on:
       - master
   pull_request:
 jobs:
+  # 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 }}

Makefile

@@ -6,3 +6,22 @@ include pgxntool/base.mk
 
 # Temporary hack
 testdeps: $(wildcard test/*/*.sql) $(wildcard test/*.sql) # Be careful not to include directories in this
+
+# 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)

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/<test>.out` for that scenario.

test/core/functions.sql

@@ -43,19 +43,32 @@ BEGIN
         );
       END IF;
 
+      /*
+       * Explicit descriptions below (not pgTAP's auto-generated default,
+       * which schema-qualifies via ncs()): this suite may run against
+       * count_nulls installed in ANY schema (see TEST_SCHEMA in the
+       * Makefile), and the description text is exact-matched by pg_regress
+       * against a single committed expected-output file - it must stay
+       * IDENTICAL no matter which schema the extension actually landed in.
+       * ncs() itself is still used to locate and call the real function;
+       * only the visible description text drops it.
+       */
       RETURN NEXT function_returns(
         ncs(), f_name, f_args
         , 'int'::regtype::text -- Sanitize type name
+        , format('Function %s(%s) should return int', f_name, array_to_string(f_args, ','))
       );
 
       -- TODO: isnt_definer
       RETURN NEXT isnt_strict(
         ncs(), f_name, f_args
+        , format('Function %s(%s) should not be strict', f_name, array_to_string(f_args, ','))
       );
 
       RETURN NEXT volatility_is(
         ncs(), f_name, f_args
         , 'immutable'
+        , format('Function %s(%s) should be IMMUTABLE', f_name, array_to_string(f_args, ','))
       );
     END LOOP;
   END LOOP;
@@ -65,16 +78,19 @@ BEGIN
     RETURN NEXT function_returns(
       ncs(), f_name, f_args
       , 'trigger'
+      , format('Function %s() should return trigger', f_name)
     );
 
     -- TODO: isnt_definer
     RETURN NEXT isnt_strict(
       ncs(), f_name, f_args
+      , format('Function %s() should not be strict', f_name)
     );
 
     RETURN NEXT volatility_is(
       ncs(), f_name, f_args
       , 'immutable'
+      , format('Function %s() should be IMMUTABLE', f_name)
     );
   END LOOP;
 END
@@ -90,6 +106,13 @@ CREATE FUNCTION pg_temp.test_trigger_raw(
   , exec text
   , errmsg text
   , errdesc text
+  /*
+   * Schema-free stand-in for exec, used ONLY in the visible description
+   * below - exec itself (schema-qualified via ncs(), by callers) is what
+   * actually runs. Keeps this suite's output identical no matter which
+   * schema count_nulls is installed in (see TEST_SCHEMA).
+   */
+  , exec_desc text
 
 ) RETURNS SETOF text LANGUAGE plpgsql AS $body$
 DECLARE
@@ -100,8 +123,15 @@ DECLARE
       , exec
     )
   ;
+  c_command_desc CONSTANT text :=
+    format( $$CREATE TRIGGER %s %s INSERT ON test_data FOR EACH ROW EXECUTE PROCEDURE %s$$
+      , trigger_name
+      , ba
+      , exec_desc
+    )
+  ;
 BEGIN
-  RETURN NEXT lives_ok( c_command, c_command );
+  RETURN NEXT lives_ok( c_command, c_command_desc );
   RETURN NEXT throws_ok(
     $$INSERT INTO test_data VALUES (1,1,NULL)$$
     , 'P0001'
@@ -136,6 +166,11 @@ BEGIN
         )
       , errmsg := coalesce( err, format( 'test_data must contain 1 %s fields', upper( replace( nn, '_', ' ' ) ) ) )
       , errdesc := 'Test ' || c_trigger_name
+      , exec_desc := format(
+          $$%s_count_trigger(1, %L)$$
+          , nn
+          , err
+        )
     )
   ;
 END
@@ -163,20 +198,20 @@ BEGIN
   RETURN NEXT bag_eq(
     format($$SELECT a, b, c, %1$I.null_count( a, b, c ), %1$I.not_null_count( a, b, c ) FROM test_data$$, ncs())
     , $$SELECT *, 3-null_count AS not_null_count FROM test_data$$
-    , format('Test %I.null_count(a, b, c)', ncs())
+    , 'Test null_count(a, b, c)'
   );
 
   -- Test JSON versions
   -- These could be combined...
   RETURN NEXT bag_eq(
     format($$SELECT a, b, c, %1$I.null_count( row_to_json( row(a, b, c) ) ), %1$I.not_null_count( row_to_json( row(a, b, c) ) ) FROM test_data$$, ncs())
     , $$SELECT *, 3-null_count AS not_null_count FROM test_data$$
-    , format('Test %I.null_count(json)', ncs())
+    , 'Test null_count(json)'
   );
   RETURN NEXT bag_eq(
     format($$SELECT a, b, c, %1$I.null_count( row_to_json( row(a, b, c) )::jsonb ), %1$I.not_null_count( row_to_json( row(a, b, c) )::jsonb ) FROM test_data$$, ncs())
     , $$SELECT *, 3-null_count AS not_null_count FROM test_data$$
-    , format('Test %I.null_count(jsonb)', ncs())
+    , 'Test null_count(jsonb)'
   );
 
   -- Doesn't work for array types
@@ -198,10 +233,13 @@ BEGIN
               WHEN args = '' AND not_ = ''      THEN $$test trigger usage: number of NULL columns[, error message]$$
               ELSE 'test trigger: first argument must not be null'
             END
-          , 'Test ' || trig
+          , 'Test ' || trig_desc
+          , trig_desc
         )
       FROM (
-        SELECT *, format( '%I.%snull_count_trigger( %s )', ncs(), not_, args ) AS trig
+        SELECT *
+          , format( '%I.%snull_count_trigger( %s )', ncs(), not_, args ) AS trig
+          , format( '%snull_count_trigger( %s )', not_, args ) AS trig_desc
           FROM
             unnest( array['not_', ''] ) not_
             , unnest( array['NULL', ''] ) args