CI: reformat Test strategy comment, document pg-tle-test/pg-tle-upgrade-test - #77
Conversation
…le-upgrade-test The "Test strategy" summary listed job names in a manually space-padded "name -- description" column, which read like a Makefile target list or a formatted spec table rather than a plain comment. Restructured each entry as a small heading (the job name alone, minimally indented) with its description as ordinary wrapped prose underneath. Also documented pg-tle-test and pg-tle-upgrade-test, which this summary never mentioned even though both jobs already exist (added in PR Postgres-Extensions#47) -- and trimmed wordiness in the `test` job's own step comment. Comment-only: no job, matrix, or CI-behavior change.
|
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 |
|
Claude finished @jnasbyupgrade's task in 1m 22s —— View job Claude finished reviewing PR #77
ReviewVerified the reformatted "Test strategy" block against the actual job definitions further down the file — the heading + prose content for One process finding: per this repo's own No functional or accuracy issues found in the diff itself. |
jnasbyupgrade
changed the title
Each entry's job name sat alone on its own line, unindented -- reads like a
Makefile target declaration rather than a reference to a CI job name, and
this repo's own Makefile has real hyphenated targets (test-long,
test-update, ...) a reader could easily conflate these with. Only one of
these names (test) even has a corresponding make target; formatting the
rest identically was actively misleading. Reworded each into an ordinary
prose sentence ("The `test` job runs...") so the name reads as a backticked
reference inside a sentence, not a declaration.
|
Claude finished @jnasbyupgrade's task in 2m 50s —— View job Claude finished reviewing PR #77
ReviewRe-reviewed after the new commit ( Checked the full rewritten block (
Minor, non-blocking nit: the first commit's subject ( No functional or accuracy issues found. |
…k extension-update-test to PG10-only extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as the `test` job (supported_pg, 12-18), but as its own matrix job: its own runner, container boot, checkout, apt-get, and `make install`, paid again per major, for a check that can run as one more step inside a container the `test` job already has running, already checked out, and already has cat_tools installed on disk in (installcheck, a TEST_DEPS prerequisite, already ran as a side effect of that job's own verify-results call). Folded `bin/test_existing update-scenario cat_tools_update 0.2.2` in as an additional call in the `test` job's "Test on PostgreSQL" step instead. Verified before folding it in, not assumed: ran `make check-relkind-source && make verify-results && bin/test_existing update-scenario cat_tools_update 0.2.2` in the same shell/cluster session (mirroring the new CI step exactly) against a scratch cluster. Confirmed no database-name collision (pg_regress's own throwaway db is named independently from `cat_tools_update`), the dependency-guard proof fires (twice -- once right after CREATE EXTENSION, once again after the full suite run), the structural-diff check (bin/structural_diff, from PR Postgres-Extensions#55) fires and reports the updated database structurally identical to a fresh install, and the full suite passes -- exit 0 end to end. extension-update-test now runs PG10 only, with no matrix at all (single source of truth: needs.changes.outputs.legacy_pg, not a hardcoded "10") -- its entire remaining purpose is the pre-0.2.2 legacy-script checks, the only place those scripts still load. Removed the now-dead `if: matrix.pg != '10'` / `if: matrix.pg == '10'` guards throughout that job (nothing left to guard against once there's no other leg) and the "Update 0.2.2 -> current" step (moved above). The `changes` job's `update_pg` output/derivation is removed too -- it had exactly one consumer, and that consumer is gone. Only the minimal comment updates needed to describe this diff: the `test` and `extension-update-test` entries in the Test strategy summary (added previously in Postgres-Extensions#77, which this is based on), and the cross-references in `pg-tle-test`'s own comment that pointed at extension-update-test for the update path it no longer covers. No coverage lost: the PG12+ update-to-current check still runs on the exact same 7 majors it always did (moved, not removed), the PG10 legacy checks are byte-for-byte unchanged, and every other job is untouched.
1 participant
The "Test strategy" summary comment listed job names in a manually space-padded "name -- description" column, which read like a Makefile target list or a formatted spec table rather than a plain comment. Restructured each entry as a small heading (the job name alone, minimally indented) with its description as ordinary wrapped prose underneath -- same content, no alignment bookkeeping.
Also documented
pg-tle-testandpg-tle-upgrade-test, which this summary never mentioned even though both jobs already exist (added in PR #47) -- and trimmed wordiness in thetestjob's own step comment (theverify-resultsexplanation).What changed
name -- descriptiontable to a heading + wrapped-prose layout.pg-tle-test/pg-tle-upgrade-testentries, previously undocumented here.testjob step'sverify-resultscomment for wordiness.#-prefixed comment.Sequencing note
This is being landed ahead of a separate, purely functional PR (folding
extension-update-test's PG12+ leg into thetestjob) specifically so that PR's own diff stays clean and doesn't fight with this comment rewrite -- see that PR for the actual behavioral change. Please merge this one first.Test plan
git diff | grep '^+', all#-prefixed)make lintclean