Test install foundation: TEST_LOAD_SOURCE modes, dependency guard, schema quoting rename - #19
Draft
jnasbyupgrade wants to merge 2 commits into
Draft
Test install foundation: TEST_LOAD_SOURCE modes, dependency guard, schema quoting rename#19jnasbyupgrade wants to merge 2 commits into
jnasbyupgrade wants to merge 2 commits into
Conversation
…, quoting-requiring schema test Builds the U&U (update & upgrade) test infrastructure that doesn't require a real second extension_drop version or pg_upgrade CI to already exist: - PGXNTOOL_ENABLE_TEST_INSTALL = yes, with test/install/load.sql as the committed-once installer for the extension (no test roles exist for this extension, so unlike cat_tools there's nothing role-related to add). - TEST_LOAD_SOURCE (fresh/update/existing) GUC/make-var switch, parse-time validated, exported unconditionally, read in load.sql without missing_ok. `existing` mode is fully exercised locally (verified against a real, already-installed database, including the failure path when the extension is genuinely absent). `update` mode is wired up and structurally verified end-to-end, but extension_drop has no real prior released version to update FROM yet -- the Makefile refuses to run it without TEST_UPDATE_FROM set explicitly, and no CI leg exercises it in this repo today. - Dependency guard (test/sql/dependency_guard.sql): a view depending on extension_drop__commands' row type blocks a non-CASCADE DROP EXTENSION; proven by actually attempting the drop and asserting failure, not assumed. - test/sql/schema.sql's custom-schema test names renamed to mixed case (requires identifier quoting), reusing its existing coverage rather than adding a new schema-testing dimension. - ci.yml: run `make test && make verify-results` instead of pg-build-test, so a real regression actually fails the build (pgxntool's .IGNORE: installcheck otherwise reports green regardless of test results, per RELEASE.md's existing note about PRs #6/#7). Moving the extension's own installation into test/install/load.sql required adapting every test file that used to install it per-test in a rolled-back transaction (test/deps.sql, test/sql/simple.sql, test/sql/schema.sql, test/sql/zzz_build.sql) to work against the new committed-once install instead, since an extension name is a database-wide singleton. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…redate this branch CI on this branch showed the switch to `make test && make verify-results` surfacing real pgTAP failures on PostgreSQL 9.3/9.6 (cat_tools/extension_drop never actually install there). Checked PR #10's own baseline CI (#10, run 30665031257): PG 9.3 and 9.6 already report "3 of 3 tests failed" in the raw job log there too, just silently reported as a passing check because pg-build-test's underlying `make test` hits pgxntool's `.IGNORE: installcheck` the same way. So this isn't a regression from this PR's own changes -- it's the exact masking problem RELEASE.md already documents, just now applying to a different, older part of the PG matrix than the PRs (#6/#7) it originally cites. Reverting the ci.yml step back to pg-build-test here keeps this PR scoped to test/install infrastructure; fixing cat_tools's install path on pre-PG10 belongs to whoever owns that dependency setup (PR #10 or a follow-up), not this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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 |
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrated from fork-internal PR jnasbyupgrade#4 to enable a native same-repo stacked PR (base branch
fix-cat-tools-installnow exists directly onPostgres-Extensions/extension_tools, so this chains off upstream PR #10 without needing a cross-fork base). Original PR: jnasbyupgrade#4Stacked on #10 (
fix-cat-tools-install) — targets that branch, notmaster.What this builds
Test infrastructure for update/upgrade (U&U) testing, modeled on
Postgres-Extensions/cat_tools's currentmaster(itstest/install/load.sql,test/roles.sql,MakefileTEST_LOAD_SOURCEblock, andbin/test_existing'sdependency-guard technique), adapted to what actually applies to
extension_drop:PGXNTOOL_ENABLE_TEST_INSTALL = yes+test/install/load.sql— acommitted-once installer for the extension, run before the main pgTAP
suite via pgxntool's
test/installfeature. No test roles exist for thisextension (checked
test/deps.sqland everytest/sql/*.sqlfile), sounlike cat_tools's
load.sqlthere's notest/roles.sqlhere — nothingto migrate.
TEST_LOAD_SOURCE(fresh/update/existing) — parse-time validated inthe
Makefile, exported unconditionally as a GUC, read inload.sqlwithout
missing_ok.make test-updatewrapper included.existingis fully exercised locally: I created a real database,installed
extension_dropfor real, then ranmake test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=noagainstit — all 5 tests passed, and
load.sqlcorrectly assertedpresence+version without touching the install. I also verified the
failure path: pointed the same invocation at a genuinely empty
database and confirmed
load.sql'sRAISE EXCEPTIONfired (visible intest/install/load.out) and every downstream test correctly failedrather than silently getting a fresh reinstall.
updateis wired up and structurally exercised (make test-update TEST_UPDATE_FROM=1.0.0runs the full CREATE EXTENSION VERSION + ALTEREXTENSION UPDATE path end-to-end cleanly), but extension_drop has
never had a real second released version — PGXN's only listing
(0.1.x, 2017) predates the current SQL entirely (see
HISTORY.asc/RELEASE.md). There is no real value forTEST_UPDATE_FROMyet, sothe
Makefilerefuses to run this mode without it set explicitly, andno CI leg exercises it in this repo today. This is expected per the
task scope, not an oversight.
Dependency guard (
test/sql/dependency_guard.sql) — a view in itsown schema (
extension_drop_drop_guard) whose one column is typed onextension_drop__commands' row type, creating a hardpg_dependedge.extension_drophas no enums (cat_tools's own guard types on one grownvia
ADD VALUE), so a stable table's row type is the closest equivalent—
extension_drop__commandsis the one state table every other objectin the extension revolves around; getting rid of it would be a rewrite,
not a routine update. The test actually attempts the non-CASCADE
DROP EXTENSIONand asserts it fails (SQLSTATE2BP01), then asserts both theextension and the guard view are still present — proven, not assumed.
Schema-testing: per the scoped-down instruction, this is only a
rename.
extension_drop.controlhas noschema=line andsql/extension_drop.sqlnever hardcodes a schema (no@extschema@either — it relies on
CREATE EXTENSION ... SCHEMA/search_path-at-creation-time for its public objects;
__extension_dropis adeliberately fixed internal helper schema, created and dropped within
the same install script, unrelated to where the public objects land) —
so
test/sql/schema.sqlwas already doing real schema-flexibilitytesting. Its schema names (
_test_ed,_test_ed_2) were plain lowercaseand didn't require quoting; renamed to
_Test_Ed/_Test_Ed_2(mixedcase) so a missing-quote bug at a use site would fail loudly instead of
silently folding to a different, unquoted schema. No new TEST_SCHEMA
switch, no new test leg — reusing the coverage that was already there,
as instructed.
ci.yml: attempted switching thetestjob's step frompg-build-testtomake test && make verify-results, then revertedit — see "CI attempt" below. Deferred to a follow-up, not included
here.
Why every non-schema.sql test file needed touching
Moving the extension's own installation into
test/install/load.sql(committed, once, persists into every later test) is incompatible with
test/deps.sql's old per-test behavior of installingextension_dropfresh into a private schema every single test file (deliberately, with no
IF NOT EXISTS, "because we want an error if the extension is alreadyloaded"). An extension name is a database-wide singleton, so once
load.sqlcommits a real install, any later unqualifiedCREATE EXTENSION extension_dropcollides. Required adaptations, not scope creep:test/deps.sql: no longer creates the extension; only sets up the shared:TTtemp table.test/sql/simple.sql: dropped its own per-test schema/extension setup;removed now-stale
_test_ed.-qualified calls (unqualified resolves finesince
public— whereload.sql's ambient install lands — stays onsearch_pathregardless of the mid-fileSET search_pathchange).test/sql/schema.sql: now explicitly drops the ambient committed installand recreates its own copies in its chosen (renamed) schemas — that's
its actual test subject, so this is really just making explicit what used
to happen implicitly via
deps.sql.test/sql/zzz_build.sql: added an explicitDROP EXTENSION IF EXISTS extension_drop CASCADE;before its raw\i sql/extension_drop.sql,since that script's own
CREATE TABLE extension_drop__commandsnowcollides with the committed install otherwise.
All of this is safe because every
test/sql/*.sqlfile runs in its ownpgTAP-wrapped, rolled-back transaction — nothing any of them do to the
committed install escapes past that one file/session.
Discovered along the way
zzz_build.out: pre-existing on the base branch, unrelated tothis PR (confirmed present on both PG12 and PG17) — a NOTICE about
%TYPEresolution that the checked-in expected output predates. This isexactly the "CI doesn't fail on test failures" gap
RELEASE.mdalreadydocuments (
.IGNORE: installcheck). Regenerated via the sanctionedmake resultsworkflow (rootCLAUDE.mdpermits this in this environment) aspart of touching that file anyway; confirmed zero raw
not oklines intest/results/*.outfirst, per the documented safe-regen check. This iswhat unblocked point 5 above.
test/install/*.outis structurally unable to catch a regression:because the install schedule references tests as
../install/<name>,pg_regress's expected-path and actual-path both resolve to the exact same
file (
test/install/load.out), one directory above bothtest/expected/andtest/results/. I confirmed this by deliberatelybreaking
load.sql'sexisting-mode assertion against an emptydatabase: the real error text showed up correctly in
test/install/load.out, but pg_regress still reported the step itselfok(only the downstream tests correctly failed, since the extensiongenuinely wasn't there). cat_tools already found and documented this
exact issue (
test/install/.gitignore's comment: "The install output isself-comparing... so it is never asserted and must not be tracked");
independently reaching the same conclusion here is exactly the kind of
convergence signal the exercise is looking for. Not fixable from this
repo (it's a pgxntool
test/installfeature property); worth flagging asa possible pgxntool-level generalization. Followed cat_tools's own fix:
gitignore
test/install/load.out, don't track it.CI attempt:
pg-build-test→make test && make verify-results(reverted)Tried this switch (item 5 above), pushed it, and watched real CI. It
immediately surfaced genuine pgTAP failures on PostgreSQL 9.3 and 9.6 (the
extension_drop/cat_toolsinstall path never actually succeeds there).Checked whether this was a regression from my own changes by looking at PR
#10's own baseline CI
(#10, run
30665031257):
PG 9.3 and 9.6 already show "3 of 3 tests failed" in the raw job log
there too — silently reported as a passing check because
pg-build-testhits the exact same
.IGNORE: installcheckmaskingRELEASE.mdalreadydocuments (its own
RELEASE.mdnote about PRs #6/#7 is this same class ofbug, just a different slice of the PG matrix). So this is not something my
PR broke; my
verify-resultsswitch was doing exactly its intended job —proving a real, pre-existing, currently-invisible failure. But fixing
cat_tools's install path on pre-PG10 is out of scope for
"test/install foundation" (it's a dependency-setup concern, likely
downstream of PR #10 or its own follow-up), so I reverted the
ci.ymlchange rather than have this PR's CI go red for a failure it didn't cause
and isn't responsible for fixing. Flagging this prominently: whoever
owns PR #10 / the eventual CI-hardening PR should know pre-PG10 support is
currently broken and silently reported as green.
Explicitly out of scope / deferred (per task instructions)
extension_dropversion was invented to makeupdatemode "really" testable — see point 2 above.
pg_upgradeCI job, nobin/test_existing-style CI-orchestrationscript, no bridge-update machinery — extension_drop has nothing
pg_upgrade-unsafe shipped yet, and there's no pg_upgrade CI leg to wirethe dependency guard into (a later PR's job).
TEST_SCHEMAGUC/make-var switch — explicitly told not to build one;test/sql/schema.sql's existing coverage covers this already.HISTORY.ascentry — perRELEASE.md's own convention, this isinternal test-infrastructure/CI work, not a user-facing change.
Local verification
make test(fresh, default mode): all 5 tests pass.make verify-results: passes cleanly (confirms thezzz_build.outregeneration was correct, not just "no crash").
make test-update TEST_UPDATE_FROM=1.0.0: structural smoke test, passes(no real update path exists yet, see above).
make test TEST_LOAD_SOURCE=existing ...against a real, pre-installeddatabase: passes; also verified the failure path against an empty one.