Gate the main test suite on test-build; add build-results; require ON_ERROR_STOP in test/install

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

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

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

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/<name>.out test/build/expected/<name>.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.