From 4916368a8fb8477616f80d26549b0617cced7395 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 20 Aug 2026 18:28:03 -0500 Subject: [PATCH] Gate the main test suite on test-build; add build-results; require ON_ERROR_STOP in test/install test-build had no real dependency edge on installcheck, so the main suite (test/install + test/sql) actually ran before test-build's own sanity check in the default config, and raced with it under `make -j`. Add an explicit `installcheck: test-build` edge (guarded to avoid recursing into test-build's own nested installcheck invocation) so a broken build blocks the main suite entirely -- its results would be meaningless otherwise. Add `make build-results` to refresh test/build/expected/*.out from the last test-build run, mirroring `make results` for the main suite. Refuses to bless any file whose actual output contains an ERROR: line. test/install/*.sql files never get a real diff -- their actual output overwrites their expected output in place, so a content difference can never fail the build. Add check-test-install-error-stop, enforcing that every test/install/*.sql file sets ON_ERROR_STOP (directly or via test/pgxntool/psql.sql), since that's the only thing that still turns a hard SQL error into a build failure. test/install/*.out is now gitignored since it's rewritten by every run and never meaningfully compared. Fixes #108. Fixes #97. Related changes in pgxntool-test: - Template's test/install file now sets ON_ERROR_STOP; its .out is no longer committed - Test coverage for the new ordering edge, build-results, and check-test-install-error-stop Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 9 +- HISTORY.asc | 31 ++++++ README.asc | 42 ++++++-- README.html | 110 +++++++++++++++++--- _.gitignore | 5 + base.mk | 117 +++++++++++++++++++++- test/bin/check-test-install-error-stop.sh | 56 +++++++++++ 7 files changed, 344 insertions(+), 26 deletions(-) create mode 100755 test/bin/check-test-install-error-stop.sh diff --git a/CLAUDE.md b/CLAUDE.md index 65e3bf7..dad169f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -171,9 +171,9 @@ Note: `make test` intentionally does *not* depend on `clean` — depending on `c **Database Connection Requirement**: PostgreSQL must be running before executing `make test`. If you get connection errors (e.g., "could not connect to server"), stop and ask the user to start PostgreSQL. -**Claude Code MUST NEVER run `make results`**. This target updates test expected output files and requires manual human verification of test changes before execution. +**Claude Code MUST NEVER run `make results` or `make build-results`**. Both update test expected output files and require manual human verification of test changes before execution. -**Claude Code MUST NEVER modify files in `test/expected/`**. These are expected test outputs that define correct behavior and must only be updated through the `make results` workflow. +**Claude Code MUST NEVER modify files in `test/expected/` or `test/build/expected/`**. These are expected test outputs that define correct behavior and must only be updated through the `make results`/`make build-results` workflows. The workflow is: 1. Human runs `make test` and examines diffs @@ -190,6 +190,11 @@ pgxntool uses PostgreSQL's pg_regress test framework: When tests fail, examine the diff output carefully. The actual test output in `test/results/` shows what your code produced, while `test/expected/` shows what was expected. +**Exceptions to the above** -- `test-build` and `test/install` (both optional, see `README.asc`) don't follow the `test/results` vs `test/expected` model: + +- **test-build** runs first, in its own separate `pg_regress` pass over `test/build/*.sql`, and gates the main suite: if it fails, `test/install`/`test/sql` never run at all. It does compare actual vs expected normally (`test/build/results/` vs `test/build/expected/`) -- use `make build-results` to refresh its expected output, not `make results`. +- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output, so a content difference can never fail the build, no matter what changed. The only thing that still fails the build is a hard SQL error, and only if the file has `ON_ERROR_STOP` set (directly or via `\i test/pgxntool/psql.sql`) -- pgxntool checks for this by default. If a `test/install/*.sql` file is misbehaving, don't go looking for a diff; check whether it errored, and don't assume a stale-looking `.out` for it means anything. + ## Key Implementation Details ### PostgreSQL Version Handling diff --git a/HISTORY.asc b/HISTORY.asc index bdd5121..92c7dec 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,3 +1,34 @@ +STABLE +------ +== `test-build` now gates the main test suite +Previously, `test-build` had no real dependency edge on `installcheck` -- +`check-stale-expected`'s own edge onto `installcheck` happened to pull the +main suite in first under serial `make`, and the two raced under `make -j`. +`test-build` now genuinely runs (and must pass) before `test/install`/ +`test/sql` do: there's no point validating install/query behavior against a +build that doesn't even come up cleanly, and the dependency edge holds under +parallel `make` too. + +== Add `make build-results` +Refreshes `test/build/expected/*.out` from the last `test-build` run's +actual output, mirroring `make results` for the main suite. Refuses to +bless any file whose actual output contains an `ERROR:` line, since that +would defeat the point of test-build; skip such a file and bless it by hand +if the error is intentional. + +== `test/install` now requires `ON_ERROR_STOP` +`test/install/*.sql` files never get a real diff -- their actual output is +written to the same file as their expected output, so a content difference +can never fail the build. Without `ON_ERROR_STOP`, a hard SQL error was +silently swallowed too, making the file "pass" regardless of what happened. +`make test` now fails if a `test/install/*.sql` file doesn't set +`ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`); see +`PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK` to disable. Since +`test/install/*.out` was never really compared against anything, and is +rewritten by every run, it's now gitignored -- stop committing it. + +Issues fixed in this release: #97, #108 + 2.3.0 ----- == Rename `PGTLE_VERSION` to `PGXNTOOL_PGTLE_VERSION` diff --git a/README.asc b/README.asc index 45c4b59..92ffc5b 100644 --- a/README.asc +++ b/README.asc @@ -47,6 +47,13 @@ This will build any .html files that can be created. See <<_Document_Handling>>. === 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>>. +`make test` runs `pg_regress` in up to two separate passes: + +1. If <<_test_build,test-build>> is enabled, it runs first, in its own isolated pass over `test/build/*.sql`. If it fails, the second pass never happens — there's no point checking install/query behavior against a build that doesn't even come up cleanly. +2. The main pass follows: any <<_testinstall,test/install>> files, then your regular `test/sql/*.sql` files, together in a single `pg_regress` invocation (so state the install files create persists into the regular tests). + +Within each pass, files run in alphabetical order by filename. + Whether `test-build` runs is controlled by the `PGXNTOOL_ENABLE_TEST_BUILD` variable — see <<_test_build>> for what it does and how to turn it on/off. NOTE: `test` intentionally does *not* depend on `clean` — that caused problems with incremental/watch-based builds. If your tests need a clean build to pass, that's a sign of a missing dependency elsewhere rather than something to fix by adding `clean` back. @@ -63,7 +70,9 @@ Validates that extension SQL files are syntactically correct before running the 1. Place SQL files in `test/build/*.sql` 2. Place expected output in `test/build/expected/*.out` 3. These files run through `pg_regress` before `make test` runs the main test suite -4. If any build test fails, the test run stops immediately with clear error messages +4. If any build test fails, `make test` stops immediately — the main suite (test/install + test/sql) never runs, since its results would be meaningless against a broken build + +NOTE: This also means a stale or simply wrong `test/build/expected/*.out` -- not just a genuinely broken build -- blocks the whole suite. See <<_build_results,build-results>> below for the supported way to refresh it, including how to handle a file that's *supposed* to show an error. **Directory structure:** @@ -128,27 +137,42 @@ This approach catches SQL syntax errors *before* running `CREATE EXTENSION`, giv 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. +==== build-results + +Refreshes `test/build/expected/*.out` from the actual output of the last `test-build` run, mirroring <<_results,results>> for the main suite: + +---- +make build-results +---- + +`build-results` will *not* bless a file whose actual output contains an `ERROR:` line — accepting an errored build as the new expected baseline would defeat the point of test-build. It skips that file (leaving its existing expected output in place), tells you exactly which file and the command to bless it with, and exits non-zero so the skip can't go unnoticed. + +If your project intentionally exercises a build-time error (e.g. asserting a migration fails the way it should), bless that file by hand instead — this is a deliberate, supported case, not a workaround, and you'll need to repeat it by hand every time that file's output legitimately changes, since `build-results` will always skip it: + +---- +cp test/build/results/.out test/build/expected/.out +---- + === 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. **How it works:** 1. Place SQL files in `test/install/*.sql` -2. Place expected output alongside as `test/install/*.out` -3. A schedule file is auto-generated that lists install files with `../install/` relative paths -4. `pg_regress` processes the install schedule first, then runs regular test files — all in one invocation, so database state persists +2. A schedule file is auto-generated that lists install files with `../install/` relative paths +3. `pg_regress` processes the install schedule first, then runs regular test files — all in one invocation, so database state persists **Directory structure:** ---- test/install/ ├── *.sql # SQL setup files (checked in) -├── *.out # Expected output (checked in, alongside .sql) +├── *.out # GENERATED, gitignored -- see WARNING below ├── .gitignore # Ignores pg_regress artifacts (*.out.diff) └── schedule # GENERATED - auto-created by make ---- -The `schedule` file is generated automatically and listed in `.gitignore`. Do not edit it. +The `schedule` file is generated automatically and listed in `.gitignore`. Do not edit it. `*.out` is gitignored too, for the reason explained below — don't commit it. **Configuration:** @@ -169,6 +193,8 @@ 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, `test/install`'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 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: @@ -718,6 +744,10 @@ Default: auto-detected -- `yes` if `test/build/*.sql` files exist, `no` otherwis Default: auto-detected -- `yes` if `test/install/*.sql` files exist, `no` otherwise. Enables or disables the <<_testinstall,test/install>> schedule-based setup feature. Same explicit-override semantics as `PGXNTOOL_ENABLE_TEST_BUILD`. +=== PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK * + +Default: `yes`. Enables or disables a build-time check that every `test/install/*.sql` file sets `ON_ERROR_STOP` (directly, or via `\i test/pgxntool/psql.sql`) -- see <<_testinstall,test/install>> for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to `no` to disable the check. + === PGXNTOOL_ENABLE_VERIFY_RESULTS * Default: `yes`. Enables or disables the <<_verify_results_safeguard,verify-results safeguard>> that blocks `make results` when tests are failing. Setting it to empty on the command line (`make PGXNTOOL_ENABLE_VERIFY_RESULTS= results`) also disables it. diff --git a/README.html b/README.html index ba71c2a..e411a51 100644 --- a/README.html +++ b/README.html @@ -507,10 +507,11 @@

PGXNtool

  • 8.7. PGXNTOOL_VERIFY_RESULTS_MODE
  • 8.8. PGXNTOOL_ENABLE_TEST_BUILD *
  • 8.9. PGXNTOOL_ENABLE_TEST_INSTALL *
    • -
  • 8.10. PGXNTOOL_ENABLE_VERIFY_RESULTS *
    • -
  • 8.11. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *
    • -
  • 8.12. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *
    • -
  • 8.13. PGXNTOOL_NO_PGXS_INCLUDE
    • +
  • 8.10. PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *
    • +
  • 8.11. PGXNTOOL_ENABLE_VERIFY_RESULTS *
    • +
  • 8.12. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *
    • +
  • 8.13. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *
    • +
  • 8.14. PGXNTOOL_NO_PGXS_INCLUDE
  • 9. Copyright
    • @@ -614,6 +615,22 @@

    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.

    +

    make test runs pg_regress in up to two separate passes:

    +
    +
    +
      +
    1. +

      If test-build is enabled, it runs first, in its own isolated pass over test/build/*.sql. If it fails, the second pass never happens — there’s no point checking install/query behavior against a build that doesn’t even come up cleanly.

      +
      2. +
    2. +

      The main pass follows: any test/install files, then your regular test/sql/*.sql files, together in a single pg_regress invocation (so state the install files create persists into the regular tests).

      +
      3. +
    +
    +
    +

    Within each pass, files run in alphabetical order by filename.

    +
    +

    Whether test-build runs is controlled by the PGXNTOOL_ENABLE_TEST_BUILD variable — see test-build for what it does and how to turn it on/off.

    @@ -635,6 +652,18 @@

    Note

    +test exits non-zero (after printing regression.diffs) if any test fails. Previously it always exited 0 regardless of test results, silently masking failures from CI and other automation that relies on the exit code. + + + + +
    + + + + @@ -661,10 +690,22 @@

    These files run through pg_regress before make test runs the main test suite

  • -

    If any build test fails, the test run stops immediately with clear error messages

    +

    If any build test fails, make test stops immediately — the main suite (test/install + test/sql) never runs, since its results would be meaningless against a broken build

    • +
    +

    +
    Note
    +    		 While you can still run make installcheck or any other valid PGXS make target directly, it’s recommended to use make test when using pgxntool. The test target ensures proper test isolation and correct dependency installation.
    + + + + +
    +
    Note
    +    		 +This also means a stale or simply wrong test/build/expected/.out — not just a genuinely broken build — blocks the whole suite. See build-results below for the supported way to refresh it, including how to handle a file that’s *supposed to show an error. +
    +

    Directory structure:

    @@ -749,6 +790,28 @@

    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. build-results

    +
    +

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

    +
    +
    +
    +
    make build-results
    +
    +
    +
    +

    build-results will not bless a file whose actual output contains an ERROR: line — accepting an errored build as the new expected baseline would defeat the point of test-build. It skips that file (leaving its existing expected output in place), tells you exactly which file and the command to bless it with, and exits non-zero so the skip can’t go unnoticed.

    +
    +
    +

    If your project intentionally exercises a build-time error (e.g. asserting a migration fails the way it should), bless that file by hand instead — this is a deliberate, supported case, not a workaround, and you’ll need to repeat it by hand every time that file’s output legitimately changes, since build-results will always skip it:

    +
    +
    +
    +
    cp test/build/results/<name>.out test/build/expected/<name>.out
    +
    +
    +

    4.4. test/install

    @@ -764,9 +827,6 @@

    Place SQL files in test/install/*.sql

  • -

    Place expected output alongside as test/install/*.out

    -
    • -

  • A schedule file is auto-generated that lists install files with ../install/ relative paths

  • @@ -781,13 +841,13 @@ 

    test/install/
 ├── *.sql              # SQL setup files (checked in)
-├── *.out              # Expected output (checked in, alongside .sql)
+├── *.out              # GENERATED, gitignored -- see WARNING below
 ├── .gitignore         # Ignores pg_regress artifacts (*.out.diff)
 └── schedule           # GENERATED - auto-created by make

    • -

    The schedule file is generated automatically and listed in .gitignore. Do not edit it.

    +

    The schedule file is generated automatically and listed in .gitignore. Do not edit it. *.out is gitignored too, for the reason explained below — don’t commit it.

    Configuration:

    @@ -823,6 +883,18 @@

    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, test/install’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). +
    +

    4.4.1. Update & Upgrade (U&U) Testing

    @@ -1071,7 +1143,7 @@

    4.9. 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.

    +

    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).

    @@ -1941,13 +2013,19 @@

    -

    8.10. PGXNTOOL_ENABLE_VERIFY_RESULTS *

    +

    8.10. PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *

    +
    +

    Default: yes. Enables or disables a build-time check that every test/install/*.sql file sets ON_ERROR_STOP (directly, or via \i test/pgxntool/psql.sql) — see test/install for why this is the only thing that makes a hard error in one of those files actually fail the build. Set to no to disable the check.

    +
    + +
    +

    8.11. PGXNTOOL_ENABLE_VERIFY_RESULTS *

    Default: yes. Enables or disables the verify-results safeguard that blocks make results when tests are failing. Setting it to empty on the command line (make PGXNTOOL_ENABLE_VERIFY_RESULTS= results) also disables it.

    -

    8.11. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *

    +

    8.12. 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).

    @@ -1956,13 +2034,13 @@

    -

    8.12. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *

    +

    8.13. 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.

    -

    8.13. PGXNTOOL_NO_PGXS_INCLUDE

    +

    8.14. PGXNTOOL_NO_PGXS_INCLUDE

    Default: unset (PGXS is included normally). Skips including PGXS ($(PGXS)) entirely. This is only for advanced scenarios where you need to manage the PGXS include yourself; most projects should never set this.

    @@ -1983,7 +2061,7 @@

    diff --git a/_.gitignore b/_.gitignore index 1873c2c..4d25197 100644 --- a/_.gitignore +++ b/_.gitignore @@ -32,6 +32,11 @@ test/build/sql/ # Created by make when test/install/*.sql files exist. test/install/schedule +# test/install/*.out is rewritten by every `make test` run and never +# meaningfully compared against anything (see base.mk's test/install +# comments) -- there's no reason to commit it or track changes to it. +test/install/*.out + # Misc tmp/ .DS_Store diff --git a/base.mk b/base.mk index 00dab50..81f9740 100644 --- a/base.mk +++ b/base.mk @@ -129,6 +129,28 @@ endif # state created by install files (tables, extensions, etc.) persists # into the main test suite. # +# IMPORTANT (for humans and AI agents reading this file): test/install does +# NOT get a real pg_regress diff, unlike every other test type pgxntool +# supports. Its schedule entries reference the original file via a relative +# "../install/" path, so pg_regress resolves BOTH the expected output +# and the actual output to the exact same file (test/install/.out) -- +# the actual run silently overwrites the expected file in place, rather than +# ever comparing it against anything. A content difference (wrong output, a +# changed column, whatever) will NEVER fail this way, no matter how it +# changes -- see issue #97. +# +# The only thing that still fails the build is psql's own exit code: a +# statement that raises a hard error only aborts psql (non-zero exit, which +# pg_regress does report as a failure) if ON_ERROR_STOP is set. Without it, +# psql prints the error, keeps going, and exits 0 -- the file "passes" +# regardless of what happened. 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 default; see +# PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK to disable it. +# +# This is intentional, documented behavior -- not a bug to be fixed quietly. +# # Variable: PGXNTOOL_ENABLE_TEST_INSTALL # - Can be set manually in Makefile or command line # - Allowed values: "yes" or "no" (case-insensitive) @@ -139,7 +161,10 @@ endif # # Directory layout (follows ~/code/extensions/archive/ pattern): # test/install/*.sql - Install SQL files -# test/install/*.out - Expected output (lives alongside .sql files) +# test/install/*.out - GENERATED, gitignored: rewritten by every run, +# lives alongside .sql files but (per the +# IMPORTANT note above) isn't meaningfully +# compared, so there's nothing to commit # test/install/schedule - Auto-generated schedule file # test/sql/schedule - Auto-generated schedule file for regular tests # @@ -165,6 +190,22 @@ else endif endif +# Variable: PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK +# - Gates check-test-install-error-stop (see TEST_DEPS wiring below): fails +# the build if any test/install/*.sql file doesn't set ON_ERROR_STOP, +# directly or via `\i test/pgxntool/psql.sql` -- see the IMPORTANT note +# above for why this is the only thing standing between a hard SQL error +# and a silent "pass". +# - Allowed values: "yes" or "no" (case-insensitive) +# - Default: "yes" +ifdef PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK + override PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK),PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK) +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 + # ------------------------------------------------------------------------------ # verify-results: Safeguard for make results # ------------------------------------------------------------------------------ @@ -361,6 +402,25 @@ check-stale-expected: installcheck TEST_DEPS += check-stale-expected endif +# ------------------------------------------------------------------------------ +# 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. This is +# a pure static scan of file 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 +endif +endif + # make test: run any test dependencies, then do a `make install installcheck`. # If regressions are found, it will output them. # @@ -502,12 +562,65 @@ TEST_BUILD_REGRESS = $(sort $(notdir $(basename $(TEST_BUILD_SQL_FILES)))) .PHONY: test-build test-build: install @$(PGXNTOOL_DIR)/run-test-build.sh $(TESTDIR) - $(MAKE) -C . REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck + $(MAKE) -C . _PGXNTOOL_TEST_BUILD_ACTIVE=yes REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck @if [ -r $(TESTDIR)/build/regression.diffs ]; then \ echo "test-build failed - see $(TESTDIR)/build/regression.diffs"; \ cat $(TESTDIR)/build/regression.diffs; \ exit 1; \ fi + +# There's no point running test/install or test/sql against a build that +# doesn't even come up cleanly -- their results would be meaningless, so +# test-build must run (and pass) before the main suite starts. +# +# Tradeoff: this also blocks the main suite on a stale/wrong +# test/build/expected/*.out, not just a genuinely broken build -- there's +# no `make results`-equivalent for test/build, so today that means either +# hand-editing the expected file or using `make build-results` below. +# +# Guarded by _PGXNTOOL_TEST_BUILD_ACTIVE: test-build's own recipe above +# recurses into `installcheck` to reuse PGXS's pg_regress plumbing for its +# separate run over test/build/*.sql. Without the guard, that nested +# installcheck would itself depend on test-build, recursing forever. +ifneq ($(_PGXNTOOL_TEST_BUILD_ACTIVE),yes) +installcheck: test-build +endif + +# build-results: bless test/build/'s actual output as the new expected +# output, mirroring `make results` for the main suite. Refuses to bless any +# file whose actual output contains "ERROR:" -- accepting an errored build +# as the new baseline would defeat the point of test-build. If a project +# intentionally exercises an error case in test/build (e.g. verifying a +# migration fails as expected), bless that file by hand instead: +# cp $(TESTDIR)/build/results/.out $(TESTDIR)/build/expected/.out +# +# This ERROR-scanning safeguard is only possible because test-build writes +# actual output to a location genuinely separate from its expected output +# (--inputdir/--outputdir both point at test/build, but pg_regress keeps +# expected/ and results/ distinct there) -- test/install intentionally does +# NOT work this way (see test/install's own comments above): its actual +# output lands on top of its expected file, so there's nothing to diff. We +# don't generally care about test-build's own output content, but when a +# build genuinely errors, having a real .diff is worth the extra plumbing -- +# it points straight at the problem instead of leaving you to comb through +# unrelated output for the one line that matters. +.PHONY: build-results +build-results: install + @$(PGXNTOOL_DIR)/run-test-build.sh $(TESTDIR) + $(MAKE) -C . _PGXNTOOL_TEST_BUILD_ACTIVE=yes REGRESS="$(TEST_BUILD_REGRESS)" REGRESS_OPTS="--inputdir=$(TESTDIR)/build --outputdir=$(TESTDIR)/build" installcheck + @mkdir -p $(TESTDIR)/build/expected + @skipped=0; \ + for f in $(TESTDIR)/build/results/*.out; do \ + [ -f "$$f" ] || continue; \ + if grep -q 'ERROR:' "$$f"; then \ + echo "build-results: skipping $$f (actual output contains ERROR:)" >&2; \ + echo " If this is intentional, bless it by hand:" >&2; \ + echo " cp $$f $(TESTDIR)/build/expected/$$(basename "$$f")" >&2; \ + skipped=1; continue; \ + fi; \ + cp "$$f" $(TESTDIR)/build/expected/$$(basename "$$f"); \ + done; \ + [ "$$skipped" = 0 ] || exit 1 endif diff --git a/test/bin/check-test-install-error-stop.sh b/test/bin/check-test-install-error-stop.sh new file mode 100755 index 0000000..26c98e5 --- /dev/null +++ b/test/bin/check-test-install-error-stop.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# +# check-test-install-error-stop.sh - Ensure test/install/*.sql files set +# ON_ERROR_STOP +# +# test/install/*.sql files run in pg_regress's own self-comparing entry (see +# the test/install comments in base.mk): actual output lands on top of the +# expected file, so there's no real diff to catch a bad result. The only +# thing that still fails the build is psql itself exiting non-zero -- which +# only happens if ON_ERROR_STOP is set. Without it, a hard SQL error is +# printed and swallowed, and the file "passes". This scans for that safety +# net so its absence is caught at build time instead of discovered the hard +# way (issue #97). +# +# A file passes if it either sets ON_ERROR_STOP itself, or sources +# test/pgxntool/psql.sql (which already sets it, among other things). +# +# Usage: check-test-install-error-stop.sh + +set -o errexit -o errtrace -o pipefail + +BASEDIR=$(dirname "$0") +source "$BASEDIR/../../lib.sh" + +if [ $# -ne 1 ]; then + die 1 "Usage: check-test-install-error-stop.sh " +fi + +testdir="$1" +install_dir="$testdir/install" +missing=() + +for f in "$install_dir"/*.sql; do + [ -f "$f" ] || continue + + if grep -q 'ON_ERROR_STOP' "$f"; then + continue + fi + if grep -qE '\\ir? +.*psql\.sql' "$f"; then + continue + fi + + missing+=("$f") +done + +if [ "${#missing[@]}" -gt 0 ]; then + error "the following test/install/*.sql files don't set ON_ERROR_STOP:" + printf ' %s\n' "${missing[@]}" >&2 + error "test/install files run in their own self-comparing pg_regress entry" \ + "(see the test/install comments in base.mk) -- without ON_ERROR_STOP, a" \ + "hard SQL error is silently swallowed instead of failing the build." + die 1 "Add '\\set ON_ERROR_STOP on' near the top of the file, or" \ + "'\\i test/pgxntool/psql.sql' (which already sets it)." +fi + +# vi: expandtab ts=2 sw=2