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}
88d025e to
3d35201
Compare
|
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. |
|
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. |
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}
|
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. |
|
Claude finished @jnasbyupgrade's task in 2m 24s —— View job Claude finished reviewing PR #54
SummaryThis review focuses on what's new since the last Claude review (
What changed in
No new issues found. Nothing to fix. |
|
Claude finished @jnasbyupgrade's task in 1m 15s —— View job Claude finished reviewing PR #54
SummaryThis review covers what's new since the last Claude review (
Checked the new summary comment (
No drift between the new summary and the detailed comments/code it points at, and nothing in this repo treats leading Makefile comments as machine-parsed documentation (no No new issues found. Nothing to fix. |
…ls cat_tools while a targeted, non-default schema is the ONLY entry on search_path, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE. The point: prove cat_tools works correctly even when the cat_tools schema itself is never part of the active search_path -- a normal, legitimate deployment choice for a tooling extension (so it never shadows anything, and callers must always schema-qualify it). If cat_tools' own SQL secretly relied on unqualified name resolution somewhere, it would keep working by accident in an ordinary fresh-install run (which never touches search_path) and only break in that deployment; TEST_SCHEMA exists to force that scenario here instead. Empty (the default) does none of that -- CREATE EXTENSION cat_tools runs exactly as a brand-new user would type it, no WITH SCHEMA clause, landing wherever the session's ambient search_path already resolves. Non-empty creates that schema (quoting it, so mixed-case names work), SETs search_path to ONLY that schema, installs with an explicit WITH SCHEMA cat_tools clause, and asserts afterward that cat_tools never appears in the resolved search_path -- which is what makes any later pgTAP pass in that run actually mean something. \set ON_ERROR_STOP on is set near the top of test/install/load.sql so that assertion (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; this already caught a real pg_tle-specific regression once (the existing-mode version assertion used to compare against pg_available_extensions.default_version, which is filesystem-based and returns NULL for a pg_tle-only registration -- fixed by reading a new cat_tools.pgxn_version GUC instead, mirroring bin/test_existing's own current_version() helper). A new TEST_LONG_SCENARIOS make variable drives test-long, an explicit list of exactly the scenarios nothing else already covers -- not a full TEST_LOAD_SOURCE x TEST_SCHEMA cross product. fresh:CatToolsSchema and update:CatToolsSchema are the only places TEST_SCHEMA is exercised at all (the latter also a partial answer to Postgres-Extensions#65, which asks for TEST_SCHEMA coverage on the update path). Both empty-schema combinations are deliberately absent: {fresh, <empty>} is exactly the plain fresh-install/default-schema case make test/installcheck already checks, and {update, <empty>} is exactly what CI's extension-update-test job already proves, more thoroughly, on the same PostgreSQL majors. Each test-long iteration gates via verify-results, this repo's stricter, documented, pgtap-aware check. CI's test job runs an explicit make verify-results for the fresh/default-schema baseline (restoring what that step always did before this PR) followed by make test-long, so every supported PostgreSQL major covers the baseline plus both quoting-requiring-schema scenarios, with no separate matrix leg needed for any of them. test-all sequences a quick make test smoke build with make test-long for a full local pre-push check. A short top-of-file summary comment maps all four test targets (test, test-update, test-long, test-all) for a reader who doesn't want to piece the picture together from four separate comment blocks. Scope boundary, called out explicitly rather than left implicit: extension-update-test and pg-upgrade-test do not exercise TEST_SCHEMA at all yet. Wiring TEST_SCHEMA through the update and pg_upgrade paths is a deliberately deferred follow-up, not an oversight.
…t` job, shrink extension-update-test to PG10-only extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as the `test` job (supported_pg, 12-18), but as its own matrix job: its own runner, container boot, checkout, apt-get, and `make install`, paid again per major, for a check that can run as one more step inside a container the `test` job already has running, already checked out, and already has cat_tools installed on disk in (installcheck, a TEST_DEPS prerequisite, already ran as a side effect of that job's own verify-results/test-long calls). Folded `bin/test_existing update-scenario cat_tools_update 0.2.2` in as an additional call in the `test` job's "Test on PostgreSQL" step instead. Verified before folding it in, not assumed: ran `make check-relkind-source && make verify-results && make test-long && bin/test_existing update-scenario cat_tools_update 0.2.2` in the same shell/cluster session (mirroring the new CI step exactly) against a scratch cluster. Confirmed no database-name collision (pg_regress's own throwaway db is named independently from `cat_tools_update`), the dependency-guard proof fires (twice -- once right after CREATE EXTENSION, once again after the full suite run), the structural-diff check (bin/structural_diff, landed via PR Postgres-Extensions#55 since this branch's last rebase) fires and reports the updated database structurally identical to a fresh install, and the full suite passes -- exit 0 end to end. extension-update-test now runs PG10 only, with no matrix at all (single source of truth: needs.changes.outputs.legacy_pg, not a hardcoded "10") -- its entire remaining purpose is the pre-0.2.2 legacy-script checks, the only place those scripts still load. Removed the now-dead `if: matrix.pg != '10'` / `if: matrix.pg == '10'` guards throughout that job (nothing left to guard against once there's no other leg) and the "Update 0.2.2 -> current" step (moved above). The `changes` job's `update_pg` output/derivation (supported_pg + legacy_pg) is removed too -- it had exactly one consumer, and that consumer is gone. Restructured the top-of-file "Test strategy" comment around what actually matters to a reader: what runs on EVERY supported PostgreSQL major (the `test` job, now including the guard-proved update check, and `pg-tle-test` as a second, independently-isolated main-matrix job -- kept separate deliberately, since it specifically proves pg_tle deployment ISOLATION, not filesystem-install coincidence, unlike the fold above) versus SPECIAL CASES that apply to one specific scenario only (extension-update-test's PG10 legacy scripts, pg-upgrade-test's/pg-tle-upgrade-test's specific binary-pg_upgrade jump pairs, and pg-upgrade-stepwise's full climb). No coverage lost: the PG12+ update-to-current check still runs on the exact same 7 majors it always did (moved, not removed), the PG10 legacy checks are byte-for-byte unchanged, and every other job is untouched.
d307f93 to
c12d0bd
Compare
|
Claude finished @jnasbyupgrade's task in 2m 12s —— View job Claude finished reviewing PR #54
Summary
No new issues found in this commit. Nothing to fix. |
A real timing test showed a single `make verify-results` run takes ~1.1s (a tiny suite) -- the scenario-exclusion logic TEST_LONG_SCENARIOS added was solving for "avoid wasting CI time re-running the suite," but re-running it costs about a second, not worth the complexity it bought. The real CI cost is container/cluster bring-up (~40s/job) and pg-upgrade-stepwise, not suite invocations. Removed TEST_LONG_SCENARIOS, its scenario-tuple shell-parsing loop, and every comment describing the old 2/3/4-combination cross-product design. In its place: - `test-schema` (new): `$(MAKE) test TEST_SCHEMA=CatToolsSchema`. - `test-update` (unchanged): stays simple/no-schema, symmetric with `test` -- resolves the "what does test-update do about schema" ambiguity by giving it none. - `test-update-schema` (new): `$(MAKE) test TEST_LOAD_SOURCE=update TEST_SCHEMA=CatToolsSchema` -- the one scenario nothing else covers (also a partial answer to issue Postgres-Extensions#65). - `test-long`: now a thin wrapper, `test-long: test-schema test-update-schema` -- bundles every test-* target too situational for plain test/test-update but not worth its own CI step. As the suite grows, a new such target just gets added to this prerequisite list. - `test-all`: thin wrapper, `test-all: test test-update test-long`. A single `.NOTPARALLEL: test test-schema test-update test-update-schema test-long test-all` covers the whole family, so test-long/test-all can use plain bare prerequisites instead of sequential $(MAKE) calls in each recipe body. Verified this is safe here specifically: grepped ci.yml/Makefile/sql.mk/lint.mk/pgxntool's own .mk files and docs -- nothing in this build ever invokes `-j`. Also verified GNU Make 4.3's actual .NOTPARALLEL behavior empirically (a throwaway two-target Makefile under `make -j4`): listing explicit targets does NOT scope narrowly to just those targets, it forces full serialization of the whole invoked build graph -- so declaring it once for the whole family gives up nothing narrower than what .NOTPARALLEL already does even when scoped "correctly," and there's no real -j parallelism anywhere in this build to lose regardless. Verified locally against a scratch cluster: real output for test-schema, test-update-schema, test-long (2 suite runs), and test-all (4 suite runs) all passing. Then deliberately broke two independent regressions and confirmed each new target actually fails loudly rather than assuming test-update's existing precedent carries over safely: (1) reintroduced the schema-quoting bug in test/install/load.sql (unquoted SET search_path) -- `make test-schema` failed with exit 2, cascading pgTAP failures from the missing schema; (2) commented out a GRANT in sql/cat_tools--0.2.3--0.3.0.sql.in that only the update path depends on (a fresh install grants it directly) -- `make test-update-schema` failed with exit 2, a real permission-denied failure. Reverted both and confirmed clean passes again. This confirms pgxntool 2.3.0's test-exits-nonzero-on-regression behavior is a real gate for these two new targets, not just assumed from test-update's own precedent. ci.yml's `test` job step updated to match: `make verify-results` (baseline, unchanged mechanism) followed by `make test-long` (now bundling test-schema/test-update-schema) followed by the guard-proved update-to-current check (unchanged from last round). Updated the top "Test strategy" summary block's description of what test-long bundles. Left a clarifying comment on issue Postgres-Extensions#65 noting its "Proposed approach" section's test-long example describes the now-replaced scenario-loop shape, though the issue's core ask (extension-update-test/pg-upgrade-test still don't exercise TEST_SCHEMA) is unaffected either way.
|
Claude finished @jnasbyupgrade's task in 2m 53s —— View job Claude finished reviewing PR #54
SummaryThis review covers what's new since the last Claude review ( What changed: the old I traced the resulting dependency graph and recipe bodies for correctness:
One point worth noting but not a defect: the top-of-file comment ( No new issues found. Nothing to fix. |
…iants
The previous round incorrectly carried over a CI-cost duplication argument (that the {update, <empty>} scenario shouldn't run twice across two separate CI jobs) into test-long's own local composition, where it doesn't apply. test-long/test-all are local dev convenience bundles, not CI cost centers -- that CI-level concern is already fully and correctly handled by the fold-into-test-job/shrink-extension-update-test change from two rounds ago, which is unaffected by this fix.
The rule for test-long is simple inclusion: every test-* target except test itself, full stop. test-long now depends on test-update, test-schema, and test-update-schema (previously just the latter two). test-all simplifies to test + test-long (test-update moved out from directly under test-all since it's now reached via test-long).
Verified with real output: `make test-long` now runs `make test TEST_LOAD_SOURCE=update`, `make test TEST_SCHEMA=CatToolsSchema`, and `make test TEST_LOAD_SOURCE=update TEST_SCHEMA=CatToolsSchema` (three invocations, three suite passes), and `make test-all` runs all four (adding the plain baseline). Both exit 0. `.NOTPARALLEL` already listed test-update, so no change needed there.
Many run: script blocks had the next section's comment starting immediately on the line right after the previous command, with no separator -- so each comment+command section wasn't visually demarcated from the next one. Added a blank line after a command whenever a new comment block (documenting the NEXT, sibling command) immediately follows, throughout the whole file (not just this PR's own changes). Left untouched, deliberately: comments that are the first line right after an if/elif/else opener (they document what follows within that new block, not a separate sibling section after a completed one -- the pattern this fix targets), and everything inside heredocs (the verify-cancel-on-close-coupling job's embedded Python script) since altering blank-line structure there touches different-language content, not this file's own comment/command structure. Pure whitespace -- verified the file still parses as valid YAML and make lint still passes.
…oal, not isolation Isolation (dedicated cluster, filesystem-cleanliness checks) is necessary for pg_tle testing to mean anything, but it isn't the goal -- the goal is proving cat_tools actually works correctly when deployed via pg_tle, across the same fresh-install and update-path scenarios the filesystem matrix already covers. Reworded the summary to lead with that, with isolation explained as the precondition that makes the proof trustworthy rather than the point of the exercise. Comment-only: no job, matrix, or CI-behavior change. Scope stays exactly as it is today (pg-tle-test's fresh install + update path, pg-tle-upgrade-test's 2 jump legs, no pg_tle stepwise job) -- a fuller pg_tle matrix expansion is tracked separately, not described here as upcoming.
|
Claude finished @jnasbyupgrade's task in 2m 32s —— View job Claude finished reviewing PR #54
SummaryThis review covers what's new since the last Claude review (
|
…e-scenario check Comment-only, no behavior change. test-update's inclusion in test-long (kept exactly as instructed) means CI's test job -- which calls make test-long directly, on every supported PostgreSQL major -- now also re-proves part of what that same job's guard-proved update-scenario check already proves more thoroughly. The Makefile's test-long comment claimed test-long/test-all are local dev convenience bundles, not CI cost centers, which is false: test-long is invoked directly from CI. ci.yml's step comment similarly claimed test-long covers nothing update-scenario also covers. Both now state plainly that this overlap is real and deliberately accepted: a single make test pass costs about a second, so one more of them per matrix leg is a different class of cost entirely from the separate-job overhead (~55s per leg) that folding update-scenario into the test job was specifically done to avoid two rounds ago.
|
Claude finished @jnasbyupgrade's task in 48s —— View job Claude finished reviewing PR #54
SummaryThis review covers what's new since the last Claude review ( This commit directly fixes the inconsistency flagged in the previous review round. That review found I re-read both files' updated comments against the actual code and confirmed the fix is accurate and complete:
No new issues found. The previously-flagged inconsistency is fully resolved. |
The prior comment framed test-long's test-update target as "real, accepted overlap" with bin/test_existing's update-scenario check, tolerated because a `make test` pass is cheap. That's wrong: they exercise different code paths entirely. test-update drives test/install/load.sql's own committed update branch (CREATE EXTENSION VERSION 0.2.2, then ALTER EXTENSION UPDATE, inside the pre-suite install step). update-scenario issues its ALTER EXTENSION UPDATE directly via psql, outside load.sql entirely, then runs the suite in TEST_LOAD_SOURCE=existing mode (load.sql's assert-only branch). A bug in load.sql's update-mode logic is only caught by test-update; a bug in the guard/structural-diff logic is only caught by update-scenario. Rewrote the Makefile's test-long comment and ci.yml's Test strategy block and `test` job step comment to say so plainly. Also added a note near pg-upgrade-test/pg-upgrade-stepwise: post-upgrade we currently only assert the version landed and run the base suite, not every dimension the main matrix covers (TEST_SCHEMA, or whatever test-long grows to). Tied to Postgres-Extensions#65 (also just broadened there to explicitly cover pg-tle-test, which has the same TEST_SCHEMA gap) rather than opening a new issue. Trimmed wordiness in the comments touched this PR without dropping any of the underlying reasoning.
ci.yml's own "Test strategy" NOTE already names five jobs that don't exercise TEST_SCHEMA (extension-update-test, pg-upgrade-test, pg-tle-upgrade-test, pg-upgrade-stepwise, pg-tle-test) -- this comment still only named the first two. All five equally drive the extension through bin/test_existing's shell-level flow, bypassing this Makefile's TEST_SCHEMA machinery the same way.
Distilled from PR Postgres-Extensions#54's back-and-forth over where TEST_SCHEMA and the update-testing check should live: a make-level dimension runs locally and never costs a container/job, while a ci.yml-level one always does unless deliberately batched -- which is its own added complexity. The one real exception is when a dimension's whole point is isolation (pg_tle needing its own cluster), not just another combination to run.
jnasbyupgrade
removed
the
claude-debug
Prompted directly by PR Postgres-Extensions#54/Postgres-Extensions#75 turning into hard-to-follow walls of text (a real contributor to a costly mid-PR mix-up), versus Postgres-Extensions#74's own description staying easy to follow despite covering real backstory. Length past the opening isn't the problem -- an unstructured mass of prose is.
|
Closing without merging -- the whole premise of this PR turned out to be a mistaken one, confirmed directly by the maintainer. Root cause of the mistake: cat_tools' control file pins What was actually worth keeping, extracted into two small, focused PRs, both green:
Everything else here -- the Issue #65 (which assumed |
1 participant
Add TEST_SCHEMA, a second independent test-harness switch that installs cat_tools while a targeted, non-default schema is the ONLY entry on search_path, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE.
The point: prove cat_tools works correctly even when the
cat_toolsschema itself is never part of the active search_path -- a normal, legitimate deployment choice for a tooling extension (so it never shadows anything, and callers must always schema-qualify it). If cat_tools' own SQL secretly relied on unqualified name resolution somewhere, it would keep working by accident in an ordinary fresh-install run (which never touches search_path) and only break in that deployment; TEST_SCHEMA exists to force that scenario here instead. Empty (the default) does none of that -- CREATE EXTENSION cat_tools runs exactly as a brand-new user would type it, no WITH SCHEMA clause, landing wherever the session's ambient search_path already resolves. Non-empty creates that schema (quoting it, so mixed-case names work), SETs search_path to ONLY that schema, installs with an explicitWITH SCHEMA cat_toolsclause, and asserts afterward thatcat_toolsnever appears in the resolved search_path -- which is what makes any later pgTAP pass in that run actually mean something.\set ON_ERROR_STOP onis set near the top of test/install/load.sql so that assertion (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; this already caught a real pg_tle-specific regression once (see below).A new
TEST_LONG_SCENARIOSmake variable drivestest-long, an explicit list of exactly the scenarios nothing else already covers -- not a full TEST_LOAD_SOURCE x TEST_SCHEMA cross product.fresh:CatToolsSchemaandupdate:CatToolsSchemaare the only places TEST_SCHEMA is exercised at all, on the fresh and update paths respectively (the latter also a partial answer to #65, which asks for TEST_SCHEMA coverage on the update path). Both empty-schema combinations are deliberately absent:{fresh, <empty>}is exactly the plain fresh-install/default-schema casemake test/installcheckalready checks, and{update, <empty>}is exactly what thetestjob's own guard-proved update-to-current check already proves, more thoroughly (it also plants and proves the dependency guard, and structurally compares against a fresh install), on the same PostgreSQL majors -- see below for that check's own history. Eachtest-longiteration gates viaverify-results, this repo's stricter, documented, pgtap-aware check. CI'stestjob runs an explicitmake verify-resultsfor the fresh/default-schema baseline (restoring what that step always did before this PR, so the baseline isn't silently dropped now thattest-longno longer covers it) followed bymake test-long, so every supported PostgreSQL major covers the baseline plus both quoting-requiring-schema scenarios, with no separate matrix leg needed for any of them.test-allsequences a quickmake testsmoke build (gates via test's own regression.diffs check as of pgxntool 2.3.0+, but not as strict as verify-results) withmake test-longfor a full local pre-push check.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 -- 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, not an oversight.A later commit in this PR ("CI: fold the PG12+ guard-proved update-to-current check into the
testjob, shrink extension-update-test to PG10-only") also fixed a real CI-cost issue found in review:extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as thetestjob, but as its own separate matrix job -- its own runner, container boot, checkout, apt-get, andmake install, paid again per major, for a check that could instead run as one more step inside a container thetestjob already has running, checked out, and with cat_tools already installed on disk. Foldedbin/test_existing update-scenario cat_tools_update 0.2.2into thetestjob's "Test on PostgreSQL" step instead, verified beforehand by running the exact same command sequence in one shell/cluster session locally (confirmed no database-name collision, the dependency-guard proof and the structural-diff check -- from PR #55, landed since this branch's last rebase -- both fire correctly, full suite passes).extension-update-testnow runs PG10 only, no matrix, referencinglegacy_pgdirectly (the now-single-consumerupdate_pgoutput was removed). No coverage was lost: the PG12+ check still runs on the exact same 7 majors it always did, just folded into an existing job instead of a separate one -- confirmed in a real CI run, andextension-update-test's job count dropped from 8 to 1 while thetestjob's own per-leg duration stayed within noise of what it was before (~60s either way, since matrix legs run in parallel). The top-of-file "Test strategy" comment is restructured accordingly: what runs on every supported PostgreSQL major (thetestjob, andpg-tle-testas a second, independently-isolated main-matrix job -- kept separate deliberately, since it specifically proves pg_tle deployment isolation, not filesystem-install coincidence) versus special cases that apply to one specific scenario only (extension-update-test's PG10 legacy scripts, pg-upgrade-test's/pg-tle-upgrade-test's specific binary-pg_upgrade jump pairs, and pg-upgrade-stepwise's full climb).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-longruns exactly the two TEST_LONG_SCENARIOS end to end,make verify-resultsalone covers the baseline, andmake test-allruns all of it together -- all passing the full suite with no expected-output divergence.