From 7a7e9fb964edb48613eeeb056a898baa9704d320 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 7 Oct 2026 16:29:34 -0500 Subject: [PATCH] Validate PGXNTOOL_VERIFY_RESULTS_MODE Any value other than exactly `pgtap` silently selected `diffs` mode, so a typo such as `pgTap` quietly weakened the `make results` safeguard. - New `pgxntool_validate_choice` helper in `base.mk`: lowercases a single-word value and checks it against an allowed list, otherwise a parse-time `$(error)` naming the variable and the value. - `PGXNTOOL_VERIFY_RESULTS_MODE` now accepts `pgtap` or `diffs` case-insensitively; anything else (including empty) is an error. Mode selection for valid values is unchanged. - README.asc documents the accepted values and case handling; HISTORY.asc gets a STABLE entry. README.html is regenerated (it was already behind README.asc). Related changes in pgxntool-test: - `make-test.bats` covers default, mixed-case, invalid and empty values at the `print-` layer, plus mixed-case recipe selection via `make -n`. Co-Authored-By: Claude Opus 5.5 --- HISTORY.asc | 6 ++++ README.asc | 4 +-- README.html | 94 +++++++++++++++++++++++++++++------------------------ base.mk | 12 +++++++ 4 files changed, 72 insertions(+), 44 deletions(-) diff --git a/HISTORY.asc b/HISTORY.asc index 2e99b76..7ecc4ab 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -83,6 +83,12 @@ command, to any value; see `test/install/*.out` was never really compared against anything, and is rewritten by every run, it's now gitignored -- stop committing it. +== Reject invalid `PGXNTOOL_VERIFY_RESULTS_MODE` values +Any value other than exactly `pgtap` silently selected `diffs` mode, so a +typo like `pgTap` quietly weakened the `make results` safeguard. The value +is now case-insensitive, and anything other than `pgtap` or `diffs` is a +hard `make` error. + == Guarantee `test/install/*.sql` ordering Install files now always run in byte-value (ASCII) filename order, regardless of filesystem or locale. If one install file depends on another, diff --git a/README.asc b/README.asc index 012943c..c47121d 100644 --- a/README.asc +++ b/README.asc @@ -268,7 +268,7 @@ IMPORTANT: *`make results` requires manual verification first*. The correct work Never run `make results` without first verifying the test changes are correct. The `results` target copies files from `test/results/` to `test/expected/`, so running it blindly will make incorrect output become the new expected behavior. ==== verify-results safeguard -By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`: +By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`, which must be one of these two values (case-insensitive; anything else is a hard `make` error): `pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. `diffs`:: Only checks whether `test/regression.diffs` exists, so it can't create a new test's first expected output; use `pgtap` mode, or `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. @@ -774,7 +774,7 @@ Default: `$(TESTDIR)`. Directory `pg_regress` writes actual test output to -- `$ === PGXNTOOL_VERIFY_RESULTS_MODE -Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. +Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. Must be `pgtap` or `diffs` (case-insensitive) when given explicitly; any other value, including an empty one, is a hard `make` error. ==== pgtap diff --git a/README.html b/README.html index 013c61c..6cbcf04 100644 --- a/README.html +++ b/README.html @@ -449,24 +449,25 @@

PGXNtool

  • 4. make targets
  • 5. Version-Specific SQL Files @@ -618,7 +619,16 @@

    -

    4.2. test

    +

    4.2. check-duplicate-docs

    +
    +

    Warns if two $(DOCS) entries would install to the same filename, which would otherwise fail make install.

    +
    +
    +

    all depends on this, so it also runs as part of a plain make, make install and make test; you only need to invoke it directly to check without building anything.

    +
    +
    +
    +

    4.3. test

    Runs your extension’s test suite: installs the extension and runs it through PGXS’s installcheck, first pulling in anything you’ve hooked into testdeps and, if enabled, sanity-checking your test SQL via test-build.

    @@ -682,7 +692,7 @@

    -

    4.3. test-build

    +

    4.4. test-build

    Validates that extension SQL files are syntactically correct before running the full test suite. This feature runs SQL files from test/build/ through pg_regress, providing better error messages than CREATE EXTENSION failures when there are syntax errors in your extension code.

    @@ -802,7 +812,7 @@

    When CREATE EXTENSION fails, PostgreSQL shows only "syntax error" with limited context. Running the SQL directly via \i shows the exact line and position of errors, making debugging much faster.

    -

    4.3.1. results-build

    +

    4.4.1. results-build

    Refreshes test/build/expected/*.out from the actual output of the last test-build run, mirroring results for the main suite:

    @@ -825,7 +835,7 @@

    -

    4.4. test/install

    +

    4.5. test/install

    Runs setup files before the main test suite within the same pg_regress invocation. This allows expensive one-time operations (like extension installation) to set up state that persists into the regular test files.

    @@ -910,7 +920,7 @@

    -

    4.4.1. Update & Upgrade (U&U) Testing

    +

    4.5.1. Update & Upgrade (U&U) Testing

    Beyond validating a plain install, it’s worth testing that your extension behaves correctly across the two transitions every extension with more than one release eventually goes through:

    @@ -986,7 +996,7 @@

    -

    4.5. testdeps

    +

    4.6. testdeps

    This rule allows you to ensure certain actions have taken place before running tests. By default it has a single prerequisite, pgtap, which will attempt to install pgtap from PGXN. This depneds on having the pgxn client installed.

    @@ -1022,7 +1032,7 @@

    -

    4.6. results

    +

    4.7. results

    Because make test ultimately runs installcheck, it’s using the Postgres test suite. Unfortunately, that suite is based on running diff between a raw output file and expected results. I STRONGLY recommend you use pgTap instead! With pgTap, it’s MUCH easier to determine whether a test is passing or not - tests explicitly pass or fail rather than requiring you to examine diff output. The extra effort of learning pgTap will quickly pay for itself. This example might help get you started.

    @@ -1058,9 +1068,9 @@

    -

    4.6.1. verify-results safeguard

    +

    4.7.1. verify-results safeguard

    -

    By default, make results will refuse to run if your tests are failing, so you can’t accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by PGXNTOOL_VERIFY_RESULTS_MODE:

    +

    By default, make results will refuse to run if your tests are failing, so you can’t accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by PGXNTOOL_VERIFY_RESULTS_MODE, which must be one of these two values (case-insensitive; anything else is a hard make error):

    @@ -1104,7 +1114,7 @@

    -

    4.7. tag

    +

    4.8. tag

    make tag will create a git tag for the current version of your extension, as determined by the META.json file (generated from META.in.json — see PGXN Distributions vs. Extensions), and push it to origin. The reason to do this is so you can always refer to the exact code that went into a released version. For where tag, dist, and post-tag-version-bump fit in a full release, see releasing-with-pgxntool.md.

    @@ -1125,13 +1135,13 @@

    4.

    -

    4.8. tree-is-clean

    +

    4.9. tree-is-clean

    make tree-is-clean fails, printing git status, if the working tree has uncommitted or untracked changes. tag and post-tag-version-bump depend on it.

    -

    4.9. post-tag-version-bump

    +

    4.10. post-tag-version-bump

    Committing versioned SQL files (see Committing Version Files) means ongoing development after a release can silently regenerate and overwrite the file that was just released, since make always regenerates whatever file matches the current default_version. make post-tag-version-bump bumps each extension’s default_version to a placeholder alias — stable by default — so a subsequent make instead regenerates sql/{ext}--stable.sql, keeping the released file frozen.

    @@ -1140,7 +1150,7 @@

    -

    4.10. dist

    +

    4.11. dist

    make dist will create a .zip file for your current version that you can upload to PGXN. It first runs tag (creating/pushing the version’s git tag as described above if needed), then uses git archive at that tag to build the .zip file, so the archive always matches the exact tagged commit. The file is named after the PGXN name and version (the top-level "name" and "version" attributes in META.json, generated from META.in.json). The .zip file is placed in the parent directory so as not to clutter up your git repo.

    @@ -1173,7 +1183,7 @@

    -

    4.11. pgxntool-sync

    +

    4.12. pgxntool-sync

    This rule will pull down the latest released version of PGXNtool via git subtree pull and then reconcile the files setup.sh copied into your project (.gitignore, test/deps.sql) with a 3-way merge (it also verifies the test/pgxntool symlink, recreating it if missing).

    @@ -1239,7 +1249,7 @@

    -

    4.12. pgxntool-version

    +

    4.13. pgxntool-version

    make pgxntool-version prints the version of the embedded pgxntool copy, read from the first line of pgxntool/HISTORY.asc. That line is "STABLE" instead of a version number if this copy was synced from an unreleased commit rather than a tagged release.

    @@ -1257,19 +1267,19 @@

    -

    4.13. list

    +

    4.14. list

    make list prints every make target defined in your project — including ones provided by PGXS and pgxntool, not just ones you wrote — one per line. Useful for discovering what’s available without reading through the Makefile.

    -

    4.14. print-%

    +

    4.15. print-%

    make print-VARNAME prints the current value (and origin) of any make variable, e.g. make print-PGXNVERSION. Useful for debugging why a variable isn’t set to what you expect.

    -

    4.15. distclean

    +

    4.16. distclean

    make distclean removes generated configuration files (META.json — generated from META.in.json — plus meta.mk and control.mk) that survive a normal make clean.

    @@ -1295,7 +1305,7 @@

    -

    4.16. pgtle

    +

    4.17. pgtle

    Generates pg_tle (Trusted Language Extensions) registration SQL files for deploying extensions in managed environments like AWS RDS/Aurora. See [_pg_tle_Support] for complete documentation.

    @@ -1312,7 +1322,7 @@

    -

    4.17. check-pgtle

    +

    4.18. check-pgtle

    Checks if pg_tle is installed and reports the version. This target: - Reports the version from pg_extension if CREATE EXTENSION pg_tle has been run in the database @@ -1328,7 +1338,7 @@

    -

    4.18. run-pgtle

    +

    4.19. run-pgtle

    Registers all extensions with pg_tle by executing the generated pg_tle registration SQL files in a PostgreSQL database. This target: - Requires pg_tle to be installed in the target database. This isn’t a make-level dependency on check-pgtle — pgtle.sh checks it itself when it runs, and will tell you to run make check-pgtle if pg_tle isn’t there @@ -1365,7 +1375,7 @@

    -

    4.19. print-pgtle

    +

    4.20. print-pgtle

    Prints the generated pg_tle registration SQL to stdout instead of running it, for consumers building a combined multi-extension install file. This target: - Depends on pgtle, so the SQL files are (re)generated first @@ -2065,7 +2075,7 @@

    8.9. PGXNTOOL_VERIFY_RESULTS_MODE

    -

    Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing.

    +

    Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing. Must be pgtap or diffs (case-insensitive) when given explicitly; any other value, including an empty one, is a hard make error.

    8.9.1. pgtap

    @@ -2164,7 +2174,7 @@

    diff --git a/base.mk b/base.mk index 8802012..086e656 100644 --- a/base.mk +++ b/base.mk @@ -89,6 +89,15 @@ pgxntool_validate_yesno = $(strip \ $(shell echo "$(1)" | tr '[:upper:]' '[:lower:]'),\ $(error $(2) must be "yes" or "no", got "$(1)"))) +# 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) +# Returns the lowercase value, or errors if it isn't exactly one of ALLOWED_VALUES. +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)"))) + # ------------------------------------------------------------------------------ # test-build: Sanity check extension files before running full test suite # ------------------------------------------------------------------------------ @@ -288,6 +297,8 @@ _CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check-test-ins # # Variable: PGXNTOOL_VERIFY_RESULTS_MODE # - Controls how verify-results detects test failures +# - Allowed values: "pgtap" or "diffs" (case-insensitive); anything else, +# including an empty value, is a parse-time error # - "pgtap" (default): scans test/results/*.out for "not ok" lines and plan # mismatches (TAP failures). Also checks regression.diffs as a fallback. # Use this mode when your test suite uses pgTap. @@ -306,6 +317,7 @@ 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) # ------------------------------------------------------------------------------ # check-stale-expected: catch orphaned/unexpected test/expected/ files