Adopt test/install, decoupling installation from the test suite

[jnasbyupgrade]

Stacked on #25 (pgxntool 2.3.0 bump). First of several phases adding update+upgrade (U&U) test coverage - see pgxntool/README.asc's `test/install` and "Update & Upgrade (U&U) Testing" sections, which this follows directly (and which, worth noting, cites this repo's own prior `test/sql/extension_tests.sql` at an old commit as a schema-targeting example, with an explicit caveat against copying its plpgsql-function-based assertion style - a caveat this redesign takes seriously).

What changed

• `test/install/load.sql`: installs count_nulls once, committed, before the main `test/sql/` schedule - replacing each test file's own per-test `CREATE EXTENSION` via `test/deps.sql`. Its own output isn't tracked (see `test/install/.gitignore`) - pg_regress resolves its expected/actual paths to the same file, so the diff is always self-identical regardless of content; correctness comes from the SQL itself failing loudly, not a textual comparison (matching cat_tools' own `test/install/load.sql`).
• Retired the old ad hoc pattern where each test file hardcoded its own literal schema name (`\set schema ...`) as a stand-in for schema-qualification coverage. `test/sql/simple.sql` (same assertions as `extension_tests.sql`, just a different hardcoded schema) is now a strict subset once there's one canonical install location - removed. `test/sql/sanity.sql`'s own bare `CREATE EXTENSION` would now conflict with test/install's already-committed install anyway, and its coverage was already a subset of `test__functionality` - removed too.
• `test/core/functions.sql`/`test/sql/extension_tests.sql`: dropped the now-undefined `:schema` psql variable references; simplified `test__check_ncs`/`test__shutdown__drop_all` to not assume a specific hardcoded target (a future schema-targeting phase will make these real assertions again).

Fresh-only, no schema targeting yet - deliberately scoped small. Schema-switching and update/upgrade mode selection are follow-up phases, stacked on this one.

Notable side effect

Since `test/install` runs in its own bare connection (not the in-suite session pgTAP's own `tap_setup.sql` runs in, which normally puts pgTAP's schema on `search_path` first), an untargeted `CREATE EXTENSION` now lands in `public` instead of pgTAP's own schema. This is actually a simplification: it's now the only possible landing location for "no schema specified," which incidentally forecloses a source of expected-output divergence between entry points (in-suite vs. an external bare-psql install) that a schema-targeting phase would otherwise have had to handle as a separate case.

Verification

`make test` / `make verify-results` pass against this container's live PG17 (2/2: `../install/load`, `extension_tests`).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 53a9c8f3-b486-4814-b911-7fcd367e79d1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 57s —— View job

---

Claude finished reviewing PR #27

• Gather context (repo CLAUDE.md, diff, changed files)
• Verify the mechanism claims in the PR description against the actual pgxntool vendor code
• Review each changed file for correctness
• Post review feedback

Summary

This is a clean, well-scoped adoption of pgxntool's test/install feature. I traced the actual mechanics through the vendored pgxntool/base.mk and pgxntool/README.asc to verify the PR's claims, and everything checks out:

• test/install/load.sql installs count_nulls once via a schedule pg_regress processes before test/sql/*, in the same invocation so the committed state persists (pgxntool/base.mk:280-301). The "self-diffing" trick — --inputdir/--outputdir both resolve to test, and the schedule's ../install/load relative path makes expected and actual paths collapse to the identical file test/install/load.out — is real and matches the documented mechanism, not just an assertion in the PR body.
• The schema-landing side effect is correct: test/pgxntool/tap_setup.sql:11 sets search_path = tap, public only within the in-suite connection; test/install's separate bare connection never sources that file, so its CREATE EXTENSION count_nulls (search_path defaulting to public) is exactly why test/expected/extension_tests.out now shows public.null_count(...) instead of schema_to_load_count_nulls.null_count(...).
• Removed test/sql/sanity.sql / simple.sql: confirmed sanity.sql's bare CREATE EXTENSION count_nulls would now fail (already installed by test/install), and its coverage (null_count over jsonb/nulls) is a subset of test__functionality in test/core/functions.sql:144-187. simple.sql was already a strict subset of extension_tests.sql's runtests() call. No orphaned :schema references remain anywhere in test/ after the cleanup.
• Makefile: PGXNTOOL_ENABLE_TEST_INSTALL = yes is a real, documented variable in this vendored pgxntool version (base.mk:132-138), and setting it explicitly (rather than relying on auto-detect) matches the stated intent — fail loudly if test/install/ ever ends up empty by accident.
• test/expected/extension_tests.out diff is fully consistent with the .sql changes (schema name substitutions, dropped assertions for the removed DROP SCHEMA step, test__check_ncs reduced to isnt(ncs(), NULL, ...)).
• CI is green (CI run passed).

Minor, non-blocking observation

test/install/.gitignore ignores load.out and install.out.diff. load.out is correct (it's the self-diffing artifact described above). install.out.diff doesn't correspond to anything actually produced by this vendored pgxntool — there's no per-test .out.diff generation in base.mk or its scripts (only a single aggregate regression.diffs), and even if such a file were ever produced for this test, the install file is named load.sql, so the artifact would presumably be load.out.diff, not install.out.diff. Likely copied from a generic template/example. Harmless either way since this test's diff is definitionally always empty, but worth double-checking against the cat_tools example this was modeled on.

No other issues found — the changes are minimal, correctly scoped to "fresh install only, no schema targeting yet" as stated, and internally consistent with the follow-up phases described in the PR body.

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 55s —— View job

---

Reviewing PR #27

• Gather context (CLAUDE.md, diff, changed files)
• Run code review
• Post findings

Summary

This is a clean, well-scoped adoption of pgxntool's test/install feature, matching the documented mechanism in pgxntool/base.mk/README.asc (confirmed by reading base.mk:125-164,280-301 directly). Since the last review pass, test/deps.sql was emptied out entirely — I checked pgxntool/README.asc:191 and it explicitly says this is the correct move once test/install is adopted ("test/deps.sql should no longer run CREATE EXTENSION itself... the committed install performed by test/install persists into every ... test file"), so that final commit is correct, not a leftover. make test's CI run for the current HEAD (5f0af6e) is green.

Finding

test/sql/extension_tests.sql:7-15 — comment misdescribes where count_nulls actually lands, contradicting the PR's own explanation and the expected output.

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

This says count_nulls is installed by test/install/load.sql, then in the same breath describes the landing schema as "in-suite, that's pgTap's own schema" — but test/install/load.sql runs in its own bare connection, not in-suite (as the PR body's own "Notable side effect" section correctly explains, and as test/pgxntool/tap_setup.sql:12's SET search_path = tap, public only ever applies to the in-suite connection that never sources it). The actual CREATE EXTENSION in test/install/load.sql runs with the default search_path of that bare connection, which is why test/expected/extension_tests.out shows public.null_count(...), not tap.null_count(...). The parenthetical describes what would happen if the extension were installed in-suite, which isn't what happens here — a future reader could easily conclude ncs() will resolve to tap, then be confused when it's public. Worth tightening to match the PR body's own (correct) explanation.

Fix this →

Other things checked, no issues found

• No orphaned :schema psql-variable references remain anywhere under test/ after the cleanup.
• test/sql/sanity.sql/simple.sql removal is safe: sanity.sql's bare CREATE EXTENSION count_nulls would now conflict with the already-committed install; its and simple.sql's coverage is a strict subset of test__functionality/extension_tests.sql.
• Makefile's PGXNTOOL_ENABLE_TEST_INSTALL = yes is a real, documented variable (base.mk:132-138) and fails loudly (rather than silently falling back) if test/install/ is ever emptied by accident — matches stated intent.
• test/expected/extension_tests.out diff is fully consistent with the .sql changes (schema substitutions, dropped DROP SCHEMA assertion, test__check_ncs reduced to isnt(ncs(), NULL, ...)).
• The minor test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces — the real self-diffing artifact is load.out) noted in the prior review pass is still present; harmless since the diff is definitionally always empty either way, but still worth a quick check against the cat_tools example this was modeled on.

[github-actions]

Claude finished @jnasbyupgrade's task in 6m 4s —— View job

---

Reviewing PR #27

• Gather context (CLAUDE.md, diff, changed files)
• Check whether previously-flagged issue was addressed
• Review new commit (b9e1d33) for correctness
• Post findings

Summary

Since the last review pass (5f0af6e), one new commit was pushed: b9e1d33 — "Guard search_path against cross-test contamination", adding _null_count_test.startup__capture_search_path / _null_count_test.teardown__search_path_unchanged to test/core/functions.sql. No CLAUDE.md exists in this repo to check against. CI (31033179792) is green for the current HEAD.

Design of the new guard looks sound. startup__capture_search_path runs once, before any per-test SAVEPOINT/ROLLBACK machinery, so the TEMP TABLE search_path_baseline it creates survives across each test's rollback; teardown__search_path_unchanged then compares against it after every test and raises a plain exception on drift. Walking through the current test file, none of test__check_ncs/test__definition/test__functionality/test__shutdown__drop_all issue a session-level SET search_path, so there's no false-positive trip expected today — this is purely a regression guard for future tests, as intended. Neither hook produces TAP output on success (no RETURN NEXT), which is why test/expected/extension_tests.out didn't need to change for this commit.

Previously-flagged issue: still open

A prior review pass on this PR flagged test/sql/extension_tests.sql lines 7-15, and this commit did not touch that file (only test/core/functions.sql), so the issue remains. The comment there still reads:

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

This still contradicts the PR's own "Notable side effect" explanation: test/install/load.sql runs in its own bare connection that never sources test/pgxntool/tap_setup.sql, so count_nulls actually lands in public (matching test/expected/extension_tests.out's public.null_count(...)), not in pgTAP's schema. Worth tightening before merge so a future reader isn't misled into expecting ncs() to resolve to tap.

Fix this →

Other things checked, no new issues found

• test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces) is still present from prior reviews — harmless, non-blocking.
• No orphaned :schema psql-variable references anywhere under test/.
• No other logic issues found in the new startup__/teardown__ guard functions.

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 12s —— View job

---

Reviewing PR #27

• Gather context (diff vs origin/master, changed files)
• Check whether previously-flagged issue was addressed
• Review new commits (5f0af6e, b9e1d33, dac37ca) for correctness
• Check CI status
• Post findings

Summary

Since the last review pass, three commits were added: 5f0af6e (empty out test/deps.sql entirely), b9e1d33 (add a search_path cross-test-contamination guard), and dac37ca (simplify that guard to compare against a static literal instead of a captured TEMP TABLE). CI for the current HEAD is green.

• test/deps.sql emptied out: confirmed against pgxntool/README.asc:191, which explicitly says test/deps.sql should no longer run CREATE EXTENSION once test/install is adopted, since the committed install persists into every test file. Correct, not a leftover.
• test/core/functions.sql:37-49 — teardown__search_path_unchanged: sound as simplified. It compares current_setting('search_path') against the static literal '_null_count_test, tap' (the same value set two lines above SET SEARCH_PATH = _null_count_test, tap; at line 4), and raises a plain exception on drift — appropriate since pgTAP's runner reports an exception from a teardown__ function as "Test died: ..." against the test that ran before it, no ok()/is() needed. Dropping the earlier TEMP TABLE-based capture (from b9e1d33) in favor of a hardcoded literal is a reasonable simplification given there's no TEST_SCHEMA concept yet in this phase — nothing to capture at runtime that isn't already known statically. Walking through the current test file, none of test__check_ncs/test__definition/test__functionality/test__shutdown__drop_all issue a session-level SET search_path, so no false-positive trip is expected today; this is a forward-looking regression guard. Neither hook produces TAP output on success, consistent with test/expected/extension_tests.out not needing changes for this addition.

Previously-flagged issue: still open

A prior review pass flagged test/sql/extension_tests.sql lines 7-15, and none of the three new commits touch that file, so it's unresolved. The comment above test__check_ncs still reads:

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

This contradicts the PR's own "Notable side effect" section: test/install/load.sql runs CREATE EXTENSION in its own bare connection, which never sources test/pgxntool/tap_setup.sql's search_path change — so count_nulls actually lands in public (matching test/expected/extension_tests.out's public.null_count(...), public.not_null_count_trigger(...), etc.), not in pgTAP's schema. A future reader could reasonably expect ncs() to resolve to tap based on this comment, then be confused when it resolves to public. Worth tightening before merge to match the PR body's own correct explanation.

Fix this →

Other things checked, no new issues found

• test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces — the real self-diffing artifact is load.out) is still present from prior reviews; harmless, non-blocking.
• Makefile's PGXNTOOL_ENABLE_TEST_INSTALL = yes and test/install/load.sql are unchanged from prior passes and still match pgxntool/base.mk's documented mechanism.
• No orphaned :schema psql-variable references anywhere under test/.
• test/expected/extension_tests.out is internally consistent with the current .sql files; no drift.

[github-actions]

Claude finished @jnasbyupgrade's task in 59s —— View job

---

Reviewing PR #27

• Gather context (diff vs origin/master, changed files)
• Check whether previously-flagged issue was addressed
• Review new commit (afe204f) for correctness
• Post findings

Summary

Since the last review pass, one new commit was pushed: afe204f — "Cross-reference teardown__search_path_unchanged with test__check_ncs", touching only test/core/functions.sql:35-36. It adds a clarifying sentence to the comment above teardown__search_path_unchanged: "Doesn't check WHERE count_nulls actually landed - see test__check_ncs in test/sql/extension_tests.sql for that." This is accurate and a good addition on its own — it correctly scopes what the new guard does and doesn't cover, and points readers at the right companion check.

Previously-flagged issue: still open, and now cross-referenced

Three prior review passes flagged the comment above test__check_ncs in test/sql/extension_tests.sql:7-15. afe204f doesn't touch that file, so it's still unresolved:

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

This still contradicts the PR's own "Notable side effect" explanation: test/install/load.sql runs CREATE EXTENSION in its own bare connection, which never sources test/pgxntool/tap_setup.sql's search_path change — so count_nulls actually lands in public (matching test/expected/extension_tests.out's public.null_count(...)), not in pgTAP's schema.

This is now slightly more consequential than before: afe204f adds a direct cross-reference from teardown__search_path_unchanged pointing readers at this comment as the place to learn "where count_nulls actually landed" — so the incorrect explanation is no longer just locally misleading, it's now the destination of an explicit pointer from another file. Worth tightening before merge so a reader following that cross-reference isn't sent to a comment that gets the mechanism backwards.

Fix this →

Other things checked, no new issues found

• test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces) is still present from prior reviews — harmless, non-blocking.
• No orphaned :schema psql-variable references anywhere under test/.
• test/expected/extension_tests.out still consistent with current .sql files; afe204f's comment-only change requires no expected-output update.
  (branch)

[github-actions]

Claude finished @jnasbyupgrade's task in 1m 3s —— View job

---

Reviewing PR #27

• Gather context (diff vs origin/master, changed files, prior review history)
• Review new commit (6bbe4ce) for correctness
• Check CI status
• Post findings

Summary

Since the last review pass, one new commit was pushed: 6bbe4ce — "Make the search_path/test__check_ncs cross-reference a SEE ALSO paragraph", touching only test/core/functions.sql:32-39. It reformats the cross-reference added in afe204f from being folded into the end of a sentence into its own labeled SEE ALSO: paragraph:

 * SEE ALSO: test__check_ncs in test/sql/extension_tests.sql, which checks
 * WHERE count_nulls actually landed (a different risk than this check).

This is accurate and a reasonable readability improvement — no logic changed, purely a comment reflow. CI for the current HEAD is green.

Previously-flagged issue: still open

Four prior review passes have flagged the comment above test__check_ncs in test/sql/extension_tests.sql:7-15, and 6bbe4ce doesn't touch that file, so it remains unresolved:

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

This still contradicts the PR's own "Notable side effect" explanation: test/install/load.sql runs CREATE EXTENSION in its own bare connection, which never sources test/pgxntool/tap_setup.sql's search_path change — so count_nulls actually lands in public (matching test/expected/extension_tests.out's public.null_count(...)), not in pgTAP's schema. The new SEE ALSO paragraph in functions.sql now points readers here even more prominently as "the place to learn where count_nulls actually landed," which makes fixing this text before merge more worthwhile, not less — a reader who follows that cross-reference is sent to an explanation that gets the mechanism backwards.

Fix this →

Other things checked, no new issues found

• test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces — the real self-diffing artifact is load.out) is still present from prior reviews; harmless, non-blocking.
• No orphaned :schema psql-variable references anywhere under test/.
• test/expected/extension_tests.out still consistent with current .sql files; 6bbe4ce's comment-only change requires no expected-output update.
  (branch)

[github-actions]

Claude finished @jnasbyupgrade's task in 1m 37s —— View job

---

Reviewing PR #27

• Gather context (diff vs origin/master, changed files, prior review history)
• Review new commits since last pass (cae2370, 0d1222b)
• Check CI status
• Post findings

Summary

Since the last review pass (6bbe4ce), two new commits were pushed: cae2370 — "Reciprocate the teardown__search_path_unchanged cross-reference" and 0d1222b — "test/deps.sql: explain why it's empty and kept". CI for the current HEAD (0d1222b) is green.

• cae2370 (test/sql/extension_tests.sql:15-18): adds a reciprocal SEE ALSO paragraph pointing from test__check_ncs's comment back to teardown__search_path_unchanged in test/core/functions.sql, correctly describing it as guarding "a different risk than this check" (drift vs. landing location). Accurate, comment-only, no logic change.
• 0d1222b (test/deps.sql): replaces the empty file with an explanatory comment. I checked both claims against pgxntool/README.asc directly: the "why it can't be deleted" claim (vendored setup.sql unconditionally does \i test/deps.sql, and it's one of the two files 3-way-merged on git subtree pull) matches README.asc:275 exactly (test/deps.sql is explicitly named as one of the reconciled setup.sh-copied files, alongside .gitignore). Accurate and a genuine improvement over a silently-empty file.

Previously-flagged issue: still open

Five prior review passes have flagged the comment above test__check_ncs in test/sql/extension_tests.sql:7-14, and neither new commit touches that text (the reciprocal SEE ALSO in cae2370 was appended after it, at line 16, leaving the original wording untouched):

/*
 * count_nulls is installed by test/install/load.sql with no schema
 * targeting - it lands wherever the session's own search_path resolves at
 * CREATE EXTENSION time (in-suite, that's pgTap's own schema, put on
 * search_path first by test/pgxntool/tap_setup.sql). ...
 */

I re-verified this against the current tree: test/install/load.sql's CREATE EXTENSION count_nulls runs in pg_regress's own bare connection for the ../install/load schedule entry, which never sources test/pgxntool/tap_setup.sql's SET search_path = tap, public (that file is only \i'd from the in-suite tap_setup.sql path used by test/sql/*). Consistent with that, test/expected/extension_tests.out shows public.null_count(anyarray), public.not_null_count(...), etc. throughout — not tap.*. So the comment's parenthetical ("in-suite, that's pgTap's own schema...") still describes a scenario that doesn't match how this extension is actually installed, contradicting the PR's own "Notable side effect" write-up.

This keeps getting more consequential rather than less: there are now two separate cross-references converging on this comment — teardown__search_path_unchanged's SEE ALSO (from afe204f/6bbe4ce) and now cae2370's reciprocal pointer back — both sending readers to a comment whose core claim about where count_nulls lands is backwards.

Fix this →

Other things checked, no new issues found

• test/install/.gitignore's install.out.diff entry (doesn't correspond to any file this vendored pgxntool actually produces — the real self-diffing artifact is load.out) is still present from prior reviews; harmless, non-blocking.
• No orphaned :schema psql-variable references anywhere under test/.
• test/expected/extension_tests.out still consistent with current .sql files; both new commits are comment-only and require no expected-output update.
  (branch)