diff --git a/CLAUDE.md b/CLAUDE.md index cca492c..5cb06a5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -179,7 +179,7 @@ make pgxntool-sync # Update to latest pgxntool via git subtree pull - Correct test dependency installation (`testdeps`, and `test-build` when enabled); `testdeps`' default `pgtap` prerequisite auto-installs pgtap via `pgxn install`, a no-op when `PGXNTOOL_ENABLE_PGXN_INSTALL=no` - Extension is filesystem-installed before tests run (`install`), unless `PGXNTOOL_ENABLE_FS_INSTALL=no` - Test comparison via `installcheck`, with diffs shown on failure -- Safeguard checks that fail the run: the stale-expected-file check (orphaned or non-`.out` files in `test/expected/`, run after `pg_regress`), and, when `test/install` is in use, the check that every `test/install/*.sql` file sets `ON_ERROR_STOP` +- Safeguard checks that fail the run: the pgTap output check (a `not ok` without `# TODO`, a missing or duplicate `1..N` plan, or a plan/test count mismatch in `test/results/*.out` or the `test/install` actual output -- even when the output matches its expected file; skipped when `PGXNTOOL_VERIFY_RESULTS_MODE=diffs`), the stale-expected-file check (orphaned or non-`.out` files in `test/expected/`, run after `pg_regress`), and, when `test/install` is in use, the check that every `test/install/*.sql` file sets `ON_ERROR_STOP` Note: `make test` intentionally does *not* depend on `clean` — depending on `clean` caused problems with incremental/watch-based builds (see `base.mk`). If your tests need a clean build to pass, that's a sign of a missing dependency elsewhere, not something to fix by adding `clean` back. @@ -207,7 +207,7 @@ When tests fail, examine the diff output carefully. The actual test output in `t **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 results-build` 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, passing any file that includes `psql.sql` or has its own `\set`/`\unset ON_ERROR_STOP`, whatever the value. 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. +- **test/install** does NOT get a real diff at all: its actual output is written to the exact same file as its expected output (in `test/install/`, even when `TESTOUT` is set: `$(TESTOUT)/install` is then a symlink to it), so a content difference can never fail the build, no matter what changed. Apart from pgTap failures (caught by the pgTap output check above), 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, passing any file that includes `psql.sql` or has its own `\set`/`\unset ON_ERROR_STOP`, whatever the value. 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 diff --git a/HISTORY.asc b/HISTORY.asc index 3d588c0..8b7977f 100644 --- a/HISTORY.asc +++ b/HISTORY.asc @@ -1,3 +1,40 @@ +STABLE +------ +== Breaking changes + +=== `make test` now fails on pgTap failures in actual output +`make test` used to fail only on `regression.diffs`, so a `not ok` line +blessed into an expected file passed. It now also scans +`test/results/*.out` and the `test/install` actual output, failing on a +`not ok` without `# TODO`, a missing or duplicate `1..N` plan line, or a +plan that doesn't match the number of tests run (a test that died partway +through). Files with no TAP output are not checked. `make results` (via +`verify-results` in its default `pgtap` mode) gains the same plan checks +and the `test/install` scan. `make test` runs this check in the `pgtap` +(default) and `pgtap-only` modes of `PGXNTOOL_VERIFY_RESULTS_MODE`, and +skips it in `diffs` mode. Part of #106. + +== New features + +=== Add `PGXNTOOL_VERIFY_RESULTS_MODE=pgtap-only` +For pgTap suites whose output can't byte-match expected files, such as +assertion descriptions that embed a per-run role name. A test with TAP +output passes on the pgTap output check and on having no SQL error line; +its `regression.diffs` entry is ignored, in `make test` as well as +`make results`. Other tests are still judged by their diffs. + +== Bug fixes + +=== Fix `test/install` when `TESTOUT` differs from `TESTDIR` +`pg_regress` writes `test/install` actual output under `$(TESTOUT)` but +reads the expected output from `$(TESTDIR)`, so `make test TESTOUT=dir` +aborted with "Directory nonexistent" and "Bail out!". `make test` now +makes `$(TESTOUT)/install` a symlink to `$(TESTDIR)/install`, so that +output always lands in `$(TESTDIR)/install/`. It fails rather than replace +anything else already at `$(TESTOUT)/install`. + +Issues fixed in this release: #106 + 2.4.0 ----- == Breaking changes diff --git a/README.asc b/README.asc index 92f132d..c874765 100644 --- a/README.asc +++ b/README.asc @@ -52,7 +52,11 @@ Warns if two `$(DOCS)` entries would install to the same filename, which would o === test Runs your extension's test suite: filesystem-installs the extension (unless `PGXNTOOL_ENABLE_FS_INSTALL=no`) 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` also fails on two safeguard checks: +`make test` also fails on these safeguard checks: + +- The pgTap output check, run after `pg_regress` finishes: fails if `test/results/*.out` or the <<_testinstall,test/install>> actual output has a `not ok` line without `# TODO`, a missing or duplicate `1..N` plan line, or a different number of tests than planned -- even when the output matches its expected file, so a blessed failure still fails. Files with no TAP output are not checked. Runs unless <<_pgxntool_verify_results_mode,`PGXNTOOL_VERIFY_RESULTS_MODE`>> is `diffs`. + +- `regression.diffs`: any difference from expected output fails, except, under <<_pgtap_only,`PGXNTOOL_VERIFY_RESULTS_MODE=pgtap-only`>>, in a test whose output is TAP. Such a test fails instead on a SQL error line, and `make test` prints a `NOTE` naming the TAP tests whose differences it ignored. - The stale-expected-file check, run after `pg_regress` finishes: fails if `test/expected/` (or `test/build/expected/`) has a `.out` file with no matching `.sql`, or a file that isn't `.out`. See <<_pgxntool_enable_check_stale_expected>>. - When <<_testinstall,test/install>> is in use, the `ON_ERROR_STOP` check: fails if any `test/install/*.sql` file neither includes `test/pgxntool/psql.sql` nor sets `ON_ERROR_STOP` itself. See <<_pgxntool_enable_test_install_error_stop_check>>. @@ -68,7 +72,7 @@ Whether `test-build` runs is controlled by the `PGXNTOOL_ENABLE_TEST_BUILD` vari 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. -NOTE: `test` exits non-zero (after printing `regression.diffs`) if any test fails, so CI and other automation can rely on its exit code. +NOTE: `test` exits non-zero (after printing any pgTap problems and `regression.diffs`) if any test fails, so CI and other automation can rely on its exit code. 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. @@ -206,7 +210,7 @@ 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, the `test/install` directory'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). +WARNING: **`+test/install/*.out+` is never actually compared against anything.** Unlike every other test type pgxntool supports, the `test/install` directory's actual output is written to the exact same file as its expected output (in `$(TESTDIR)/install/`, even when `TESTOUT` is set; see <<_testout>>), so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. Apart from pgTap failures (below), 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: its actual output goes through the same pgTap output check as `test/sql` (see <<_pgxntool_verify_results_mode>>), so a `not ok` or plan problem there does fail `make test`. ==== Update & Upgrade (U&U) Testing @@ -272,10 +276,11 @@ IMPORTANT: *`make results` requires manual verification first*. The correct work Never run `make results` without first verifying the test changes are correct. The `results` target copies files from `test/results/` to `test/expected/`, so running it blindly will make incorrect output become the new expected behavior. ==== verify-results safeguard -By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`, which must be one of these two values (case-insensitive; anything else is a hard `make` error): +By default, `make results` will refuse to run if your tests are failing, so you can't accidentally promote incorrect output into the new expected results. Which failures it looks for is controlled by `PGXNTOOL_VERIFY_RESULTS_MODE`, which must be one of these three values (case-insensitive; anything else is a hard `make` error): -`pgtap` (the default):: Scans `test/results/*.out` for pgTAP `not ok` lines (ignoring `# TODO` items) and plan-count mismatches, then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. -`diffs`:: Only checks whether `test/regression.diffs` exists, so it can't create a new test's first expected output; use `pgtap` mode, or `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. +`pgtap` (the default):: Scans `test/results/*.out` and the <<_testinstall,test/install>> actual output for pgTAP `not ok` lines (ignoring `# TODO` items), missing or duplicate plan lines, and plan-count mismatches (the same check `make test` runs in this mode), then also checks `test/regression.diffs` for any output mismatch pgTAP itself wouldn't catch. A test that has no expected output file yet is the one exception: it always shows up in `regression.diffs` (pgxntool creates an empty expected file for it, because `pg_regress` aborts on a missing one), so `make results` is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. +`pgtap-only`:: Like `pgtap`, except that a test whose output is TAP is judged only by its pgTAP output and by having no SQL error line; its `regression.diffs` entry is ignored. This mode governs `make test` too; see <<_pgtap_only>>. +`diffs`:: Only checks whether `test/regression.diffs` exists, and `make test` skips the pgTap output check. It can't create a new test's first expected output; use `pgtap` mode, or `PGXNTOOL_ENABLE_VERIFY_RESULTS=no` for that one run. If tests are failing, `make results` prints an explanation of what failed and exits without touching `test/expected/`; fix the failures, then re-run `make results`. @@ -284,7 +289,7 @@ It also refuses when `pg_regress` didn't finish at all — an unreachable server To select a mode explicitly: ---- # In your Makefile -PGXNTOOL_VERIFY_RESULTS_MODE = diffs # or pgtap (the default) +PGXNTOOL_VERIFY_RESULTS_MODE = diffs # or pgtap-only, or pgtap (the default) ---- To disable this safeguard entirely (not recommended): @@ -782,19 +787,26 @@ Default: `test`. Root directory for test input files: `$(TESTDIR)/sql/`, `$(TEST === TESTOUT -Default: `$(TESTDIR)`. Directory `pg_regress` writes actual test output to -- `$(TESTOUT)/results/`, `$(TESTOUT)/regression.diffs`, etc. -- via `--outputdir` in `REGRESS_OPTS`. Kept separate from `TESTDIR` so generated output can be pointed somewhere other than the directory holding your checked-in `sql`/`expected` files, if you want that separation. +Default: `$(TESTDIR)`. Directory `pg_regress` writes actual test output to -- `$(TESTOUT)/results/`, `$(TESTOUT)/regression.diffs`, etc. -- via `--outputdir` in `REGRESS_OPTS`. Kept separate from `TESTDIR` so generated output can be pointed somewhere other than the directory holding your checked-in `sql`/`expected` files, if you want that separation. The one exception is <<_testinstall,test/install>> output, which always lands in `$(TESTDIR)/install/` (gitignored): when `TESTOUT` differs from `TESTDIR`, `make test` creates `$(TESTOUT)/install` as a symlink to it, and fails rather than replace anything else already at that path. === PGXNTOOL_VERIFY_RESULTS_MODE -Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing. Must be `pgtap` or `diffs` (case-insensitive) when given explicitly; any other value, including an empty one, is a hard `make` error. +Default: `pgtap`. Controls how the <<_verify_results_safeguard,verify-results safeguard>> decides whether tests are failing, and whether `make test` runs the pgTap output check (it does in `pgtap` and `pgtap-only` modes, and skips it in `diffs` mode). Must be `pgtap`, `pgtap-only` or `diffs` (case-insensitive) when given explicitly; any other value, including an empty one, is a hard `make` error. ==== pgtap Scans `test/results/*.out` for pgTap `not ok` lines and plan mismatches, falling back to checking `regression.diffs` too — except for a test that has no expected output yet, whose first expected file `make results` is allowed to create. Use this mode when your test suite uses pgTap. +[[_pgtap_only]] +==== pgtap-only + +For pgTap suites whose output can't byte-match a committed expected file, such as assertion descriptions that embed a per-run role name. A test whose result file is TAP output (it has a top-level `ok N`, `not ok N` or `1..N` line) passes on two things: the pgTap output check, and no SQL error line (`ERROR:` as psql prints it). Its `regression.diffs` entry is ignored. This applies to `make test` as well as `make results`; `make test` prints one `NOTE` line naming the TAP tests whose differences it ignored. + +A test without TAP output is still judged by its diff: any difference fails `make test`, and `make results` applies the `pgtap` rules above. <<_testinstall,test/install>> output gets the pgTap output check only. + ==== diffs -Checks only for `regression.diffs`, matching classic pg_regress behavior. Use this mode when your tests use plain SQL expected-output comparison only. +Checks only for `regression.diffs`, matching classic pg_regress behavior; `make test` skips the pgTap output check. Use this mode when your tests use plain SQL expected-output comparison only. === PGXNTOOL_ENABLE_TEST_BUILD * diff --git a/README.html b/README.html index 10a92d7..f513946 100644 --- a/README.html +++ b/README.html @@ -633,11 +633,17 @@

Runs your extension’s test suite: filesystem-installs the extension (unless PGXNTOOL_ENABLE_FS_INSTALL=no) 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 also fails on two safeguard checks:

+

make test also fails on these safeguard checks:

-test exits non-zero (after printing regression.diffs) if any test fails, so CI and other automation can rely on its exit code. +test exits non-zero (after printing any pgTap problems and regression.diffs) if any test fails, so CI and other automation can rely on its exit code. @@ -926,7 +932,7 @@

Warning -test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’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). +test/install/*.out is never actually compared against anything. Unlike every other test type pgxntool supports, the test/install directory’s actual output is written to the exact same file as its expected output (in $(TESTDIR)/install/, even when TESTOUT is set; see TESTOUT), so there is no diff — a wrong or changed result will never fail the build, no matter how much it changes. Apart from pgTap failures (below), 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: its actual output goes through the same pgTap output check as test/sql (see PGXNTOOL_VERIFY_RESULTS_MODE), so a not ok or plan problem there does fail make test. @@ -1082,17 +1088,21 @@

4.7.1. verify-results safeguard

-

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

+

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

pgtap (the default)
-

Scans test/results/*.out for pgTAP not ok lines (ignoring # TODO items) and plan-count mismatches, then also checks test/regression.diffs for any output mismatch pgTAP itself wouldn’t catch. A test that has no expected output file yet is the one exception: it always shows up in regression.diffs (pgxntool creates an empty expected file for it, because pg_regress aborts on a missing one), so make results is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

+

Scans test/results/*.out and the test/install actual output for pgTAP not ok lines (ignoring # TODO items), missing or duplicate plan lines, and plan-count mismatches (the same check make test runs in this mode), then also checks test/regression.diffs for any output mismatch pgTAP itself wouldn’t catch. A test that has no expected output file yet is the one exception: it always shows up in regression.diffs (pgxntool creates an empty expected file for it, because pg_regress aborts on a missing one), so make results is allowed to create its first expected output — provided its own pgTAP output is clean and free of SQL errors. If a new test legitimately needs output containing a SQL error blessed, use PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

+
+
pgtap-only
+
+

Like pgtap, except that a test whose output is TAP is judged only by its pgTAP output and by having no SQL error line; its regression.diffs entry is ignored. This mode governs make test too; see pgtap-only.

diffs
-

Only checks whether test/regression.diffs exists, so it can’t create a new test’s first expected output; use pgtap mode, or PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

+

Only checks whether test/regression.diffs exists, and make test skips the pgTap output check. It can’t create a new test’s first expected output; use pgtap mode, or PGXNTOOL_ENABLE_VERIFY_RESULTS=no for that one run.

@@ -1108,7 +1118,7 @@ 

# In your Makefile
-PGXNTOOL_VERIFY_RESULTS_MODE = diffs  # or pgtap (the default)
+PGXNTOOL_VERIFY_RESULTS_MODE = diffs # or pgtap-only, or pgtap (the default)
@@ -2112,13 +2122,13 @@

8.8. TESTOUT

-

Default: $(TESTDIR). Directory pg_regress writes actual test output to — $(TESTOUT)/results/, $(TESTOUT)/regression.diffs, etc. — via --outputdir in REGRESS_OPTS. Kept separate from TESTDIR so generated output can be pointed somewhere other than the directory holding your checked-in sql/expected files, if you want that separation.

+

Default: $(TESTDIR). Directory pg_regress writes actual test output to — $(TESTOUT)/results/, $(TESTOUT)/regression.diffs, etc. — via --outputdir in REGRESS_OPTS. Kept separate from TESTDIR so generated output can be pointed somewhere other than the directory holding your checked-in sql/expected files, if you want that separation. The one exception is test/install output, which always lands in $(TESTDIR)/install/ (gitignored): when TESTOUT differs from TESTDIR, make test creates $(TESTOUT)/install as a symlink to it, and fails rather than replace anything else already at that path.

8.9. PGXNTOOL_VERIFY_RESULTS_MODE

-

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

+

Default: pgtap. Controls how the verify-results safeguard decides whether tests are failing, and whether make test runs the pgTap output check (it does in pgtap and pgtap-only modes, and skips it in diffs mode). Must be pgtap, pgtap-only or diffs (case-insensitive) when given explicitly; any other value, including an empty one, is a hard make error.

8.9.1. pgtap

@@ -2127,9 +2137,18 @@

-

8.9.2. diffs

+

8.9.2. pgtap-only

+
+

For pgTap suites whose output can’t byte-match a committed expected file, such as assertion descriptions that embed a per-run role name. A test whose result file is TAP output (it has a top-level ok N, not ok N or 1..N line) passes on two things: the pgTap output check, and no SQL error line (ERROR: as psql prints it). Its regression.diffs entry is ignored. This applies to make test as well as make results; make test prints one NOTE line naming the TAP tests whose differences it ignored.

+
+
+

A test without TAP output is still judged by its diff: any difference fails make test, and make results applies the pgtap rules above. test/install output gets the pgTap output check only.

+
+
+
+

8.9.3. diffs

-

Checks only for regression.diffs, matching classic pg_regress behavior. Use this mode when your tests use plain SQL expected-output comparison only.

+

Checks only for regression.diffs, matching classic pg_regress behavior; make test skips the pgTap output check. Use this mode when your tests use plain SQL expected-output comparison only.

@@ -2217,7 +2236,7 @@

diff --git a/base.mk b/base.mk index 36c7e79..61778a4 100644 --- a/base.mk +++ b/base.mk @@ -200,11 +200,16 @@ endif # 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. +# resolves the expected output to $(TESTDIR)/install/.out and the actual +# output to $(TESTOUT)/install/.out. When TESTOUT differs from TESTDIR, +# $(TESTOUT)/install is a symlink to $(TESTDIR)/install (see +# _test-install-output-link), so the output always lands in +# $(TESTDIR)/install/ and both paths are the same file. 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 TAP check (see +# PGXNTOOL_VERIFY_RESULTS_MODE) does still scan that actual output. # # 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 @@ -299,17 +304,31 @@ _PGXNTOOL_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check # - Usage: Controls whether verify-results target exists and blocks make results # # Variable: PGXNTOOL_VERIFY_RESULTS_MODE -# - Controls how verify-results detects test failures -# - Allowed values: "pgtap" or "diffs" (case-insensitive); anything else, -# including an empty value, is a parse-time error -# - "pgtap" (default): scans test/results/*.out for "not ok" lines and plan -# mismatches (TAP failures). Also checks regression.diffs as a fallback. -# Use this mode when your test suite uses pgTap. -# - "diffs": checks only for regression.diffs existence (classic pg_regress behavior) -# Use this mode when your tests use plain SQL expected-output comparison only. -# -# Implementation: See verify-results target definition and results target modification -# (search for "verify-results" and "results:" in this file) +# - Controls how verify-results and `make test` detect test failures, +# including whether `make test` runs the TAP check +# - Allowed values: "pgtap", "pgtap-only" or "diffs" (case-insensitive); +# anything else, including an empty value, is a parse-time error +# - "pgtap" (default): runs check-tap-output.sh over $(TESTOUT)/results/*.out +# and the test/install actual output, failing on a `not ok` without +# `# TODO`, a missing or duplicate plan line, or a plan/test count +# mismatch -- even when the output matches its expected file. Also +# checks regression.diffs. Applies to both `make test` and +# verify-results. Use this mode when your test suite uses pgTap. +# - "pgtap-only": like "pgtap", but a test whose result file is TAP output +# passes on its TAP and on having no SQL error line; its regression.diffs +# block is ignored, in `make test` as well as verify-results. For pgTap +# suites whose assertion descriptions vary per run. +# - "diffs": checks only for regression.diffs existence (classic pg_regress +# behavior); `make test` skips the TAP check. Use this mode when your +# tests use plain SQL expected-output comparison only. +# +# Variable: _PGXNTOOL_CHECK_TAP_OUTPUT_SCRIPT (internal shim, not user-facing) +# - Path to the script the `test` recipe invokes +# - Default: $(PGXNTOOL_DIR)/test/bin/check-tap-output.sh +# +# Implementation: See verify-results target definition, results target modification +# and the `test:` recipe (search for "verify-results", "results:" +# and "test: $(TEST_DEPS)" in this file) # ifdef PGXNTOOL_ENABLE_VERIFY_RESULTS override PGXNTOOL_ENABLE_VERIFY_RESULTS := $(call pgxntool_validate_yesno,$(PGXNTOOL_ENABLE_VERIFY_RESULTS),PGXNTOOL_ENABLE_VERIFY_RESULTS) @@ -320,7 +339,9 @@ endif # Default mode: pgtap (scans results/*.out for TAP failures) PGXNTOOL_VERIFY_RESULTS_MODE ?= pgtap -override PGXNTOOL_VERIFY_RESULTS_MODE := $(call _pgxntool_validate_choice,$(PGXNTOOL_VERIFY_RESULTS_MODE),PGXNTOOL_VERIFY_RESULTS_MODE,pgtap diffs) +override PGXNTOOL_VERIFY_RESULTS_MODE := $(call _pgxntool_validate_choice,$(PGXNTOOL_VERIFY_RESULTS_MODE),PGXNTOOL_VERIFY_RESULTS_MODE,pgtap pgtap-only diffs) + +_PGXNTOOL_CHECK_TAP_OUTPUT_SCRIPT ?= $(PGXNTOOL_DIR)/test/bin/check-tap-output.sh # ------------------------------------------------------------------------------ # _check-stale-expected: catch orphaned/unexpected test/expected/ files @@ -415,6 +436,10 @@ ifeq ($(PGXNTOOL_ENABLE_TEST_INSTALL),yes) _PGXNTOOL_INSTALL_SCHEDULE = $(TESTDIR)/install/schedule EXTRA_CLEAN += $(_PGXNTOOL_INSTALL_SCHEDULE) +# Actual output of each install file (see the test/install section above), +# for the TAP output check. +_PGXNTOOL_TEST_INSTALL_OUTPUT_FILES = $(TEST_INSTALL_SQL_FILES:.sql=.out) + # Add install schedule; REGRESS stays as-is (regular tests run after schedule) REGRESS_OPTS += --schedule=$(_PGXNTOOL_INSTALL_SCHEDULE) @@ -427,6 +452,24 @@ $(_PGXNTOOL_INSTALL_SCHEDULE): done installcheck: $(_PGXNTOOL_INSTALL_SCHEDULE) + +# pg_regress writes the actual output to $(TESTOUT)/results/../install/ but +# diffs it against $(TESTDIR)/expected/../install/; those only agree if +# $(TESTOUT)/install and $(TESTDIR)/install are the same directory. Never +# replace something else already at that path: it isn't ours to delete. +ifneq ($(abspath $(TESTOUT)),$(abspath $(TESTDIR))) +.PHONY: _test-install-output-link +_test-install-output-link: + @link='$(TESTOUT)/install'; target='$(abspath $(TESTDIR)/install)'; \ + if [ -L "$$link" ] && [ "$$(readlink "$$link")" = "$$target" ]; then exit 0; fi; \ + if [ -e "$$link" ] || [ -L "$$link" ]; then \ + echo "ERROR: $$link exists and is not a symlink to $$target" >&2; \ + echo "test/install output must land in $$target; remove $$link and rerun" >&2; \ + exit 1; \ + fi; \ + mkdir -p '$(TESTOUT)' && ln -s "$$target" "$$link" +installcheck: | _test-install-output-link +endif endif PGXS := $(shell $(PG_CONFIG) --pgxs) @@ -529,8 +572,22 @@ ifeq ($(PGXNTOOL_ENABLE_FS_INSTALL),yes) TEST_DEPS += install endif TEST_DEPS += installcheck +# +# The TAP check runs in this recipe rather than as a TEST_DEPS target so that +# `make PGXNTOOL_ENABLE_VERIFY_RESULTS=no results` (which depends on TEST_DEPS, +# not on `test`) still blesses output unconditionally. Outside diffs mode both +# checks always run, so a failure reports TAP problems and regression.diffs together. In +# pgtap-only mode, verify-results-pgtap.sh decides which regression.diffs +# blocks still count. test: $(TEST_DEPS) +ifneq ($(PGXNTOOL_VERIFY_RESULTS_MODE),diffs) + @status=0; \ + $(_PGXNTOOL_CHECK_TAP_OUTPUT_SCRIPT) $(TESTOUT)/results/*.out $(_PGXNTOOL_TEST_INSTALL_OUTPUT_FILES) || status=1; \ + $(if $(filter pgtap-only,$(PGXNTOOL_VERIFY_RESULTS_MODE)),$(PGXNTOOL_DIR)/verify-results-pgtap.sh --pgtap-only --for-test $(TESTOUT) || status=1;,if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; status=1; fi;) \ + exit $$status +else @if [ -r $(TESTOUT)/regression.diffs ]; then cat $(TESTOUT)/regression.diffs; exit 1; fi +endif # # verify-results: Safeguard for make results @@ -538,9 +595,9 @@ test: $(TEST_DEPS) # Checks if tests are passing before allowing make results to proceed ifeq ($(PGXNTOOL_ENABLE_VERIFY_RESULTS),yes) .PHONY: verify-results -ifeq ($(PGXNTOOL_VERIFY_RESULTS_MODE),pgtap) +ifneq ($(PGXNTOOL_VERIFY_RESULTS_MODE),diffs) verify-results: - @$(PGXNTOOL_DIR)/verify-results-pgtap.sh $(TESTOUT) + @$(PGXNTOOL_DIR)/verify-results-pgtap.sh $(if $(filter pgtap-only,$(PGXNTOOL_VERIFY_RESULTS_MODE)),--pgtap-only )$(TESTOUT) $(_PGXNTOOL_TEST_INSTALL_OUTPUT_FILES) else verify-results: @if [ -r $(TESTOUT)/regression.diffs ]; then \ diff --git a/test/bin/check-tap-output.sh b/test/bin/check-tap-output.sh new file mode 100755 index 0000000..e813f44 --- /dev/null +++ b/test/bin/check-tap-output.sh @@ -0,0 +1,80 @@ +#!/usr/bin/env bash +# +# check-tap-output.sh - Fail on TAP (pgTap) failures in pg_regress output +# +# A file that matches its expected output byte for byte still fails here if +# its TAP says the test failed, so a `not ok` blessed into an expected file +# can't pass `make test` (issue #106). +# +# A file counts as TAP output if it has a top-level test line +# (`ok N`/`not ok N`) or plan line (`1..N`); every other file passes +# untouched. A TAP file fails if: +# - it has a `not ok` line without `# TODO` +# - it has pgTap's "Looks like you planned" line +# - it has no plan line: a no_plan() test that died before finish() +# - it has more than one plan line: nothing sane to compare against +# - its count of top-level test lines differs from the plan: a plan(N) +# test that died partway through (`1..0` with no tests passes) +# Only unindented lines count, so subtest output is ignored. +# +# Arguments that don't exist are ignored, so an unmatched shell glob is +# harmless. +# +# Usage: check-tap-output.sh [--list] FILE... +# Exits 1 if any file fails, else 0. With --list, checks nothing: prints +# each FILE that counts as TAP output, one per line, and exits 0. + +set -o errexit -o errtrace -o pipefail + +list= +if [ "${1:-}" = --list ]; then + list=1 + shift +fi + +failed=0 +for f in "$@"; do + [ -f "$f" ] || continue + + # " ", where is the last plan's count. + read -r tests plans planned < <(awk ' + /^(not )?ok [0-9]/ { tests++ } + /^1\.\.[0-9]/ { plans++; planned = substr($1, 4) + 0 } + END { print tests + 0, plans + 0, planned + 0 } + ' "$f") + + [ "$tests" -gt 0 ] || [ "$plans" -gt 0 ] || continue + + if [ -n "$list" ]; then + printf '%s\n' "$f" + continue + fi + + notok=$(grep '^not ok' "$f" | grep -v '# TODO' || true) + if [ -n "$notok" ]; then + echo "ERROR: pgtap failure detected in $f" + echo "$notok" + failed=1 + fi + + # One plan problem per file: pgTap's own mismatch line already says what a + # count check would. + if grep -q 'Looks like you planned' "$f"; then + echo "ERROR: pgtap plan mismatch in $f" + grep 'Looks like you planned' "$f" + failed=1 + elif [ "$plans" -eq 0 ]; then + echo "ERROR: pgtap output in $f has no plan line (1..N); the test likely died before finishing" + failed=1 + elif [ "$plans" -gt 1 ]; then + echo "ERROR: pgtap output in $f has $plans plan lines (1..N); expected exactly one" + failed=1 + elif [ "$tests" -ne "$planned" ]; then + echo "ERROR: pgtap plan mismatch in $f: planned $planned tests but found $tests" + failed=1 + fi +done + +exit "$failed" + +# vi: expandtab ts=2 sw=2 diff --git a/verify-results-pgtap.sh b/verify-results-pgtap.sh index afd24a0..0920ee5 100755 --- a/verify-results-pgtap.sh +++ b/verify-results-pgtap.sh @@ -1,42 +1,81 @@ #!/usr/bin/env bash # pgxntool/verify-results-pgtap.sh - Check pgtap results before 'make results' # -# Scans pgtap output files for failures and plan mismatches, then checks -# regression.diffs as a fallback, ignoring entries for tests that have no -# expected output yet. Exits non-zero if any problems are found. +# Scans pgtap output files for failures and plan problems (see +# test/bin/check-tap-output.sh), then checks regression.diffs as a fallback, +# ignoring entries for tests that have no expected output yet. Exits non-zero +# if any problems are found. # -# Usage: verify-results-pgtap.sh TESTOUT +# Usage: verify-results-pgtap.sh [--pgtap-only] [--for-test] TESTOUT [FILE...] +# FILE: additional pgtap output files to scan besides TESTOUT/results/*.out +# --pgtap-only: a test whose result file is TAP output is judged by its TAP +# and by having no SQL error line, not by its regression.diffs block +# --for-test: judge regression.diffs the way `make test` does: every block +# left fails, and the TAP scan is left to the caller # -# Called by the verify-results target in base.mk (pgtap mode). +# Called by the verify-results target in base.mk, and by the `test` recipe +# when PGXNTOOL_VERIFY_RESULTS_MODE=pgtap-only. set -e -TESTOUT="${1:?Usage: verify-results-pgtap.sh TESTOUT}" - -# Check for pgtap failures in result files (excluding TODO items) -failed=0 -for f in "$TESTOUT"/results/*.out; do - [ -f "$f" ] || continue - if grep -q '^not ok' "$f"; then - notok=$(grep '^not ok' "$f" | grep -v '# TODO' || true) - if [ -n "$notok" ]; then - echo "ERROR: pgtap failure detected in $f" - echo "$notok" - failed=1 - fi - fi - if grep -q 'Looks like you planned' "$f"; then - echo "ERROR: pgtap plan mismatch in $f" - grep 'Looks like you planned' "$f" - failed=1 - fi +pgtap_only= +for_test= +while :; do + case ${1:-} in + --pgtap-only) pgtap_only=1; shift ;; + --for-test) for_test=1; shift ;; + *) break ;; + esac done -if [ $failed -ne 0 ]; then + +TESTOUT="${1:?Usage: verify-results-pgtap.sh [--pgtap-only] [--for-test] TESTOUT [FILE...]}" +shift + +check_tap="$(dirname "$0")/test/bin/check-tap-output.sh" + +if [ -n "$for_test" ]; then + cannot= +else + cannot=" Cannot run 'make results'." +fi +status=0 + +# pg_regress feeds psql on stdin, so a failing statement in the test file +# itself reports as "ERROR: ..." at column 0, while one inside an \i'd file +# (setup.sql, finish.sql) reports as "psql:test/pgxntool/setup.sql:3: ERROR: +# ...". Matching either position, and psql's own two spaces after the +# severity, keeps a passing test whose description merely mentions "ERROR:" +# out of it. +sql_error='(^|: )ERROR: ' + +# Check for pgtap failures in result files and any extra files (base.mk passes +# the test/install actual outputs). +if [ -z "$for_test" ] && ! "$check_tap" "$TESTOUT"/results/*.out "$@"; then echo - echo "pgtap failures detected. Cannot run 'make results'." + echo "pgtap failures detected.$cannot" exit 1 fi +# With its diff ignored, a SQL error is the one thing besides its TAP that can +# still fail a TAP test. +if [ -n "$pgtap_only" ]; then + errored= + while IFS= read -r f; do + if [ -n "$f" ] && grep -qE "$sql_error" "$f"; then + errored="$errored $f +" + fi + done <