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..89e001d 100644 --- a/Makefile +++ b/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 diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000..48885fc --- /dev/null +++ b/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/.out` for that scenario. diff --git a/test/expected/extension_tests.out b/test/expected/extension_tests.out index a0cab9d..73e1a05 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() @@ -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 diff --git a/test/expected/extension_tests_1.out b/test/expected/extension_tests_1.out new file mode 100644 index 0000000..c43e443 --- /dev/null +++ b/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 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..db06342 100644 --- a/test/sql/extension_tests.sql +++ b/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$;