Skip to content

Add update+upgrade (U&U) test foundation: test/install, load-mode switch, TEST_SCHEMA, dependency guard - #3

Closed
jnasbyupgrade wants to merge 1 commit into
new_featuresfrom
u-and-u-foundation
Closed

Add update+upgrade (U&U) test foundation: test/install, load-mode switch, TEST_SCHEMA, dependency guard#3
jnasbyupgrade wants to merge 1 commit into
new_featuresfrom
u-and-u-foundation

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Owner

Summary

Foundation-only slice of the update+upgrade (U&U) testing pattern (§1-§4, §7,
checklist items 1-6/9/10 of the internal design doc this implements). A
follow-up PR will build the CI job (bin/test_existing-style script,
ci.yml matrix) on top of this branch. make test,
make test TEST_LOAD_SOURCE=update (or make test-update), and
make test TEST_LOAD_SOURCE=existing (paired with CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no) all now work
correctly by hand -- verified locally on both PG12 and PG17, including
deliberately-broken existing-mode scenarios (missing extension,
version-mismatched extension) to confirm the assert actually fails loudly
rather than silently passing.

Base branch note: this targets new_features on this fork (not
upstream master directly) because upstream (Postgres-Extensions/object_reference)
doesn't have a new_features branch at all -- it only exists here, feeding
into upstream via the already-open Postgres-Extensions/object_reference#2.
This PR stacks on top of that.

What this does

  • PGXNTOOL_ENABLE_TEST_INSTALL = yes + test/install/load.sql: a
    single, committed-once installer (runs once, before the whole pgTAP suite,
    in pgxntool's own test/install schedule) replacing the per-test
    CREATE EXTENSION object_reference CASCADE that used to live in
    test/load.sql. Three modes selected by an object_reference.test_load_mode
    GUC (fresh/update/existing), matching cat_tools PR Add SQL style linter (vendored Postgres-Extensions/linter) Postgres-Extensions/object_reference#16/#46's pattern.
  • TEST_LOAD_SOURCE/TEST_UPDATE_FROM/TEST_UPDATE_TO make vars,
    parse-time validated, exported unconditionally via PGOPTIONS, plus a
    make test-update convenience wrapper.
  • TEST_SCHEMA: a second, independent GUC for where the test session's
    ambient search_path targets (not how the extension got installed, but
    where). Read once via test/schema.sql, applied at install time
    (test/install/load.sql) and per-test (test/deps.sql, since
    search_path is session-local and doesn't carry over from the committed
    install session). Exercised locally with a mixed-case value
    (MixedCaseSchema) to confirm identifier quoting works correctly
    end-to-end.
  • sql/object_reference--0.1.0--0.2.0.sql: object_reference had no
    update path from 0.1.0 at all (only full install scripts existed), so
    ALTER EXTENSION UPDATE from 0.1.0 would have failed outright. This
    backfills a real, hand-authored script covering both actual deltas between
    the two frozen versions:
    1. Removing the reg* pseudotype columns from _object_reference._object_oid
      in favor of a single plain object_oid oid NOT NULL column (with
      classid also changed from regclass to oid) -- requires dropping
      and recreating _object_v/_object_v__for_update (can't CREATE OR REPLACE VIEW a column removal), backfilling object_oid from objid
      (safe because the old objid_must_match CHECK already guarantees
      they're equal), and dropping the now-pointless null_count trigger.
    2. Adding the object_reference.object__cleanup() function and the
      object_group__object__cleanup trigger that fires it.
    • sql/object_reference--0.1.0.sql and sql/object_reference--0.2.0.sql
      (the frozen versioned files) were not touched, per this repo's own
      convention -- only new files were added.
    • TEST_UPDATE_FROM defaults to 0.1.0, the only older released version
      that exists right now, so there's no multi-origin duplicity to worry
      about yet.
    • default_version in object_reference.control is untouched (still
      0.2.0) -- this PR backfills a missing historical path, it doesn't ship
      a new version.
  • Dependency guard (test/guard.sql, not wired into make test): a view
    in a new object_reference_drop_guard schema hard-depending on
    _object_reference.object.object_id -- the extension's own surrogate-key
    identity registry, which nothing in its update path has ever dropped or
    renamed. Manually verified: a non-CASCADE DROP EXTENSION fails with
    this guard planted, and CASCADE removes it cleanly along with the
    extension. Left for the follow-up CI PR to actually plant+prove
    automatically as part of an existing-mode flow.

What I took verbatim / adapted / skipped from cat_tools PR Postgres-Extensions#16/#46

  • Verbatim (adapted only in names/comments): the test_load_mode GUC
    dispatch structure in test/install/load.sql (fresh/update/existing
    \if blocks, the existing-mode DO block asserting
    extversion/default_version), the TEST_LOAD_SOURCE Makefile block
    (parse-time validation, unconditional PGOPTIONS export, the
    test-update wrapper), and the dependency-guard technique (a view
    hard-depending on a stable member).
  • Adapted: cat_tools needs pg_temp.drop_role()/pg_temp.create_role()
    helpers because its 0.2.2 install script uses a bare, non-idempotent
    CREATE ROLE. object_reference's own role creation (both 0.1.0 and 0.2.0)
    already wraps CREATE ROLE in a DO/EXCEPTION WHEN duplicate_object
    block, so none of that machinery is needed here -- plain
    DROP EXTENSION ... CASCADE is sufficient before a fresh/update
    re-install.
  • Skipped, deliberately: test/roles.sql. I grepped test/sql/*.sql and
    test/helpers/*.sql for role/grant usage first: object_reference's own
    install creates object_reference__usage/object_reference__dependency
    roles, and existing tests assert privileges on those roles
    (schema_privs_are(..., 'object_reference__dependency', ...)), but nothing
    creates or uses a test-actor role the way cat_tools's suite does to probe
    permission boundaries. Inventing that just to justify a roles.sql file
    would be scope creep beyond test-infrastructure work, so I skipped it.
    Also skipped: the bridge-update machinery (cat_tools-specific technical
    debt for its own old unsafe releases -- doesn't apply, object_reference has
    no known pg_upgrade-unsafe historical version) and testing multiple
    update origins (only one exists right now).
  • Invented (not in cat_tools): TEST_SCHEMA -- cat_tools's repo has zero
    hits for test_schema/TEST_SCHEMA, so this is doc guidance implemented
    ahead of the reference implementation, worth watching for
    convergence/divergence.

Bugs found (and fixed) only by actually running this, not just reviewing the diff

  • A PostgreSQL nested-comment gotcha: any comment containing a literal
    test/sql/*.sql-style glob accidentally opens a nested comment
    (l/* reads as a comment-start), and a stray nested */ inside a
    clarifying comment (reg*/object_oid) closes one early -- both produced
    real syntax errors that only surfaced when the affected file actually ran.
  • The update script's DROP VIEW _object_reference._object_v needs
    CASCADE: two functions declare RETURNS _object_reference._object_v
    (including, confusingly, a function of the same name as the other view,
    _object_reference._object_v__for_update), which only showed up as a real
    "other objects depend on it" error when run through actual
    ALTER EXTENSION UPDATE, not a plain psql -f (which additionally isn't
    a valid way to test this at all -- extension-member objects can only be
    dropped inside real update-script execution context).
  • The update script's own DDL trips object_reference's own
    zzz__object_reference_drop event trigger (fires on every sql_drop,
    including the update script's own DROP VIEW), which queries the exact
    view being dropped -- needed an explicit ALTER EVENT TRIGGER ... DISABLE
    /ENABLE around the update script.
  • test/install/load.sql needed \i test/pgxntool/psql.sql for
    ON_ERROR_STOP: pg_regress resolves the ../install/load schedule
    entry's expected-output path to the same file under both
    --inputdir/expected and --outputdir/results, making content-diffing a
    no-op self-comparison that can never fail on its own -- confirmed by
    deliberately breaking existing-mode assertions (missing extension,
    wrong version) and observing they were silently swallowed until this fix.
  • Adopting test/install broke two things that assumed a fresh CREATE EXTENSION per test (the old, pre-test/install behavior): zzz_build.sql
    (raw-source-load sanity check, now needs to drop its own session-local
    copy of the persistent schema first) and test/dump/load_all.sql (a
    separate, non-pg_regress database that now needs its own explicit
    CREATE EXTENSION, since test/load.sql no longer installs one for it).

Testing

Ran locally (both PG12 and PG17):

  • make test (fresh) -- all 8 pass
  • make test TEST_LOAD_SOURCE=update / make test-update -- all 8 pass,
    identical expected output to fresh (proves update ≡ fresh)
  • make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no against a
    genuinely fresh install -- all 8 pass; against a missing or
    version-mismatched install -- fails loudly and correctly, as designed
  • make test TEST_SCHEMA=MixedCaseSchema -- confirmed the schema is
    actually created and targeted, with only the expected extra
    "already exists" NOTICE as a diff
  • Manual guard proof (test/guard.sql): non-CASCADE DROP EXTENSION
    fails, CASCADE succeeds and removes the guard

🤖 Generated with Claude Code

…tch,

TEST_SCHEMA, dependency guard, and the missing 0.1.0->0.2.0 update script

Modeled on cat_tools PR Postgres-Extensions#16/#46's test/install/load.sql pattern (fresh /
update / existing load modes via a test_load_mode GUC propagated through
PGOPTIONS), adapted to object_reference's own conventions rather than
copied wholesale:

- test/install/load.sql: committed-once installer, replacing the
  per-test CREATE EXTENSION that used to live in test/load.sql. No
  test/roles.sql: object_reference's own tests never create actor
  roles to test permission boundaries (grepped test/sql and
  test/helpers to confirm), so there's nothing to centralize.
- TEST_LOAD_SOURCE/TEST_UPDATE_FROM/TEST_UPDATE_TO GUCs + a test-update
  wrapper target, same propagation mechanism as cat_tools.
- TEST_SCHEMA (doc's own guidance, not something cat_tools implements
  yet): a second, independent GUC for where the test session's ambient
  search_path points, read via test/schema.sql and applied both at
  install time and per-test (test/deps.sql), since search_path is
  session-local.
- sql/object_reference--0.1.0--0.2.0.sql: object_reference had no
  update path from 0.1.0 at all; this backfills one, converging the
  reg*-pseudotype removal and object__cleanup trigger forward from the
  frozen 0.1.0/0.2.0 sources (never edited directly).
- test/guard.sql: a dependency-guard view anchored on
  _object_reference.object.object_id, proven (manually) to block a
  non-CASCADE DROP EXTENSION. Not wired into `make test` -- it's for a
  follow-up CI PR's existing-mode flow.

Fixed along the way (surfaced by actually running fresh/update/existing
locally, not just reviewing the diff):
- zzz_build.sql's raw-source-load sanity check collided with the now-
  persistent object_reference schema/extension; it drops its own
  session-local copy first (transaction is never committed).
- test/dump/load_all.sql exercises a separate, non-pg_regress database
  and needs its own explicit CREATE EXTENSION now that test/load.sql
  itself no longer installs one.
- test/install/load.sql needs \i test/pgxntool/psql.sql for
  ON_ERROR_STOP: its expected-output path collapses to a self-
  comparison (../install/load resolves to the same file under both
  --inputdir/expected and --outputdir/results), so only a non-zero
  psql exit code -- not a content diff -- can actually fail it.
- the update script must disable the zzz__object_reference_drop event
  trigger for its own duration; _etg_drop() queries the very view the
  script drops and recreates.

Verified locally on PG12 and PG17: fresh, update (test-update), and
existing (against both a fresh install and deliberately broken states)
all behave correctly, and the TEST_SCHEMA switch was exercised with a
mixed-case value to confirm quoting works.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jnasbyupgrade

Copy link
Copy Markdown
Owner Author

CI status note: the current CI run fails, but this is a pre-existing, external dependency issue predating this branch, not something introduced here.

Root cause: pgxn install --unstable cat_tools in CI resolves to cat_tools 0.2.1 (whatever the real PGXN index currently publishes as "best unstable"), and a plain CREATE EXTENSION object_reference CASCADE; against that version fails with ERROR: column "oid" specified more than once -- unrelated to any test-infrastructure change in this PR.

Confirmed this predates this branch: the most recent CI run on the unmodified new_features base (run 30565168710, before this PR's commit) fails with the exact same error in the exact same place (psql:test/load.sql:7: ERROR: 42701: column "oid" specified more than once, i.e. the old per-test CREATE EXTENSION object_reference CASCADE). fail-fast cancels the other PG-version jobs once one fails, so it's not directly visible that they'd all hit it too, but since the failure isn't PG-major-specific (it's cat_tools 0.2.1's own SQL, cascaded in identically regardless of PG version), it's the same root cause across the matrix.

This is out of scope for this PR (no ci.yml changes here, per the task boundary with the follow-up CI PR) and isn't something a test-infrastructure change can fix -- it needs either a compatible cat_tools release on PGXN, or a version pin, which belongs in the CI PR's scope, not here.

Locally (which uses a locally-built/newer cat_tools, 0.3.0, rather than the PGXN-published 0.2.1), the exact same code passes cleanly in fresh, update, and existing modes on both PG12 and PG17 -- see the PR description's Testing section.

@jnasbyupgrade

Copy link
Copy Markdown
Owner Author

Superseded by upstream Postgres-Extensions#18 (u-and-u-testing), which implements the same U&U test foundation against the current stable-version model. Postgres-Extensions#18's own description already flags this PR as superseded (built against the outdated 0.2.0/reg*-pseudotype model, and includes a TEST_SCHEMA dimension that test-fixes.md's item 9 identifies as a mistake for this schema-pinned extension). Closing without merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant