CI: binary pg_upgrade testing, docs-only gate, single-source PG matrix - #23
Conversation
…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>
|
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 |
- test job: `make verify-results` alone races install vs installcheck under this container's ambient parallel make (they're independent prerequisites of the same `test` target) -- pg_regress could start, and fail with "extension ... is not available", before install's file copy finished. Split into two separate `make` invocations, which can't race with each other. Reproduced the failure mode's shape locally (though not the race itself -- couldn't get this container's make to lose the race on demand) and confirmed the two-step form still passes. - pg-upgrade-test job: never installed pgtap system-wide on either cluster. It worked by accident in local dry-runs only because this dev container already had pgtap installed for some PG majors from earlier testing -- confirmed by deliberately clearing /usr/share/postgresql/16/extension/ (a major this container had never used) and re-running the full recreate-old -> prepare-old -> pg_upgrade -> run-suite cycle end to end: it failed the same way PR Postgres-Extensions#23's CI did ("extension pgtap is not available"), then passed once both `pgxn install pgtap --sudo --pg_config ...` steps were added (old cluster before prepare-old, new cluster before its make install -- pg_upgrade itself needs pgtap available on the new cluster too, not just post-upgrade). --pg_config is explicit on both, not left to rely on pg-start's PATH-switching, since that's exactly the kind of ambient-state assumption that already broke once in this same job. Verified locally end-to-end (real pg_ctlcluster/pg_createcluster/pg_upgrade, not just make test): old=12/new=16, a pair this container had never exercised before, all the way through bin/test_existing run-suite with zero raw "not ok" TAP lines. Also re-confirmed plain `make test` still passes on PG12 and PG17 after these changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
First CI run failed everywhere (all 7 PG-matrix jobs + both pg_upgrade legs). Root-caused and fixed in 812e493:
Both fixes verified locally end-to-end (real |
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.
The ORDER BY rationale added for the pg_upgrade row-ordering fix used consecutive -- lines for one continuous remark; convert it to a /* */ block per the repo's comment convention (see the sibling foundation fix in advanced-testing/foundation).
1 participant
Summary
PR 2 of the advanced update+upgrade (U&U) testing stack, on top of #22
(
advanced-testing/foundation, not yet merged -- this PR targets thatbranch, not
master). Implements the remaining CI-structure items from theadvanced-extension-testing pattern, modeled on
Postgres-Extensions/cat_tools'sactual
ci.yml/bin/test_existing(read directly off itsmaster, not justprose about it) and scoped down deliberately per the plan agreed before
starting.
Note:
advanced-testing/foundationwas pushed to this repo (unchanged, samecommit as the fork's branch backing #22) purely so this PR's base could
reference it directly -- it is not new work, just a ref needed for a clean
stacked diff. Once #22 merges, this PR should be retargeted to
masteras afollow-up (not done here).
What shipped
pushscoped tobranches: [master];pull_requeststays unrestricted. test_factory was explicitlynamed as one of the repos still needing this.
changes/docs-only gate instead of workflow-levelpaths-ignore-- computes the real per-push diff, fail-safe (docs_only=false)written as the literal first line so any early exit/error leaves the safe
default in place. Avoids the stuck-Pending-required-check trap a
workflow-level
paths-ignorewould cause.(
NEWEST=17,FLOOR=10-- unchanged from the existing matrix), derivedonce in the
changesjob and consumed viafromJSONby both thetestand new
pg-upgrade-testmatrices.all-checks-passedgate, self-verifying its ownneeds:list matchesthe actual job set. This is the check to wire up as required in branch
protection -- I don't have permission to change that setting myself.
pg-upgrade-testjob: binarypg_upgradelegs (10 -> 17,16 -> 17) -- install the current version on an old cluster,pg_upgradeto a newer major, run the suite against the migrated objects in
existingmode. No bridge step: test_factory has shipped only one version (0.5.0),
so every leg installs current directly on the old cluster -- there's no
older,
pg_upgrade-unsafe install to carry forward.bin/test_existing(new, test_factory-specific) and.github/scripts/pg_upgrade_cluster(new, generic pg_upgrade CImechanics) -- the install -> pg_upgrade -> assert -> run-suite flow
factored into committed scripts instead of inline YAML per job, per the
doc's own guidance.
bin/test_existingis much smaller than cat_tools'sown: no bridge/multi-origin subcommands, just
prepare-old+run-suite.testjob now gates onmake verify-results, not pgxn-tools'pg-build-test. Found while rewriting this job:pg-build-test's ownmake installcheck || status=$?never actually catches a failure, becausepgxntool marks
installcheck.IGNORE--makeitself exits 0 thereregardless of
regression.diffs. The old CI was silently green on a realregression.
PGXNTOOL_ENABLE_VERIFY_RESULTSwas already pgxntool's owndefault, but I pinned it explicitly in the Makefile (matching
ENABLE_TEST_INSTALL's existing explicit-over-implicit convention) so afuture pgxntool default change can't silently disable the gate.
bin/test_existing'sassert_versionderives the expected version from
make -s print-PGXNVERSION, neverhardcoded, with empty-value guards on both sides (
"" != ""is false, so abroken extraction can't silently pass).
A real bug the local pg_upgrade dry run actually found
Per the brief, I ran the full old-cluster-install ->
pg_upgrade->new-cluster-suite-run cycle locally before trusting any of this in CI (PG12
-> PG17 and PG12 -> PG16, using throwaway
initdbdata directories, nevertouching the container's shared clusters) -- and it surfaced a real, if
low-stakes, bug:
test/helpers/create.sql's security-definer-function checkhad no
ORDER BY, so its row order depended onpg_proc's physical layout.That happens to match creation order on a fresh
CREATE EXTENSIONbut isnot preserved by
pg_upgrade's dump/restore (which reconstructs it in adifferent, apparently name-sorted, order) -- producing a real but harmless
text diff (every individual pgTAP assertion still said
ok, just reordered)against the fresh-install expected output. Fixed with an explicit
ORDER BY p.oid::regproc::text.Bonus: this makes ONE set of expected-output files valid for fresh,
existing, and pg_upgraded modes alike -- no third numbered alternate file
needed (unlike the doc's more general guidance for genuinely different
axes like
TEST_SCHEMA), since the divergence here was pure incidentalnon-determinism, not a legitimate different-but-correct scenario. Simpler
than it first looked once actually run.
Convergence / divergence from cat_tools PR #16 (and its
mastersince)Took near-verbatim (generic CI mechanics, no cat_tools-specific
content): the shape of
.github/scripts/pg_upgrade_cluster(
recreate-old/upgradesubcommands,INITDB_OPTSconvention, pg_upgradelog capture for both PG17+'s
pg_upgrade_output.d/and older's CWD), thechangesjob's docs-only fail-safe-first-line pattern, theall-checks-passedself-verifyingneedscheck, and the dynamicversion-assertion empty-guard pattern.
Adapted:
bin/test_existingkeeps only two subcommands(
prepare-old/run-suite) instead of cat_tools's six -- noplant-guard/update/update-scenario/update-check, becausetest_factory's dependency guard is planted and proved entirely inside
test/install/load.sql's existing-mode branch (from PR #22), not by thisscript, and there's no update path to exercise yet. The
changesjob alsodrops cat_tools's "find the last commit where real code changed" reporting
machinery -- useful polish, but not part of the checklist items this PR
scoped to; noting it here as a real simplification, not an oversight, in
case a human wants it added later.
Skipped, and why (all decided before starting, confirmed still correct
after doing the work):
extension-update-testjob -- test_factory has shipped only one version(0.5.0), so
TEST_LOAD_SOURCE=updatehas no real historical update scriptto exercise yet. The mechanism exists (PR Add test/install foundation for update+upgrade testing (fresh/update/existing) #22); no CI job drives it.
(recovering from old
pg_upgrade-unsafe releases). test_factory has nocatalog-touching views and only one shipped version, so there's nothing to
bridge from.
pg-upgrade-stepwise(every-major climb) -- test_factory has nocatalog-internals-touching views/functions (no
SELECT *over a systemcatalog), so the per-major-boundary risk this protects against is low. A
human may disagree and ask for it later; flagging explicitly rather than
silently omitting.
deployment.
stablepseudo-version -- real feature work, not part of atesting-infrastructure PR, not requested.
Verification
make verify-resultspasses cleanly (fresh mode) on both PG12 and PG17.pg_upgrade-> new-cluster-suite-run cycle,using the actual committed
bin/test_existingand.github/scripts/pg_upgrade_cluster(not just ad hoc commands), passeswith zero raw
not okTAP lines -- run twice, PG12->PG17 and PG12->PG16,in scratch data directories that never touched the container's shared
clusters.
.github/workflows/ci.ymlparses cleanly under PyYAML; both new shellscripts pass
bash -n.Test plan
test+pg-upgrade-test+all-checks-passed)master(follow-up, not done here)all-checks-passedas a required status check inbranch protection
🤖 Generated with Claude Code