Add TEST_SCHEMA test-harness switch - #54
Conversation
…s a schema before installing cat_tools, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE.
Empty (the default) leaves search_path untouched, so CREATE EXTENSION cat_tools runs exactly as a brand-new user would type it. A non-empty value creates that schema (quoting it, so mixed-case names work) and SETs search_path to it first. cat_tools' control file pins schema = 'cat_tools' with relocatable = false, so this does not relocate the extension itself -- what it proves is that the propagation pipeline handles a quoting-requiring schema name correctly end to end, and that installing while some other, possibly hostile, schema is ambient doesn't break anything. A DO block in test/install/load.sql makes that a real check rather than a silent no-op: PostgreSQL doesn't error on a search_path entry that was never created, so a quoting bug elsewhere in the pipeline could otherwise go undetected.
CI wires this into the `test` job as a dedicated, always-quoting-requiring leg (mixed-case `CatToolsSchema`, on the newest supported major) alongside the existing TEST_SCHEMA-empty legs on every supported major. The job's matrix moves from a bare `pg:` list to a pure `include:` list of {pg, test_schema} objects computed in the `changes` job, because GitHub's matrix `include` merges an object into any existing combination whose keys it matches rather than adding a new one -- pairing test_schema with a pg value already in a bare list would silently replace that version's default run instead of adding a second, separate leg.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…vel loop, so every supported PostgreSQL major exercises both schema variants instead of only the newest. Per discussion with the maintainer: the `test` job's matrix reverts to a plain pg list (no more crossing test_schema against pg via a computed include list -- that mechanism, and the reasoning for why a bare cross would have collided with GitHub's matrix include-merge semantics, is removed along with it). Instead, a new TEST_SCHEMAS make variable (default: "" and a mixed-case quoting-requiring name) drives a new test-long target that loops verify-results once per schema value, and CI's test job now runs `make test-long` on every major. A test-all target sequences test, test-update and test-long for a full local pre-push check. extension-update-test and pg-upgrade-test still do not exercise TEST_SCHEMA -- called out explicitly in both the Makefile and the ci.yml "Test strategy" comment as a deliberately deferred follow-up, not an oversight.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .PHONY: test-all | ||
| test-all: | ||
| $(MAKE) test | ||
| $(MAKE) test-update | ||
| $(MAKE) test-long | ||
|
|
There was a problem hiding this comment.
test-all's first two legs can't actually fail on a regression: $(MAKE) test and $(MAKE) test-update (= $(MAKE) test TEST_LOAD_SOURCE=update) both terminate in pgxntool's installcheck, which is .IGNORE'd — this repo's own CLAUDE.md states exactly this: "make test does not return non-zero on test regressions... To actually detect failures, use make verify-results."
test-long's first loop iteration (TEST_SCHEMA="", default TEST_LOAD_SOURCE=fresh) already re-covers the fresh-install case via the gating verify-results, making the $(MAKE) test leg redundant as well as non-gating. The $(MAKE) test-update leg is the real gap: it's the only update-mode coverage in test-all, and since it never routes through verify-results, an update-path regression makes make test-all exit 0 — silently defeating its purpose as a full local pre-push gate.
| .PHONY: test-all | |
| test-all: | |
| $(MAKE) test | |
| $(MAKE) test-update | |
| $(MAKE) test-long | |
| test-all: | |
| $(MAKE) verify-results TEST_LOAD_SOURCE=update | |
| $(MAKE) test-long |
|
|
||
| DO $DO$ | ||
| BEGIN | ||
| IF NOT EXISTS ( | ||
| SELECT 1 FROM pg_namespace WHERE nspname = current_setting('cat_tools.test_schema') | ||
| ) THEN | ||
| RAISE EXCEPTION | ||
| 'TEST_SCHEMA target schema % was not created as named -- quoting bug?' | ||
| , current_setting('cat_tools.test_schema') | ||
| ; | ||
| END IF; | ||
| END | ||
| $DO$; | ||
| \endif | ||
| -- end \if :cat_tools_has_schema | ||
|
|
There was a problem hiding this comment.
This guard doesn't actually catch the regression its own comment describes, and even if it did, it couldn't fail the test run — two separate problems:
-
Checks the wrong side of the pipeline. The condition only re-verifies that
CREATE SCHEMA(line 79) produced a namespace with the configured name (pg_namespace.nspname = current_setting('cat_tools.test_schema')). It never inspects the resolvedsearch_path. The comment above states the failure mode as "CREATE SCHEMA would create one name, SET search_path would reference another" — but if line 80'sSET search_pathalone regressed to unquoted interpolation (case-folding e.g.CatToolsSchema→cattoolsschema) while line 79 stays correctly quoted, the schema still exists under its correct name,NOT EXISTSis false, and the guard passes silently whilesearch_pathpoints at a nonexistent schema — exactly the scenario it's meant to catch. Detecting that requires checking the resolved path itself, e.g.current_setting('cat_tools.test_schema') <> ALL (current_schemas(false)), notpg_namespace. -
Even when it does fire,
RAISE EXCEPTIONcan't fail anything.test/install/load.sql's output is self-comparing — pg_regress resolves both the expected and result paths to the same file here (seetest/install/.gitignore), so nothing is ever diffed. NoON_ERROR_STOPis set for this script either, so psql prints theERROR:and keeps going rather than aborting —CREATE EXTENSION cat_toolsstill runs, and nothing downstream (verify-results, the pgTAP suite) observes the botchedsearch_path. Adding\set ON_ERROR_STOP onnear the top of this file would make the guard load-bearing: aborting here would preventCREATE EXTENSIONfrom running, and every pgTAP test would then fail loudly with a real, diffed regression — but that's a structural change to the file worth doing deliberately rather than as a one-line patch here.
1 participant
Add TEST_SCHEMA, a second independent test-harness switch that targets a schema before installing cat_tools, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE.
Empty (the default) leaves search_path untouched, so CREATE EXTENSION cat_tools runs exactly as a brand-new user would type it. A non-empty value creates that schema (quoting it, so mixed-case names work) and SETs search_path to it first. cat_tools' control file pins schema = 'cat_tools' with relocatable = false, so this does not relocate the extension itself -- what it proves is that the propagation pipeline handles a quoting-requiring schema name correctly end to end, and that installing while some other, possibly hostile, schema is ambient doesn't break anything. A DO block in test/install/load.sql makes that a real check rather than a silent no-op: PostgreSQL doesn't error on a search_path entry that was never created, so a quoting bug elsewhere in the pipeline could otherwise go undetected.
A new TEST_SCHEMAS make variable (default: empty and one mixed-case quoting-requiring name,
CatToolsSchema) drives a newtest-longtarget that loopsverify-resultsonce per TEST_SCHEMA value;test-allsequencestest,test-updateandtest-longfor a full local pre-push check. CI'stestjob runsmake test-longon every supported PostgreSQL major, so both schema variants are covered everywhere, with no separate matrix leg needed for it.Scope boundary, called out explicitly in the Makefile and the ci.yml "Test strategy" comment rather than left implicit:
extension-update-testandpg-upgrade-testdo not exercise TEST_SCHEMA/TEST_SCHEMAS at all yet. Wiring TEST_SCHEMA through the update and pg_upgrade paths is a deliberately deferred follow-up, not an oversight.Verified locally on scratch PG12 and PG17 clusters (isolated from the container's shared clusters, since another concurrent agent appeared to be exercising cat_tools's shared roles/databases there):
make test-longandmake test-allboth loop overTEST_SCHEMA=(empty) thenTEST_SCHEMA=CatToolsSchema, passing the full 16-test suite each time with no expected-output divergence (the created schema is real --\dnshows it verbatim, quoting preserved -- but cat_tools itself always lands in thecat_toolsschema regardless, per the control-file constraint above).