From a27cff5e6a1183767107329c4f43a8a0bc3f6f7b Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 17:57:28 -0500 Subject: [PATCH 1/2] Fix installcheck running before install; make test now fails on regressions installcheck now has an explicit `install` prerequisite. check-stale-expected's `check-stale-expected: installcheck` edge could previously pull installcheck in as a prerequisite before `test`'s own `install installcheck` pair ran, since Make doesn't guarantee left-to-right order between unrelated prerequisites of the same target. On a fresh/uninstalled tree (e.g. CI), this ran the full pg_regress suite against a database missing the extension, failing every test that touches it. Fixes #79. `make test`'s recipe now exits non-zero when `regression.diffs` is non-empty, still printing the diff first. `.IGNORE: installcheck` made installcheck's real pg_regress exit status irrelevant to Make, and `test`'s recipe never checked `regression.diffs` itself -- so `make test` always exited 0, even when every single test failed, silently defeating CI. Fixes #49. `results`/`verify-results` now depend on `$(TEST_DEPS)` directly instead of on `test`, since `test`'s new exit-1-on-diff would otherwise abort the results workflow before verify-results ever got to inspect and report the diff itself (the whole point of `make results` is to bless output that differs from the old expected files). Related changes in pgxntool-test: - New `make test` coverage from a genuinely uninstalled state (issue #79) - `make test`/`make results`/`verify-results` coverage updated to assert the new exit statuses (issue #49) - Fixed a latent expected-output mismatch in two test fixtures that `make test` never caught before, because it never checked its own exit status Co-Authored-By: Claude Sonnet 5 --- HISTORY.asc | 8 ++++++++ base.mk | 34 +++++++++++++++++++++++++++------- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/HISTORY.asc b/HISTORY.asc index 231809f..ea783b2 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,3 +1,11 @@ +STABLE +------ +== Fix `make test` always exiting 0, even when every test fails +`.IGNORE: installcheck` made `make` treat `installcheck` as always successful +regardless of `pg_regress`'s exit status, and `test`'s recipe only `cat`ed +`regression.diffs` for visibility without checking it. `make test` now exits +non-zero when `regression.diffs` is non-empty, after printing it. + 2.2.0 ----- == Add `check-stale-expected` to catch orphaned test/expected files diff --git a/base.mk b/base.mk index e77cff2..73c1551 100644 --- a/base.mk +++ b/base.mk @@ -307,6 +307,17 @@ DATA += $(wildcard *.control) .IGNORE: installcheck installcheck: $(TEST_RESULT_FILES) $(TEST_SQL_FILES) | $(TESTDIR)/sql/ $(TESTDIR)/expected/ $(TESTOUT)/results/ +# installcheck must run after install: pg_regress needs the extension already +# installed (CREATE EXTENSION requires the control/SQL files to be in place). +# PGXS's own installcheck target doesn't declare that dependency -- it assumes +# the caller runs `make install installcheck` manually. That assumption breaks +# when something else (e.g. check-stale-expected below) depends on installcheck +# directly: `test`'s TEST_DEPS lists install/installcheck as independent, +# unordered prerequisites, so nothing stops installcheck's own prerequisite +# chain from running before install. An explicit edge here, same as +# check-stale-expected's, is the only ordering guarantee Make actually gives. +installcheck: install + # # TEST SUPPORT # @@ -355,7 +366,7 @@ TEST_DEPS += test-build endif TEST_DEPS += install installcheck test: $(TEST_DEPS) - @if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; fi + @if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; exit 1; fi # # verify-results: Safeguard for make results @@ -379,18 +390,27 @@ verify-results: endif endif -# make results: runs `make test` and copies all result files to expected. +# make results: runs the same steps as `make test` and copies all result files +# to expected. # DO NOT RUN THIS UNLESS YOU'RE CERTAIN ALL YOUR TESTS ARE PASSING! # -# Dependency chain (verify-results: test) guarantees test completes before verify-results -# checks regression.diffs, even under make -j. Listing both as independent prerequisites -# of results would allow them to run concurrently, letting verify-results see stale state. +# Depends on $(TEST_DEPS) directly rather than on `test` itself: `test`'s own +# recipe now exits non-zero as soon as it sees a regression.diffs mismatch +# (see `test:` above), which would abort this chain before verify-results ever +# got a chance to inspect the diff and report it properly. verify-results (or +# the plain diffs check below, when disabled) is the one that's supposed to +# decide pass/fail here, not `test`. +# +# Dependency chain (verify-results: $(TEST_DEPS)) guarantees those complete +# before verify-results checks regression.diffs, even under make -j. Listing +# them as independent prerequisites of results would allow them to run +# concurrently, letting verify-results see stale state. .PHONY: results ifeq ($(PGXNTOOL_ENABLE_VERIFY_RESULTS),yes) -verify-results: test +verify-results: $(TEST_DEPS) results: verify-results else -results: test +results: $(TEST_DEPS) endif @mkdir -p $(TESTDIR)/expected @for f in $(TESTOUT)/results/*.out; do \ From 5c2da690b292eb1a7e4ff5049f80fbc581a9c8f9 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 31 Jul 2026 15:15:31 -0500 Subject: [PATCH 2/2] HISTORY.asc: add fixed-issues line for #35, #49, #79 Per the newly-adopted convention (issue number always goes on this line when a commit's Fixes/Closes/Resolves closes it, regardless of whether the change is significant enough for its own narrative entry), even though #79's own bullet was dropped as too minor to narrate. Co-Authored-By: Claude Sonnet 5 --- HISTORY.asc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/HISTORY.asc b/HISTORY.asc index ea783b2..11c9b58 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -6,6 +6,8 @@ regardless of `pg_regress`'s exit status, and `test`'s recipe only `cat`ed `regression.diffs` for visibility without checking it. `make test` now exits non-zero when `regression.diffs` is non-empty, after printing it. +Issues fixed in this release: #35, #49, #79 + 2.2.0 ----- == Add `check-stale-expected` to catch orphaned test/expected files