diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index ffaef1f..4de1682 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -25,6 +25,25 @@ Addition specific to this paired repo, on top of the general convention in `../../ai/CLAUDE.md`: if you pushed to both pgxntool and pgxntool-test, start a background task for each — do not monitor them sequentially. +## Naming: `PGXNTOOL_` Is API, `_PGXNTOOL_` Is Internal + +- `PGXNTOOL_*` variables are user-facing override points: document them in + README.asc and treat any change as an API change. +- `_PGXNTOOL_*` variables (and `_pgxntool_*` functions) are internal-only, + including seams that exist so pgxntool-test can stub a script. Internal + make targets likewise start with `_` (e.g. `_check-stale-expected`), which + also keeps them out of `make list`. +- Internal variables that predate the convention (`TEST_DEPS`, + `TEST_SQL_FILES`, `TEST_BUILD_*`, `REGRESS_DBNAME`, `MAJORVER`, `GE91`, + ..., plus `PGXNTOOL_DIR`, which keeps its bare prefix) stay as they are, + since consumers may reference them. Don't rename them; give every new + internal variable the `_PGXNTOOL_` prefix. + +pgxntool-test's `test/standard/make-variables.bats` checks every variable +base.mk defines against the classified list in its +`test/lib/pgxntool-variables.txt`, so adding a variable means classifying +it there. + ## HISTORY.asc: Sorted Issue Numbers The "Issues fixed in this release:" line in HISTORY.asc must list issue diff --git a/HISTORY.asc b/HISTORY.asc index 05e7b7f..d2dbd83 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -67,7 +67,8 @@ same `_PGXNTOOL_` consistency): `PGXNTOOL_BASE_MK_INCLUDED` -> `_CHECK_STALE_EXPECTED_SCRIPT` -> `_PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT`. None of these are documented anywhere as override points, but if you happened to reference one directly (unsupported, but possible), update to the new -name. +name. The internal `check-stale-expected` target is likewise renamed to +`_check-stale-expected`; `make test` runs it as before. == Add `make results-build` Refreshes `test/build/expected/*.out` from the last `test-build` run's diff --git a/README.asc b/README.asc index 9824449..92f132d 100644 --- a/README.asc +++ b/README.asc @@ -206,7 +206,7 @@ Without `test/install`, each test file typically needs to run `CREATE EXTENSION` **Key detail:** Install files and regular tests run in a single `pg_regress` invocation. This means the database is NOT dropped between install and test phases — state created by install files persists into the main test suite. -WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself). +WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The *only* thing that still fails the build is a hard SQL error, and only if the file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (`_check-test-install-error-stop`, see `PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in `test/sql` instead (or use pgTap assertions from within the install file itself). ==== Update & Upgrade (U&U) Testing @@ -822,13 +822,13 @@ Default: `yes`. Enables or disables the <<_verify_results_safeguard,verify-resul === PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED * -Default: `yes`. Enables or disables the check-stale-expected safeguard, which fails `make test` if `test/expected/` (or `test/build/expected/`) contains a `.out` file with no corresponding `.sql` file -- catching a stale file left behind after a test was renamed or removed. Set to `no` to make the check a complete no-op (it's dropped from `TEST_DEPS` entirely). +Default: `yes`. Enables or disables the `_check-stale-expected` safeguard, which fails `make test` if `test/expected/` (or `test/build/expected/`) contains a `.out` file with no corresponding `.sql` file -- catching a stale file left behind after a test was renamed or removed. Set to `no` to make the check a complete no-op (it's dropped from `TEST_DEPS` entirely). This is also the supported pattern for disabling one specific optional pgxntool feature: a documented `PGXNTOOL_ENABLE_*` variable, gating both the target's registration into `TEST_DEPS`/etc. and (where applicable) the target's own definition. Cleanly replacing or overriding an arbitrary pgxntool-generated recipe with your own entirely is a separate, larger, and not-yet-decided design question -- see https://github.com/Postgres-Extensions/pgxntool/issues/30[issue #30] (two proposed approaches) -- and is *not* what this mechanism solves. === PGXNTOOL_CHECK_EXPECTED_FILE_TYPES * -Default: `yes`. Sub-check of check-stale-expected, independent of `PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED`: fails (with a distinct error message and exit code from the orphaned-`.out` check) if `test/expected/` (or `test/build/expected/`) contains any file that isn't `*.out`. Set to `no` to disable just this sub-check while leaving the orphaned-`.out` check active. +Default: `yes`. Sub-check of `_check-stale-expected`, independent of `PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED`: fails (with a distinct error message and exit code from the orphaned-`.out` check) if `test/expected/` (or `test/build/expected/`) contains any file that isn't `*.out`. Set to `no` to disable just this sub-check while leaving the orphaned-`.out` check active. === PGXNTOOL_NO_PGXS_INCLUDE diff --git a/README.html b/README.html index 5658ad6..10a92d7 100644 --- a/README.html +++ b/README.html @@ -926,7 +926,7 @@

Warning -test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The only thing that still fails the build is a hard SQL error, and only if the file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (check-test-install-error-stop, see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in test/sql instead (or use pgTap assertions from within the install file itself). +test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’s actual output is written to the exact same file as its expected output, so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. The only thing that still fails the build is a hard SQL error, and only if the file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql) — without it, psql prints the error, keeps going, and the file "passes" regardless. pgxntool enforces this by default (_check-test-install-error-stop, see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable), but that only guarantees errors are caught — it does not give you real output validation. If you need actual output comparison, put that logic in test/sql instead (or use pgTap assertions from within the install file itself). @@ -2172,7 +2172,7 @@

8.16. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *

-

Default: yes. Enables or disables the check-stale-expected safeguard, which fails make test if test/expected/ (or test/build/expected/) contains a .out file with no corresponding .sql file — catching a stale file left behind after a test was renamed or removed. Set to no to make the check a complete no-op (it’s dropped from TEST_DEPS entirely).

+

Default: yes. Enables or disables the _check-stale-expected safeguard, which fails make test if test/expected/ (or test/build/expected/) contains a .out file with no corresponding .sql file — catching a stale file left behind after a test was renamed or removed. Set to no to make the check a complete no-op (it’s dropped from TEST_DEPS entirely).

This is also the supported pattern for disabling one specific optional pgxntool feature: a documented PGXNTOOL_ENABLE_* variable, gating both the target’s registration into TEST_DEPS/etc. and (where applicable) the target’s own definition. Cleanly replacing or overriding an arbitrary pgxntool-generated recipe with your own entirely is a separate, larger, and not-yet-decided design question — see issue #30 (two proposed approaches) — and is not what this mechanism solves.

@@ -2181,7 +2181,7 @@

8.17. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *

-

Default: yes. Sub-check of check-stale-expected, independent of PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED: fails (with a distinct error message and exit code from the orphaned-.out check) if test/expected/ (or test/build/expected/) contains any file that isn’t *.out. Set to no to disable just this sub-check while leaving the orphaned-.out check active.

+

Default: yes. Sub-check of _check-stale-expected, independent of PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED: fails (with a distinct error message and exit code from the orphaned-.out check) if test/expected/ (or test/build/expected/) contains any file that isn’t *.out. Set to no to disable just this sub-check while leaving the orphaned-.out check active.

@@ -2217,7 +2217,7 @@

diff --git a/base.mk b/base.mk index 8d302cf..36c7e79 100644 --- a/base.mk +++ b/base.mk @@ -91,9 +91,9 @@ pgxntool_validate_yesno = $(strip \ # Helper function: normalize a single-word variable to lowercase and validate # it against a list of allowed values. -# Usage: $(call pgxntool_validate_choice,VALUE,VARIABLE_NAME,ALLOWED_VALUES) +# Usage: $(call _pgxntool_validate_choice,VALUE,VARIABLE_NAME,ALLOWED_VALUES) # Returns the lowercase value, or errors if it isn't exactly one of ALLOWED_VALUES. -pgxntool_validate_choice = $(strip \ +_pgxntool_validate_choice = $(strip \ $(if $(and $(filter 1,$(words $(1))),$(filter $(3),$(shell echo "$(1)" | tr '[:upper:]' '[:lower:]'))),\ $(shell echo "$(1)" | tr '[:upper:]' '[:lower:]'),\ $(error $(2) must be one of: $(3); got "$(1)"))) @@ -211,7 +211,7 @@ endif # pg_regress does report as a failure) if ON_ERROR_STOP is set. So it is # entirely up to each test/install/*.sql file to `\set ON_ERROR_STOP on` (or # `\i test/pgxntool/psql.sql`, which already does) if it wants failures -# caught at all. check-test-install-error-stop below enforces this by +# caught at all. _check-test-install-error-stop below enforces this by # default; see PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable it. # # Why not just force `-v ON_ERROR_STOP=1` onto the psql invocation instead of @@ -270,7 +270,7 @@ else endif # Variable: PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK -# - Gates check-test-install-error-stop (see TEST_DEPS wiring below): fails +# - Gates _check-test-install-error-stop (see TEST_DEPS wiring below): fails # the build if a test/install/*.sql file neither includes # test/pgxntool/psql.sql nor has its own `\set`/`\unset ON_ERROR_STOP` # (any value counts) -- see the IMPORTANT note @@ -284,7 +284,7 @@ else PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK = yes endif -_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check-test-install-error-stop.sh +_PGXNTOOL_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check-test-install-error-stop.sh # ------------------------------------------------------------------------------ # verify-results: Safeguard for make results @@ -320,10 +320,10 @@ endif # Default mode: pgtap (scans results/*.out for TAP failures) PGXNTOOL_VERIFY_RESULTS_MODE ?= pgtap -override PGXNTOOL_VERIFY_RESULTS_MODE := $(call pgxntool_validate_choice,$(PGXNTOOL_VERIFY_RESULTS_MODE),PGXNTOOL_VERIFY_RESULTS_MODE,pgtap diffs) +override PGXNTOOL_VERIFY_RESULTS_MODE := $(call _pgxntool_validate_choice,$(PGXNTOOL_VERIFY_RESULTS_MODE),PGXNTOOL_VERIFY_RESULTS_MODE,pgtap diffs) # ------------------------------------------------------------------------------ -# check-stale-expected: catch orphaned/unexpected test/expected/ files +# _check-stale-expected: catch orphaned/unexpected test/expected/ files # ------------------------------------------------------------------------------ # Variable: PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED # - Can be set manually in Makefile or command line @@ -338,16 +338,16 @@ override PGXNTOOL_VERIFY_RESULTS_MODE := $(call pgxntool_validate_choice,$(PGXNT # - Allowed values: "yes" or "no" (case-insensitive) # - Default: "yes" # - Set to "no" to disable just this sub-check while leaving the rest of -# check-stale-expected (the orphaned-.out check) active +# _check-stale-expected (the orphaned-.out check) active # - Passed through to check-stale-expected.sh; see that script for the # distinct error message/exit code this sub-check uses # # Variable: _PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT (internal shim, not user-facing) -# - Path to the script the check-stale-expected target invokes +# - Path to the script the _check-stale-expected target invokes # - Default: $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh # -# Implementation: See check-stale-expected target definition (search for -# "check-stale-expected:" in this file) +# Implementation: See _check-stale-expected target definition (search for +# "_check-stale-expected:" in this file) # ifdef PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED override PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED),PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED) @@ -442,11 +442,11 @@ installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR # installed (CREATE EXTENSION requires the control/SQL files to be in place). # PGXS's own installcheck target doesn't declare that dependency -- it assumes # the caller runs `make install installcheck` manually. That assumption breaks -# when something else (e.g. check-stale-expected below) depends on installcheck +# when something else (e.g. _check-stale-expected below) depends on installcheck # directly: `test`'s TEST_DEPS lists install/installcheck as independent, # unordered prerequisites, so nothing stops installcheck's own prerequisite # chain from running before install. An explicit edge here, same as -# check-stale-expected's, is the only ordering guarantee Make actually gives. +# _check-stale-expected's, is the only ordering guarantee Make actually gives. # # Gated behind PGXNTOOL_ENABLE_FS_INSTALL (see its definition above): when # disabled, `installcheck` must run against whatever is already installed @@ -467,7 +467,7 @@ endif TEST_DEPS = testdeps # ------------------------------------------------------------------------------ -# check-stale-expected: catch orphaned/unexpected test/expected/ files +# _check-stale-expected: catch orphaned/unexpected test/expected/ files # ------------------------------------------------------------------------------ # Purpose: test/expected/*.out must mirror test/sql/*.sql 1:1 (likewise # test/build/expected/*.out vs test/build/*.sql, when test-build is in use). @@ -485,30 +485,30 @@ TEST_DEPS = testdeps # See PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED / PGXNTOOL_CHECK_EXPECTED_FILE_TYPES # above for how to disable this entirely or just its non-.out file sub-check. ifeq ($(PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED),yes) -.PHONY: check-stale-expected -check-stale-expected: installcheck +.PHONY: _check-stale-expected +_check-stale-expected: installcheck @$(_PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT) $(TESTDIR) $(PGXNTOOL_CHECK_EXPECTED_FILE_TYPES) -TEST_DEPS += check-stale-expected +TEST_DEPS += _check-stale-expected endif # ------------------------------------------------------------------------------ -# check-test-install-error-stop: catch missing ON_ERROR_STOP in test/install +# _check-test-install-error-stop: catch missing ON_ERROR_STOP in test/install # ------------------------------------------------------------------------------ # Purpose: test/install/*.sql files never get a real pg_regress diff (see the # IMPORTANT note in the test/install section above) -- ON_ERROR_STOP is the # only thing that still turns a hard SQL error into a build failure. A file # passes if it includes test/pgxntool/psql.sql or has any `\set`/`\unset # ON_ERROR_STOP` command of its own. This is a pure static scan of file -# contents, so unlike check-stale-expected it doesn't need to run after +# contents, so unlike _check-stale-expected it doesn't need to run after # installcheck -- it needs no ordering edge at all. # # See PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK above to disable. ifeq ($(PGXNTOOL_ENABLE_TEST_INSTALL),yes) ifeq ($(PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK),yes) -.PHONY: check-test-install-error-stop -check-test-install-error-stop: - @$(_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT) $(TESTDIR) -TEST_DEPS += check-test-install-error-stop +.PHONY: _check-test-install-error-stop +_check-test-install-error-stop: + @$(_PGXNTOOL_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT) $(TESTDIR) +TEST_DEPS += _check-test-install-error-stop endif endif @@ -985,7 +985,7 @@ endif # # # This declaration is deliberately OUTSIDE the ifeq below (unlike e.g. -# check-stale-expected's own .PHONY, which lives inside its ifeq): testdeps' +# _check-stale-expected's own .PHONY, which lives inside its ifeq): testdeps' # own `testdeps: pgtap` prerequisite (see testdeps' definition) is # unconditional, so pgtap must always resolve to *some* rule. Without this # unconditional .PHONY, disabling the block below would leave `pgtap`