Add TEST_SCHEMA test-harness switch - #54
Conversation
|
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…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 checks the RESOLVED search_path (current_schemas(false), not pg_namespace -- checking pg_namespace only proves CREATE SCHEMA worked, not that SET search_path actually resolved to it) so a quoting bug in either statement is actually caught rather than passing silently. \set ON_ERROR_STOP on is set near the top of load.sql so that check (and every other RAISE EXCEPTION in the file) can actually fail the pg_regress run instead of psql printing an error and continuing past it.
A new TEST_SCHEMAS make variable (default: empty and one mixed-case quoting-requiring name, `CatToolsSchema`) and TEST_LOAD_SOURCES (fresh, update) drive a new `test-long` target that loops `verify-results` once per {TEST_LOAD_SOURCE, TEST_SCHEMA} combination -- the only target that can actually gate on a regression, since plain `test`/`test-update` terminate in pgxntool's .IGNORE'd installcheck. `test-all` sequences `test` and `test-long` for a full local pre-push check. CI's `test` job runs `make test-long` on every supported PostgreSQL major, so all four combinations are covered everywhere, with no separate matrix leg needed for any of them.
Scope boundary, called out explicitly in the Makefile and the ci.yml "Test strategy" comment rather than left implicit: `extension-update-test` and `pg-upgrade-test` do not exercise TEST_SCHEMA/TEST_SCHEMAS at all yet (they drive the extension through bin/test_existing's own createdb/CREATE EXTENSION/ALTER EXTENSION UPDATE flow, not this Makefile's TEST_LOAD_SOURCE path). Wiring TEST_SCHEMA through the update and pg_upgrade paths is a deliberately deferred follow-up (Postgres-Extensions#65), not an oversight.
88d025e to
3d35201
Compare
…e existing-mode version assertion in test/install/load.sql compared the installed version against pg_available_extensions.default_version, which is FILESYSTEM-based and returns NULL for an extension registered purely via pg_tle (no control file ever touches disk in that deployment path). Before ON_ERROR_STOP, this always-broken-for-pg_tle comparison silently printed an error and continued; with ON_ERROR_STOP now enforcing that RAISE EXCEPTION actually aborts the script, every pg_tle CI job started failing (confirmed by reproducing the exact existing-mode check against a local pg_tle-only registration, where pg_available_extensions came back NULL). Fixed by threading PGXNVERSION (the distribution version from META.json) into a new cat_tools.pgxn_version GUC and comparing against that instead -- mirroring bin/test_existing's own current_version() helper, which already avoids pg_available_extensions for the identical reason. The export has to live in a second PGOPTIONS append AFTER `include sql.mk`, since PGXNVERSION isn't defined until base.mk's meta.mk include runs. Re-verified end to end: reproduced the pg_tle failure locally (built and registered pg_tle 1.5.2 against a scratch cluster, ran bin/test_existing's exact update-scenario flow with TEST_EXISTING_DEPLOY=pgtle), confirmed the fix resolves it, and reran the full make test-long / test-all matrix (both TEST_LOAD_SOURCE values x both TEST_SCHEMA values) to confirm nothing else regressed.
|
Claude Code is working… I'll analyze this and get back to you. |
1 similar comment
|
Claude Code is working… I'll analyze this and get back to you. |
The rationale for this was only in the file's header comment, several paragraphs above -- easy to miss and remove without realizing every RAISE EXCEPTION guard in this file (TEST_SCHEMA, test_load_mode, the existing-mode version assertion) would silently become a no-op.
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude Code is working… I'll analyze this and get back to you. |
…explicit The mechanism set up an ambient schema and put it on search_path, but never did the thing its own comments claimed: CREATE EXTENSION ran with no WITH SCHEMA clause (implicit placement only, via the non-relocatable control file), and nothing ever asserted that 'cat_tools' itself stays out of the resolved search_path. The actual point of TEST_SCHEMA is to prove cat_tools' own internal SQL doesn't secretly depend on unqualified name resolution to its own schema -- a real, legitimate deployment keeps a tooling extension's schema off every role's default search_path, and a latent dependency on it being searchable would keep working by accident in every other ordinary test run and only break there. - CREATE EXTENSION now explicitly adds WITH SCHEMA cat_tools when TEST_SCHEMA is set, instead of relying on implicit placement. - A CREATE SCHEMA IF NOT EXISTS cat_tools precedes it: unlike a bare CREATE EXTENSION (which auto-creates the control-file-pinned schema if needed), WITH SCHEMA requires that schema to already exist, even when it's the same name -- confirmed by testing this against a scratch cluster, which is also how the missing CREATE SCHEMA surfaced in the first place. - A new assertion after install fails loudly if 'cat_tools' ever appears in the resolved search_path, which is the property this whole mechanism exists to prove. Verified it actually fires by manually forcing the violation. - Rewrote the TEST_SCHEMA comments in both load.sql and the Makefile around this actual goal; the previous framing (a quoting-pipeline check) doesn't hold up -- psql substitutes the same variable identically regardless of which statement it appears in, so CREATE SCHEMA and SET search_path can't diverge on quoting -- and is left as a much narrower fixture sanity check. Tested against an isolated scratch cluster (fresh/update x empty/CatToolsSchema, plus test-long's full matrix), since this repo's usual pgstart-managed clusters were too polluted with unrelated leftover test databases from other concurrent work to trust for this.
jnasbyupgrade
added
the
claude-debug
|
Claude finished @jnasbyupgrade's task in 5m 53s —— View job Claude finished reviewing PR #54
SummaryBoth issues raised by the earlier automated review on this PR have already been fixed by later commits in this branch:
I re-reviewed the diff (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
… flagged by review.
test-all's comment claimed `make test` is "non-gating", contradicting CLAUDE.md and pgxntool 2.3.0+: test's own recipe already exits non-zero on a regression.diffs entry, it's just not as strict as verify-results's pgtap-aware check. Reworded to say what's actually true.
Verified a second finding before touching anything: test-long's {TEST_LOAD_SOURCE=update, TEST_SCHEMA=<empty>} scenario duplicates CI's extension-update-test job -- both run the SAME 0.2.2-to-current update on the SAME PostgreSQL majors (12-18), and extension-update-test does strictly more (plants and proves the dependency guard) than a bare verify-results call ever would. Replaced test-long's TEST_SCHEMAS x TEST_LOAD_SOURCES cross product with an explicit TEST_LONG_SCENARIOS list of exactly the three scenarios that add value (fresh:<empty>, fresh:CatToolsSchema, update:CatToolsSchema), dropping only the redundant one. Updated the Makefile and ci.yml "Test strategy" comments to match the three-scenario design and explain why the fourth is deliberately absent.
|
Claude finished @jnasbyupgrade's task in 2m 22s —— View job Claude finished reviewing PR #54
SummaryBoth substantive issues flagged by the earlier automated review (2026-08-01) are resolved by the latest commits:
The most recent commit ( I re-read both files in full against the PR description's claims (propagation of
No code changes made — this was a review-only request, and no new findings survived scrutiny. |
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. The guard in test/install/load.sql checks the RESOLVED search_path (
current_schemas(false), notpg_namespace-- checkingpg_namespaceonly proves CREATE SCHEMA worked, not that SET search_path actually resolved to it) so a quoting bug in either statement is actually caught.\set ON_ERROR_STOP onis set near the top of load.sql so that check (and every other RAISE EXCEPTION in the file) can actually fail the pg_regress run instead of psql printing an error and continuing past it.A new
TEST_LONG_SCENARIOSmake variable drivestest-long, which loopsverify-resultsonce per explicitTEST_LOAD_SOURCE:TEST_SCHEMAscenario -- deliberately an explicit list, not a full cross product:fresh:<empty>,fresh:CatToolsSchema, andupdate:CatToolsSchemaare kept, butupdate:<empty>is dropped because it's exactly what CI'sextension-update-testjob already proves, more thoroughly (it also plants and proves the dependency guard), on the same PostgreSQL majors -- repeating just the suite-pass subset of that here would add CI wall-clock with no added confidence.test-allrunstest(a quick smoke build; gates viatest's own regression.diffs check as of pgxntool 2.3.0+, but not as strict as verify-results's pgtap-aware check) followed bytest-long. CI'stestjob runsmake test-longon every supported PostgreSQL major, so all three scenarios are covered everywhere, with no separate matrix leg needed for any of them.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 at all yet. Wiring TEST_SCHEMA through the update and pg_upgrade paths is a deliberately deferred follow-up (#65), not an oversight -- thoughupdate:CatToolsSchemaabove is a partial answer to it on the plain extension-update path.Verified locally on scratch PostgreSQL clusters (isolated from the container's shared clusters, since concurrent agents were also exercising cat_tools's shared roles/databases there), including a real pg_tle 1.5.2 build to reproduce and fix a pg_tle-specific regression the ON_ERROR_STOP change exposed (the existing-mode version assertion previously compared against
pg_available_extensions.default_version, which is filesystem-based and returns NULL for a pg_tle-only registration; it now reads acat_tools.pgxn_versionGUC instead, mirroringbin/test_existing's owncurrent_version()helper).make test-longandmake test-allboth run exactly the three scenarios above end to end, passing the full suite each time with no expected-output divergence.