diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fbdb91..aa89faf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,8 @@ jobs: timeout-minutes: 25 outputs: run-tests: ${{ steps.check.outputs.run_tests }} - test-ref: ${{ steps.check.outputs.test_ref }} + pgxntool-test-owner: ${{ steps.check.outputs.pgxntool_test_owner }} + pgxntool-test-ref: ${{ steps.check.outputs.pgxntool_test_ref }} steps: - name: Find paired pgxntool-test PR or check commit-with-no-tests label @@ -86,7 +87,8 @@ jobs: `paired-test-PR requirement and the Postgres test matrix.` ); core.setOutput('run_tests', 'false'); - core.setOutput('test_ref', ''); + core.setOutput('pgxntool_test_owner', ''); + core.setOutput('pgxntool_test_ref', ''); return; } @@ -115,7 +117,8 @@ jobs: ); } core.setOutput('run_tests', 'true'); - core.setOutput('test_ref', 'master'); + core.setOutput('pgxntool_test_owner', 'Postgres-Extensions'); + core.setOutput('pgxntool_test_ref', 'master'); return; } @@ -145,6 +148,12 @@ jobs: pr.head.repo?.owner?.login === prOwner ); if (matching.length > 1) { + // Can't tell which paired branch is the real one, but a real + // signal still matters more than none -- fall back to a test + // run against pgxntool-test/master while enforcement fails. + core.setOutput('run_tests', 'true'); + core.setOutput('pgxntool_test_owner', 'Postgres-Extensions'); + core.setOutput('pgxntool_test_ref', 'master'); core.setFailed( `Multiple open pgxntool-test PRs from ${prOwner} match branch ` + `'${branch}'. Cannot determine which one to use.\n\n` + @@ -165,6 +174,11 @@ jobs: pull_number: prNumber }); if (currentPR.labels.some(l => l.name === NO_TEST_LABEL)) { + // Contradictory state, but a paired branch to test against is + // right there -- use it for a real signal while enforcement fails. + core.setOutput('run_tests', 'true'); + core.setOutput('pgxntool_test_owner', testPR.head.repo.owner.login); + core.setOutput('pgxntool_test_ref', testPR.head.ref); core.setFailed( `PR has the '${NO_TEST_LABEL}' label, but a paired ` + `pgxntool-test PR #${testPR.number} exists on branch '${branch}'.\n\n` + @@ -215,6 +229,12 @@ jobs: const elapsed = Date.now() - waitStart; if (elapsed >= MAX_WAIT_MS) { const mins = Math.round(elapsed / 60000); + // Timed out waiting for the paired PR's own CI, but its + // branch is known -- use it for a real signal while + // enforcement fails, rather than leaving zero signal. + core.setOutput('run_tests', 'true'); + core.setOutput('pgxntool_test_owner', testPR.head.repo.owner.login); + core.setOutput('pgxntool_test_ref', testPR.head.ref); if (runs.length === 0) { core.setFailed( `pgxntool-test PR #${testPR.number} has no CI runs for ` + @@ -266,7 +286,8 @@ jobs: `SHA ${sha.slice(0, 7)} — tests run there, not here.` ); core.setOutput('run_tests', 'false'); - core.setOutput('test_ref', sha); + core.setOutput('pgxntool_test_owner', ''); + core.setOutput('pgxntool_test_ref', ''); return; } @@ -295,6 +316,13 @@ jobs: } // Neither a paired test PR nor the override label was found. + // Enforcement below is unchanged (this PR still fails the + // check), but that's not a reason to leave the maintainer with + // zero test signal while deciding whether to override it -- + // run against pgxntool-test/master anyway. + core.setOutput('run_tests', 'true'); + core.setOutput('pgxntool_test_owner', 'Postgres-Extensions'); + core.setOutput('pgxntool_test_ref', 'master'); // Fail with a clear, actionable message. core.setFailed( `No paired pgxntool-test PR found for branch '${branch}', ` + @@ -314,7 +342,11 @@ jobs: test: needs: check-test-pr - if: needs.check-test-pr.outputs.run-tests == 'true' + # !cancelled() (not bare always()): check-test-pr's own failure paths are + # exactly the cases that still need a real test run (see its script), so + # this must run despite check-test-pr's conclusion -- but an actual + # workflow cancellation should still skip it. + if: ${{ !cancelled() && needs.check-test-pr.outputs.run-tests == 'true' }} # ----------------------------------------------------------------------- # CROSS-REPO REUSABLE WORKFLOW — READ BEFORE CHANGING THIS REF # See: .github/workflows/CLAUDE.md for full architecture notes. @@ -330,7 +362,8 @@ jobs: # pgxntool: this PR's own branch, on its own account (a fork for fork PRs). pgxntool-owner: ${{ github.event.pull_request.head.repo.owner.login }} pgxntool-branch: ${{ github.event.pull_request.head.ref }} - # pgxntool-test: no paired test PR in this path, so use canonical master - # from Postgres-Extensions only (never a fork's master). - pgxntool-test-owner: Postgres-Extensions - pgxntool-test-ref: master + # pgxntool-test: whichever owner/ref check-test-pr decided gives a real + # signal for this path (the paired PR's own branch when one exists and + # is usable, canonical master otherwise -- see check-test-pr's script). + pgxntool-test-owner: ${{ needs.check-test-pr.outputs.pgxntool-test-owner }} + pgxntool-test-ref: ${{ needs.check-test-pr.outputs.pgxntool-test-ref }} diff --git a/CLAUDE.md b/CLAUDE.md index 17ca775..30bb6d9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -181,9 +181,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 @@ -200,6 +200,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 986cdfe..32129b6 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -27,7 +27,25 @@ 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. -Issues fixed in this release: #87, #21 +== 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: #87, #21, #97, #108 2.3.0 ----- diff --git a/README.asc b/README.asc index 63b2ffb..2d13e5d 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: @@ -741,6 +767,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 7329e01..fc709d5 100644 --- a/README.html +++ b/README.html @@ -509,13 +509,15 @@

PGXNtool

  • 8.8. PGXNTOOL_VERIFY_RESULTS_MODE
  • 8.9. PGXNTOOL_ENABLE_TEST_BUILD *
  • 8.10. PGXNTOOL_ENABLE_TEST_INSTALL *
    • -
  • 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
    • +
  • 8.11. PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK *
    • +
  • 8.12. PGXNTOOL_ENABLE_VERIFY_RESULTS *
    • +
  • 8.13. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *
    • +
  • 8.14. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *
    • +
  • 8.15. PGXNTOOL_NO_PGXS_INCLUDE
    • -
  • 9. Copyright
    • +
  • 9. General PostgreSQL Notes
    • +
  • 10. Copyright
    • @@ -616,6 +618,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.

    @@ -675,10 +693,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
    +    		 +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:

    @@ -763,6 +793,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

    @@ -778,9 +830,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

  • @@ -795,13 +844,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:

    @@ -837,6 +886,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

    @@ -1997,13 +2058,19 @@

    -

    8.11. PGXNTOOL_ENABLE_VERIFY_RESULTS *

    +

    8.11. 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.12. 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.12. PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED *

    +

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

    @@ -2012,13 +2079,13 @@

    -

    8.13. PGXNTOOL_CHECK_EXPECTED_FILE_TYPES *

    +

    8.14. 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.14. PGXNTOOL_NO_PGXS_INCLUDE

    +

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

    @@ -2026,7 +2093,18 @@

    -

    9. Copyright

    +

    9. General PostgreSQL Notes

    +
    +
    +

    POSTGRES-NOTES.asc collects guidance about PostgreSQL itself +rather than about PGXNtool — things that come up constantly while writing an extension +but that PGXNtool has no part in. It currently covers which .control file parameters +(superuser, trusted) control who is allowed to install your extension.

    +
    +
    +
    +
    +

    10. Copyright

    Copyright (c) 2026 Jim Nasby <Jim.Nasby@gmail.com>

    @@ -2039,7 +2117,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 93fac4e..c9fe747 100644 --- a/base.mk +++ b/base.mk @@ -129,6 +129,33 @@ endif # state created by install files (tables, extensions, etc.) persists # into the main test suite. # +# IMPORTANT: 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 forces the build to fail 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. 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. +# +# Why not just force `-v ON_ERROR_STOP=1` onto the psql invocation instead of +# checking each file? Because test/install and test/sql run inside the same +# pg_regress invocation -- forcing it there would also apply to test/sql, +# breaking any test file that deliberately triggers an error mid-file and +# keeps going to check what happens next, a normal pg_regress pattern. +# That's too big an API change to make silently; the per-file opt-in keeps +# test/sql semantics untouched. +# +# 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 +166,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 +195,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 +407,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. # @@ -538,12 +603,70 @@ 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. +# +# Runs test-build itself (leading `-` ignores its exit status) instead of +# duplicating its run-test-build.sh + installcheck steps: by the time +# test-build's own regression.diffs check fails, the actual output +# build-results needs is already on disk -- the failure just means "there's +# a diff", which is exactly what build-results exists to bless. +.PHONY: build-results +build-results: + -$(MAKE) -C . test-build + @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