Add test/install foundation for update+upgrade testing (fresh/update/existing) - #22
Closed
jnasbyupgrade wants to merge 3 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThe test system now supports Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant pg_regress
participant FoundationInstaller
participant PostgreSQL
Developer->>Makefile: run test with load mode
Makefile->>pg_regress: provide TEST_LOAD_SOURCE and PGOPTIONS
pg_regress->>FoundationInstaller: execute test/install/load.sql
FoundationInstaller->>PostgreSQL: validate mode and inspect extensions
FoundationInstaller->>PostgreSQL: reset, install, update, or guard extension state
pg_regress->>PostgreSQL: run mode-aware regression tests
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
3 tasks
jnasbyupgrade
marked this pull request as ready for review
July 30, 2026 22:02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 11-12: Correct the TEST_LOAD_SOURCE validation in the Makefile so
it checks the variable value against the exact allowed tokens fresh, update, and
existing, rather than treating it as a filter pattern. Ensure wildcard values
and multi-word values such as “fresh update” fail validation before being
exported through PGOPTIONS.
In `@test/CLAUDE.md`:
- Around line 53-57: Update the command example near the real pre-existing
install instructions by declaring its fenced block as bash and adding blank
lines immediately before and after the fence, preserving the command unchanged.
In `@test/helpers/create_extension.sql`:
- Around line 9-12: Adjust the subquery formatting in the already_installed
query so the WHERE clause begins on its own line, satisfying SQLFluff LT14 while
preserving the existing existence check and result.
In `@test/install/load.sql`:
- Around line 48-50: Update test/install/load.sql lines 48-50 to record whether
this run created test_role, then add post-suite teardown to drop only that role.
In test/install/load.sql lines 110-112, remove the test_factory_drop_guard.guard
view and schema during the same teardown without deleting pre-existing user
objects; ensure the committed-session test flow cleans up all artifacts
automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f6b95e1-08dc-4604-a332-1f31c20c8d1e
⛔ Files ignored due to path filters (4)
test/expected/base_1.outis excluded by!**/*.outtest/expected/install_1.outis excluded by!**/*.outtest/expected/pgtap_1.outis excluded by!**/*.outtest/install/load.outis excluded by!**/*.out
📒 Files selected for processing (9)
Makefiletest/CLAUDE.mdtest/helpers/create.sqltest/helpers/create_extension.sqltest/helpers/deps.sqltest/install/load.sqltest/roles.sqltest/sql/install.sqltest/sql/pgtap.sql
Comment on lines
+11
to
+12
| ifeq ($(filter $(TEST_LOAD_SOURCE),fresh update existing),) | ||
| $(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject wildcard and multi-word load modes.
Line 11 reverses filter’s pattern/text arguments, so values such as % or fresh update pass validation and are exported through PGOPTIONS. Validate an exact, single allowed token instead.
Proposed fix
-ifneq ($(filter $(TEST_LOAD_SOURCE),fresh update existing),)
+ifneq ($(words $(TEST_LOAD_SOURCE)),1)
+$(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)')
+endif
+ifeq ($(filter fresh update existing,$(TEST_LOAD_SOURCE)),)
$(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)')
endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 11 - 12, Correct the TEST_LOAD_SOURCE validation in
the Makefile so it checks the variable value against the exact allowed tokens
fresh, update, and existing, rather than treating it as a filter pattern. Ensure
wildcard values and multi-word values such as “fresh update” fail validation
before being exported through PGOPTIONS.
Comment on lines
+53
to
+57
| Run against a real pre-existing install with: | ||
| ``` | ||
| make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> \ | ||
| EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the command example lint-clean.
Add blank lines around the fence and declare the shell language.
Proposed fix
Run against a real pre-existing install with:
- ```
+
+ ```bash
make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> \
EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
Run against a real pre-existing install with:
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/CLAUDE.md` around lines 53 - 57, Update the command example near the
real pre-existing install instructions by declaring its fenced block as bash and
adding blank lines immediately before and after the fence, preserving the
command unchanged.
Source: Linters/SAST tools
Comment on lines
+9
to
+12
| SELECT | ||
| current_setting('test_factory.test_load_mode') <> 'fresh' | ||
| AND EXISTS (SELECT 1 FROM pg_extension WHERE extname = :'extension_name') | ||
| AS already_installed |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the SQLFluff layout violation.
Line 11 places WHERE inline inside the subquery, which violates enforced LT14.
Proposed fix
- AND EXISTS (SELECT 1 FROM pg_extension WHERE extname = :'extension_name')
+ AND EXISTS (
+ SELECT 1
+ FROM pg_extension
+ WHERE extname = :'extension_name'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SELECT | |
| current_setting('test_factory.test_load_mode') <> 'fresh' | |
| AND EXISTS (SELECT 1 FROM pg_extension WHERE extname = :'extension_name') | |
| AS already_installed | |
| SELECT | |
| current_setting('test_factory.test_load_mode') <> 'fresh' | |
| AND EXISTS ( | |
| SELECT 1 | |
| FROM pg_extension | |
| WHERE extname = :'extension_name' | |
| ) | |
| AS already_installed |
🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 11-11: The 'WHERE' keyword should always start a new line.
(LT14)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helpers/create_extension.sql` around lines 9 - 12, Adjust the subquery
formatting in the already_installed query so the WHERE clause begins on its own
line, satisfying SQLFluff LT14 while preserving the existing existence check and
result.
Source: Linters/SAST tools
Comment on lines
+48
to
+50
| SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_role \gset | ||
| \if :need_role | ||
| CREATE ROLE :test_role; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not leave committed test artifacts in the target database.
load.sql runs in its own committed session, so a newly created test_role and test_factory_drop_guard.guard persist after the suite. In existing mode this mutates the real pre-existing target; subsequent runs can also overwrite the guard view.
test/install/load.sql#L48-L50: record whethertest_rolewas created by this run and remove it in a post-suite teardown.test/install/load.sql#L110-L112: remove the guard view/schema in that same teardown, without deleting pre-existing user objects.
As per coding guidelines, “Run tests in transactions with automatic rollback so test data is cleaned up after execution.”
🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 48-48: The 'WHERE' keyword should always start a new line.
(LT14)
📍 Affects 1 file
test/install/load.sql#L48-L50(this comment)test/install/load.sql#L110-L112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/install/load.sql` around lines 48 - 50, Update test/install/load.sql
lines 48-50 to record whether this run created test_role, then add post-suite
teardown to drop only that role. In test/install/load.sql lines 110-112, remove
the test_factory_drop_guard.guard view and schema during the same teardown
without deleting pre-existing user objects; ensure the committed-session test
flow cleans up all artifacts automatically.
Source: Coding guidelines
This was referenced Jul 30, 2026
Closed
jnasbyupgrade
added a commit
to jnasbyupgrade/test_factory
that referenced
this pull request
Aug 4, 2026
Same root cause as the syntax.out regeneration two commits back: these files were authored on this branch's original fork-based master, which predates upstream PR Postgres-Extensions#24 (adds the sql-lint tool and its comment-stacked-dashes rule, 3+ consecutive -- lines must use /* */). Rebasing onto sync-pgxntool-2.3.0 makes the linter apply to them for the first time. Watch for the same nested-block-comment hazard PR Postgres-Extensions#22's own comment conversion hit (see commit 6a0627d "Convert multi-line SQL comments to /* */ style"): syntax.sql's original wording included the literal substring "sql/*--*.sql", where the "/*" opens a second, unwanted nested comment (Postgres block comments nest) -- reworded to avoid any "/*"/"*/" substrings appearing inside comment bodies, not just at the intended block boundaries.
…existing) Implements the load-mode switch, dependency guard, and single-source-of-truth role name from the advanced update+upgrade testing pattern (modeled on Postgres-Extensions/cat_tools PR Postgres-Extensions#16 and what has landed on its master since), scoped to what test_factory can actually exercise today: - test/install/load.sql (pgxntool's PGXNTOOL_ENABLE_TEST_INSTALL) now owns getting the extension to its target state via TEST_LOAD_SOURCE= fresh|update|existing, propagated as GUCs (test_factory.test_load_mode etc), validated at both make-parse-time and read time. - test/helpers/create_extension.sql skips CREATE EXTENSION when load.sql already installed it (update/existing), while keeping fresh mode's original no-IF-NOT-EXISTS behavior (hard error on stale state) unchanged. - test/sql/install.sql and the install-ordering half of test/sql/pgtap.sql are skipped under existing mode: their own non-CASCADE DROP EXTENSION is a deliberate fresh-mode-only test that would otherwise trip the guard. - Dependency guard (existing mode only): a view depending on tf.tap(text,text) blocks a stray non-CASCADE DROP EXTENSION test_factory_pgtap; test_factory itself already has a natural guard for free via test_factory_pgtap's own `requires` clause, which load.sql also proves still holds. - test/roles.sql is now the single source of truth for the test_role name, \i'd via test/helpers/deps.sql and test/install/load.sql. - Alternate expected output (test/expected/{base,install,pgtap}_1.out) for the existing-mode leg, since it legitimately produces different (but equally valid) output -- generated from real `existing`-mode runs, no raw "not ok" TAP lines in either leg. Verified against both PG12 and PG17: fresh (default), TEST_LOAD_SOURCE=update (currently a no-op -- see comment in load.sql for why no CI job drives it yet), and TEST_LOAD_SOURCE=existing against a real pre-populated database (the make test ... --use-existing recipe). Fresh mode's expected output is byte-for-byte unchanged from before this change. Skipped/deferred (see PR description): TEST_SCHEMA (test_factory is non-relocatable with hardcoded schema names, so the ambient-search_path failure mode it protects against can't occur here); an update-path CI job (no second version has ever shipped); the bridge-update/multi-origin machinery from PR Postgres-Extensions#16 (cat_tools-specific technical debt, not applicable). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Several review comments added by the test/install foundation work used consecutive -- lines for what was really one continuous remark. This repo's convention (see the pre-existing test/helpers/create.sql and test/sql/install.sql) is to use C-style /* */ blocks for any comment spanning more than one line, reserving -- for single-line remarks. While converting test/roles.sql, reworded "test/sql/*.sql" to "*.sql files under test/sql/" -- the original phrasing contained a literal /* immediately after test/sql, which Postgres's nesting-aware block comment parser reads as an unwanted nested comment opener, leaving the enclosing comment unterminated.
… crash test/CLAUDE.md still referenced test/sql/install.sql (deleted by add-test-build, now this branch's base) in the existing-mode section and the alternate-expected-file list; updated both to reflect that packaging/dependency-declaration checks now live in test/build/install.sql instead. Also fixes a real, pre-existing bug in test/sql/pgtap.sql surfaced by testing TEST_LOAD_SOURCE=update locally (not just the fresh/existing legs the rebase itself required): test/install/load.sql's update mode installs test_factory only, never test_factory_pgtap. pgtap.sql's non-existing-mode branch calls test/helpers/create_extension.sql for test_factory (a no-op when already installed, per its own already_installed check) and then unconditionally DROP TABLEs pre_install_role/post_install_role -- which that no-op never created, so the DROP errored under update mode. Guarded the same way test/helpers/create.sql already guards its own read of those tables (to_regclass(...) IS NOT NULL), rather than inventing a new pattern.
jnasbyupgrade
force-pushed
the
advanced-testing/foundation
branch
from
August 4, 2026 22:20
6a0627d to
9d7d76b
Compare
jnasbyupgrade
added a commit
to jnasbyupgrade/test_factory
that referenced
this pull request
Aug 4, 2026
…trix Implements the remaining CI-structure items from the advanced update+upgrade testing pattern (modeled on Postgres-Extensions/cat_tools's ci.yml/ bin/test_existing and what has landed on its master since), stacked on the foundation from PR Postgres-Extensions#22 (TEST_LOAD_SOURCE, dependency guard, load.sql): - Scope `push` to `branches: [master]`; `pull_request` stays unrestricted -- fixes the double-triggered-CI-on-every-PR-commit bug (test_factory was named as one of the repos still needing this). - Job-level `changes` gate: computes a per-push docs-only diff (fail-safe = not-docs-only as the literal first line) instead of a workflow-level `paths-ignore`, so heavy jobs can skip on doc-only pushes without leaving all-checks-passed stuck Pending. - Single source of truth for the supported-PostgreSQL-major list (NEWEST=17, FLOOR=10, matching the existing matrix), derived once in `changes` and consumed via fromJSON by both the `test` and new `pg-upgrade-test` matrices. - `all-checks-passed` gate, self-checking that its `needs` list matches the actual job set. - New `pg-upgrade-test` job: binary pg_upgrade legs (10->17, 16->17) -- install the current version on an old cluster, pg_upgrade to a newer major, then run the suite against the migrated objects in existing mode. No bridge step needed: test_factory has shipped only one version, so every leg installs current directly on the old cluster. Mechanics factored into `.github/scripts/pg_upgrade_cluster` (generic, modeled on cat_tools's script of the same name) and `bin/test_existing` (test_factory-specific, much smaller than cat_tools's own since there's no bridge/multi-origin machinery to carry -- prepare-old + run-suite is the whole surface). - `test` job now gates on `make verify-results` instead of pgxn-tools' `pg-build-test`: pgxntool marks installcheck `.IGNORE`, so the old job was silently exiting 0 even when regression.diffs was nonempty. PGXNTOOL_ENABLE_VERIFY_RESULTS is already pgxntool's own default but is now pinned explicitly in the Makefile, matching ENABLE_TEST_INSTALL's existing explicit-over-implicit convention. - Dynamic version assertion (bin/test_existing's assert_version): the installed version is always derived from `make -s print-PGXNVERSION`, never hardcoded, with empty-value guards on both sides. Real bug found by actually running the pg_upgrade dry run locally (PG12/16 -> PG17, using throwaway data directories, per the verification requirement -- not just written and trusted): test/helpers/create.sql's security-definer function check had no ORDER BY, so its row order depended on pg_proc's physical layout. That happens to match creation order on a fresh CREATE EXTENSION but is NOT preserved by pg_upgrade's dump/restore, which produced a real (but harmless -- every individual assertion still said "ok") text diff against the fresh-install expected output. Fixed with an explicit ORDER BY, which turns out to make one set of expected-output files valid for fresh, existing, AND pg_upgraded modes alike -- no third numbered alternate file needed, simpler than it first looked. Skipped, per the scoping decided before starting (see PR description for full reasoning): extension-update-test job (no second version has ever shipped), bridge/multi-origin update machinery (cat_tools-specific technical debt test_factory doesn't have), pg-upgrade-stepwise (test_factory has no catalog-internals-touching views/functions), pg_tle testing (not a deployment target), the `stable` pseudo-version (real feature work, not requested). Verified locally: make verify-results passes cleanly on both PG12 and PG17 (shared clusters); a full old-cluster-install -> pg_upgrade -> new-cluster existing-mode suite run (PG12->PG17 and PG12->PG16, using scratch data directories, never touching the shared clusters) passes with zero raw "not ok" TAP lines using the actual committed bin/test_existing and pg_upgrade_cluster scripts, not just ad hoc commands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 4, 2026
Collaborator
Author
|
Superseded by #34 -- recreated as an upstream-branch PR so it can be part of a formal GitHub stack ( |
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
1 participant
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.
Rebase note
Rebased onto
add-test-build(this stack is nowsync-pgxntool-2.3.0->add-test-build-> this PR -> #23; coordinator is handlinggh stack linkseparately).add-test-build(#26) deletedtest/sql/install.sqland trimmed the leading dependency-check block fromtest/sql/pgtap.sql(moved totest/build/install.sql); this PR's own mode-gating touched both of those same files, so real conflicts had to be resolved with judgment rather than a mechanical merge:test/sql/install.sqlno longer exists after Add test/build: extension-metadata tests + raw SQL syntax checks #26, so this PR's mode-gating commit for that file is now a no-op -- dropped rather than resurrecting the file. The alternate expected filetest/expected/install_1.outis correspondingly removed too (nothing left for it to be an alternate of).test/sql/pgtap.sql: kept Add test/build: extension-metadata tests + raw SQL syntax checks #26's trim (no dependency-check block, noSET search_path = tap) and re-derived the\if :is_existinggating around it -- the\else(fresh/update) branch is Add test/build: extension-metadata tests + raw SQL syntax checks #26's already-trimmed content; the existing-mode branch is unchanged (it never had the dependency-check block to begin with).test/CLAUDE.mdupdated to stop referencing the now-deletedtest/sql/install.sqland the now-three-file-turned-two-file alternate-expected-output list.A real, pre-existing bug found and fixed during this rebase's own local verification (not introduced by the rebase itself -- present in this PR's original design regardless):
TEST_LOAD_SOURCE=updatecrashed intest/sql/pgtap.sql.test/install/load.sql's update mode installstest_factoryonly, nevertest_factory_pgtap;pgtap.sql's non-existing-mode branch callstest/helpers/create_extension.sqlfortest_factory(a no-op since it's already installed) and then unconditionallyDROP TABLEspre_install_role/post_install_role-- which that no-op never created. Guarded the same waytest/helpers/create.sqlalready guards its own read of those tables (to_regclass(...) IS NOT NULL), rather than inventing a new pattern. This wasn't caught before because no CI job drivesupdatemode yet (per this PR's own "what's skipped" section below) -- found by testing it locally anyway while verifying the rebase.Re-verified all three modes locally after resolving conflicts: fresh (default),
update(now doesn't crash), andexistingagainst a real pre-populated database (tapschema + pgtap installed first, thentest_factory_pgtap CASCADE) -- zero rawnot okTAP lines in the fresh and existing legs, output byte-identical to the pre-recorded_1.outalternates.Summary
First of a small stack implementing the advanced update+upgrade (U&U)
testing pattern for this repo, modeled on
Postgres-Extensions/cat_toolsPR #16 and what has landed on its
mastersince (PR #44 test-harnessoverhaul, PR #46 update-path convergence, PR #48 PG-major single source of
truth). This PR is the foundation piece: the
test/install/load.sqlload-mode switch, the dependency guard, and single-sourcing the test role
name. A follow-up PR will restructure CI on top of this (docs-only gate,
PG-major single source of truth,
all-checks-passed, thepush/pull_requestdouble-trigger fix, and a binarypg_upgradejob).Now stacked on
add-test-build(#26) ->sync-pgxntool-2.3.0, in turn built on currentmaster(pgxntool 2.3.0, which is what providesPGXNTOOL_ENABLE_TEST_INSTALL/test/install/in the first place).What this adds
TEST_LOAD_SOURCE=fresh|update|existing(Makefile), propagated astest_factory.test_load_mode/test_update_from/test_update_toGUCs,validated at both
make-parse-time ($(error ...)) and read time (nomissing_ok, reject unknown values).test/install/load.sql: pgxntool'sPGXNTOOL_ENABLE_TEST_INSTALLfeature runs this once, committed, before the regular test files, so its
state survives into every one of them.
fresh(default) leavestest/sql/*.sqlto install the extension themselves exactly as before;updatedoesCREATE EXTENSION VERSION :from+ALTER EXTENSION UPDATE;existingonly asserts the extensions are present at the currentversion and never drops/creates.
test/helpers/create_extension.sqlnow skipsCREATE EXTENSIONwhenload.sqlalready installed it (update/existing), while fresh mode keepsits original behavior unchanged (no
IF NOT EXISTS, so a stale/unexpectedinstall still errors loudly instead of silently testing wrong state).
tf.tap(text,text)blocks a stray non-CASCADEDROP EXTENSION test_factory_pgtap.test_factoryitself doesn't need an artificialguard --
test_factory_pgtap's own control file (requires = 'pgtap, test_factory') already blocks a non-CASCADEDROP EXTENSION test_factoryas long as
test_factory_pgtapis installed, andload.sqlproves thatnatural protection still holds too. This matters because the
install-ordering half of
pgtap.sqlis exactly the kind of thing theguard exists to catch if it ran under existing mode -- so it's skipped
under
test_load_mode=existinginstead. (Packaging/dependency-declarationchecks that used to additionally live in
test/sql/install.sqlweremoved to
test/build/install.sqlby Add test/build: extension-metadata tests + raw SQL syntax checks #26, stacked below this PR.)test/roles.sql: single source of truth for thetest_rolename,\i'd viatest/helpers/deps.sql(test files) andload.sql(which nowowns creating the role idempotently, once, instead of each test file
doing its own unguarded
CREATE ROLE).(
test/expected/{base,pgtap}_1.out) for theexisting-mode leg,which legitimately produces different-but-correct output (skipped
sections, a skipped role-restore check) -- pg_regress's native numbered
alternate-file convention, generated from real
existing-mode runs.Verification
Ran on both PG12 and PG17:
make test(fresh, default) -- unchanged, byte-identical expected output.make test TEST_LOAD_SOURCE=update-- doesn't error (currently a no-op;see below).
make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=noagainst areal pre-populated database (extension installed,
tapschema set upfirst) -- passes cleanly with zero raw
not okTAP lines in either leg.make check-stale-expected-- clean (it already recognizes pg_regress's_N.outalternate-file convention).What's skipped, and why (per the doc's "ultimate goal" -- this record is
meant to help decide what's generic enough for pgxntool itself)
TEST_LOAD_SOURCE=updateyet. test_factory has onlyever shipped one version (0.5.0), so there's no historical update script
to exercise --
CREATE EXTENSION VERSION '0.5.0'+ALTER EXTENSION UPDATEis a no-op today and wouldn't prove anything fresh-mode CIdoesn't already cover. The mechanism is built now (per the doc's
checklist items 3-5) so it's ready the moment a second version ships.
TEST_SCHEMA(doc §3a) is not implemented. test_factory isnon-relocatable with hardcoded internal schema names (
tf,_tf,_test_factory_test_data-- seesql/test_factory.sql), notsearch_path-relative. The ambient-search_path failure mode
TEST_SCHEMAexists to catch (code accidentally assuming
public) structurally cannotoccur here. This is a genuine divergence, not an oversight.
That's cat_tools-specific technical debt (recovering from old
pg_upgrade-unsafe releases cut before this testing existed); nothingtest_factory has shipped needs it.
pg-upgrade-stepwise(every-major climb) and pg_tle testing are leftfor a possible follow-up, not this PR -- no evidence test_factory
targets pg_tle/RDS deployment, and test_factory has no catalog-touching
views (checked
sql/test_factory--0.5.0.sql) so the per-major-boundaryrisk
pg-upgrade-stepwiseprotects against is low.scope for an extension-level PR):
test/install/*.sql's actual output andits "expected" comparison target resolve to the same physical path
(
test/install/<name>.out, since--inputdir'sexpected/../install/and
--outputdir'sresults/../install/both collapse toinstall/).In practice this means pg_regress can never detect a real regression in a
test/install/*.sqlfile via output diffing -- it always overwrites and"passes". Worth a hard failure via
RAISE EXCEPTION-style assertionsinstead of relying on diffed output for anything that must actually catch
a regression (what
load.sqldoes throughout this PR).Convergence with cat_tools PR #16
propagation mechanism (
TEST_LOAD_SOURCE-> GUC ->current_setting(),no
missing_ok), the dependency-guard technique (plant a stable-memberview, prove drop is blocked, re-prove after every step), and "factor the
install->guard->assert flow into the install script itself" instead of
inline duplication.
type grown via
ADD VALUE; test_factory has no enum, so it targetstf.tap(text,text)instead (and gets test_factory's own protection forfree via
test_factory_pgtap'srequiresclause -- cat_tools has noequivalent second extension, so it needed an explicit guard for
everything).