test: replace search_path assertion with a real pgTAP test, checked at file end - #76
Conversation
… search_path cat_tools' control file pins schema = 'cat_tools' with relocatable = false -- it can never be installed into a user-chosen schema, so there was never a "which schema" question to test. The real, narrower risk is that some cat_tools view/function could call another cat_tools object unqualified, relying on search_path to resolve it -- which would keep working by accident in any session that happens to have 'cat_tools' searchable, and only break for a real caller who doesn't. Verified empirically before adding this: pgxntool's own tap_setup.sql (\i'd by every test/sql/*.sql file via test/setup.sql) already sets `search_path = tap, public` -- cat_tools' schemas are already never on it today, for every test in the suite, with no toggle needed. This assertion just makes that fact explicit and self-documenting instead of true by accident of the shared baseline, so a future change to it fails loudly instead of silently widening what "passing" means. current_schemas(false) is Postgres's own unqualified-name search list, so checking cat_tools/_cat_tools are absent from it is a direct proof, not a proxy for one (confirmed the check fires: manually SET search_path to include 'cat_tools' and confirmed the RAISE EXCEPTION triggers with the expected message). Always-on, no GUC, no variable, no separate test target -- this is not a toggleable dimension, just a permanent fact this suite now checks.
|
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 |
|
Claude finished @jnasbyupgrade's task in 1m 56s —— View job Code review
Summary: Adds a Checks performed:
No issues found. Checked for bugs and CLAUDE.md compliance. |
…ile end Replace test/setup.sql's DO-block assertion (checked once, at the START of every test file) with test/finish.sql, a real, plan()-counted pgTAP test \i'd right before finish() at the END of every test/sql/*.sql file. This is strictly stronger: a start-only check cannot see a test that mutates search_path partway through a file. Confirmed empirically that each test/sql file runs as its own psql connection wrapped in a transaction that never commits (rolled back when the connection closes), so a plain SET search_path already cannot leak into a LATER file regardless of when it's checked -- the real gap a start-only check leaves open is a mutation that stays in effect for the rest of THAT SAME file's own tests, which checking again at the end now catches. Not foolproof: a test that mutates search_path and then restores it before this check would still slip through. Making it a counted pgTAP test (not a DO block) means a new test file author bumping their own plan() count has to notice this test exists -- a natural reminder not to add a search_path-dependent test without thinking about it. Every test/sql/*.sql file's plan() count is bumped by 1 and expected output regenerated via `make results` accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… second \i Each test/sql/*.sql file now has exactly one finish-side \i (test/finish.sql), mirroring how they already have exactly one setup-side \i (test/setup.sql, which itself \i's test/pgxntool/setup.sql as its first line). test/finish.sql runs the search_path assertion and then \i's test/pgxntool/finish.sql as its last line, rather than every test file calling both directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
changed the title
|
Claude finished @jnasbyupgrade's task in 1m 39s —— View job Code review
Summary: Replaces the earlier Checks performed:
No issues found. |
The previous comment recounted the empirical investigation that justified this design (psql-connection-per-file, transaction rollback semantics) -- that belongs in the PR description, not as a permanent code comment. Keep only the conclusion: what this asserts, why file-end (not just setup), the one caveat, and the plan()-bump reminder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 2m 34s —— View job Code review
Summary: Since the last review pass (which covered through Finding
Everything else checked and unchanged from prior passes: the shortened comment's content is accurate and still explains the "why file-end" rationale and the mutate-then-restore caveat without recounting the empirical investigation history (matches CLAUDE.md's Bug Fixes guidance — no novels, no unnecessary history). Logic of the No other issues found. |
1 participant
cat_tools' control file pins
schema = 'cat_tools'withrelocatable = false-- it can never be installed into a user-chosen schema, so there is no "which schema" question worth testing. The real, narrower risk is that some cat_tools view or function could call another cat_tools object unqualified, relying onsearch_pathto resolve it -- which would keep working by accident in any session that happens to havecat_toolssearchable, and only break for a real caller who doesn't.This adds a permanent, real pgTAP test that rules that out:
test/finish.sql,\i'd by everytest/sql/*.sqlfile in place of a direct\i test/pgxntool/finish.sql, asserts thatcat_tools/_cat_toolsare absent fromcurrent_schemas(false)-- Postgres's own unqualified-name resolution list -- and then chains through totest/pgxntool/finish.sqlitself (mirroring howtest/setup.sqlalready wrapstest/pgxntool/setup.sql). Because it's checked at the END of every test file rather than as an invisible side note, every passing test in this suite is itself proof that cat_tools' internal SQL fully schema-qualifies its own cross-references, rather than merely happening to resolve by search_path accident.Why check at the end of each file, not just the start
pgxntool's own
tap_setup.sqlalready setssearch_path = tap, publicfor every test file, so a check placed only at the very start (as an earlier version of this PR did, via atest/setup.sqlDO block) can only prove the ambient baseline was clean before that file's own statements ran -- it can't see a test that mutatessearch_pathpartway through.Confirmed empirically before making this change: each
test/sql/*.sqlfile runs as its own psql connection, wrapped in a transaction that is never committed (seetest/pgxntool/finish.sql's "TRANSACTION INTENTIONALLY LEFT OPEN") and so always rolls back when that connection closes. A plainSET search_pathmade anywhere in the file is undone by that rollback, so it already cannot leak into a LATER file no matter when it's checked. What a start-only check misses is a mutation that remains in effect for the REST of that same file's own tests after it happens -- checking again immediately before that rollback, at the end of the file, catches that case. Not foolproof: a test that mutatessearch_pathand then restores it before the check runs would still slip through.Why a real pgTAP test, not a DO block
Making this a
plan()-counted assertion, rather than an invisibleDO $$ ... $$block, means a future test file author bumping their ownplan()count has to notice this test exists -- a natural nudge not to add a search_path-dependent test without thinking about it.Test plan
make verify-resultspasses locally with the newtest/finish.sqlin place, andtest/expected/*.outregenerated viamake resultsSET search_path = cat_tools, tap, publicinto a scratch test file and confirmed only that file'stest/finish.sqlcheck failed -- files run afterward were unaffected, confirming the per-file connection/rollback isolationmake lintclean