diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5111af6..4dbfdb7 100644 --- a/.github/workflows/ci.yml +++ b/.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: diff --git a/Makefile b/Makefile index 4386cc6..7a0a979 100644 --- a/Makefile +++ b/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 diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000..3ae3a96 --- /dev/null +++ b/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/.out` for that scenario. diff --git a/test/expected/extension_tests.out b/test/expected/extension_tests.out index a0cab9d..8913db9 100644 --- a/test/expected/extension_tests.out +++ b/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() diff --git a/test/install/load.sql b/test/install/load.sql index 19883c0..37db853 100644 --- a/test/install/load.sql +++ b/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; diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 0ea7590..1cd1bcb 100644 --- a/test/sql/extension_tests.sql +++ b/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$;