Add test/install foundation for update+upgrade testing (fresh/update/existing) - #22
Conversation
…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>
|
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:
📝 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 |
![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
| 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.
| 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
| 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.
| 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
| 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
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.
1 participant
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).Built on top of
master(already has pgxntool 2.2.0 via #21, which is whatprovides
PGXNTOOL_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
test/sql/install.sql's own non-CASCADEDROP EXTENSION(a deliberatefresh-mode-only test of drop/recreate) is exactly the kind of thing the
guard exists to catch if it ran under existing mode -- so both
install.sqland the install-ordering half ofpgtap.sqlare skippedunder
test_load_mode=existinginstead.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,install,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).