Repository navigation
Fail make test on pgTap problems in actual output; add pgtap-only mode - #141
Open
jnasbyupgrade wants to merge 5 commits into
Open
jnasbyupgrade wants to merge 5 commits into
jnasbyupgrade wants to merge 5 commits into
Conversation
`make test` failed only on `regression.diffs`, so a `not ok` line blessed into an expected file passed, as did a test that died before printing its plan, and `test/install` output was never checked at all. Add `test/bin/check-tap-output.sh`, which fails a file with TAP output on a `not ok` without `# TODO`, pgTap's "Looks like you planned" line, a missing or duplicate `1..N` plan line, or a test count that differs from the plan. Files without TAP output pass untouched. - The `test` recipe runs it over `$(TESTOUT)/results/*.out` and the `test/install` actual output, then still prints `regression.diffs`; either failing fails the run. New `PGXNTOOL_ENABLE_CHECK_TAP_OUTPUT` (default `yes`) disables it. It lives in the recipe, not `TEST_DEPS`, so `make PGXNTOOL_ENABLE_VERIFY_RESULTS=no results` still blesses. - `verify-results-pgtap.sh` uses the same script for its TAP phase, so `make results` gains the plan checks and the `test/install` scan. - The `test/install` actual output is `$(TESTOUT)/install/<name>.out`; the base.mk comment now says so. Part of Postgres-Extensions#106. Related changes in pgxntool-test: - Unit tests for `check-tap-output.sh` and the new `verify-results-pgtap.sh` behavior - `make test` wiring tests, plus an end-to-end blessed `not ok` failure Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pg_regress writes `test/install` actual output to `$(TESTOUT)/results/../install/<name>.out` but diffs it against `$(TESTDIR)/expected/../install/<name>.out`. With a separate `TESTOUT`, `make test` aborted with "Directory nonexistent" and "Bail out!", and creating the directory only turned that into a missing-expected-file diff. When `test/install` is enabled and `TESTOUT` differs from `TESTDIR`, the new `_test-install-output-link` (an order-only prerequisite of `installcheck`) makes `$(TESTOUT)/install` a symlink to `$(TESTDIR)/install`, so both paths resolve to the same file again on every PostgreSQL version. Anything else already at that path fails the build rather than being replaced. `_PGXNTOOL_TEST_INSTALL_OUTPUT_FILES` now names the real file in `$(TESTDIR)/install/`. Related changes in pgxntool-test: - Test that the rule creates the symlink for a separate `TESTOUT`, and fails on a pre-existing directory
No related changes in pgxntool-test.
pgTap suites whose assertion descriptions embed per-run names (a suffixed role, say) can never byte-match committed expected output, even when every assertion passes. In the new `pgtap-only` mode, a test whose result file is TAP output passes on the pgTap output check and on having no SQL error line; its `regression.diffs` block is ignored. This applies to `make test` as well as `make results`. Tests without TAP output are still judged by their diffs, and `test/install` output gets the TAP check only. - `verify-results-pgtap.sh` gains `--pgtap-only`, plus `--for-test` for `make test`'s rule that every remaining block fails. The same regression.diffs parser serves both. `make test` prints one NOTE line naming the TAP tests whose differences it ignored. - `check-tap-output.sh --list` prints which files count as TAP output, so both scripts share one definition. - `pgtap-only` with `PGXNTOOL_ENABLE_CHECK_TAP_OUTPUT=no` is a parse-time error, since TAP tests would then pass on nothing. Fixes Postgres-Extensions#106 Related changes in pgxntool-test: - Direct-script tests for `--pgtap-only`, `--for-test` and `--list` - Mode validation and `make test` dry-run wiring for `pgtap-only`
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool-test
that referenced
this pull request
Oct 14, 2026
Add tests for Postgres-Extensions/pgxntool@3e8be3f (part of Postgres-Extensions/pgxntool#141): with `TESTOUT` different from `TESTDIR`, `_test-install-output-link` makes `$(TESTOUT)/install` a symlink to `$(TESTDIR)/install`, and fails rather than replace a pre-existing directory there. Both checks run without Postgres, folded into the existing schedule test in `test-test-install.bats`.
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool-test
that referenced
this pull request
Oct 14, 2026
Add tests for Postgres-Extensions/pgxntool@15e558b (part of Postgres-Extensions/pgxntool#141, for Postgres-Extensions/pgxntool#106): - `verify-results-pgtap-script.bats`: under `--pgtap-only` (with and without `--for-test`), a TAP test whose only difference is its description passes, and fails under plain pgtap; a non-TAP diff still fails; a TAP test with a SQL error line fails. - `check-tap-output-script.bats`: `--list` names only TAP files. - `make-test.bats`: the mode validation tests cover `pgtap-only` and its parse-time error with `PGXNTOOL_ENABLE_CHECK_TAP_OUTPUT=no`, and a dry run shows `make test` and `verify-results` using the new logic.
make test on pgTap failures and plan problems in actual outputmake test on pgTap problems in actual output; add pgtap-only mode
`PGXNTOOL_VERIFY_RESULTS_MODE` already says whether a suite uses pgTap, so it now also decides whether `make test` runs the TAP output check: it runs in the `pgtap` (default) and `pgtap-only` modes and is skipped in `diffs` mode. That removes the separate toggle and the parse-time error for `pgtap-only` combined with it. Part of Postgres-Extensions#106. Related changes in pgxntool-test: - The wiring tests use `PGXNTOOL_VERIFY_RESULTS_MODE=diffs` to prove the TAP check is skipped; tests for the removed variable are dropped
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool-test
that referenced
this pull request
Oct 15, 2026
Update tests for Postgres-Extensions/pgxntool@7eec927 (part of Postgres-Extensions/pgxntool#141, for Postgres-Extensions/pgxntool#106): - `make-test.bats`: the dry-run and stub tests prove `PGXNTOOL_VERIFY_RESULTS_MODE=diffs` skips the TAP check, instead of the removed variable; drop its invalid-value test and the `pgtap-only` conflict assertion. - `pgxntool-variables.txt`: drop the variable from the inventory.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
make testnow fails on pgTap failures in actual output, not just onregression.diffs: anot okwithout# TODO, a missing or duplicate1..Nplan line, or a test count that differs from the plan. It also scans thetest/installactual output, which never gets a real diff. A newPGXNTOOL_VERIFY_RESULTS_MODE=pgtap-onlyjudges TAP tests by their TAP alone, ignoring description-only differences, which is the validator #106 asks for. This PR also fixestest/installunder a separateTESTOUT.Fixes #106
Merge order: merge this PR first, then the paired pgxntool-test PR (Postgres-Extensions/pgxntool-test#107). This is an addition, and the test PR exercises behavior that only exists here.
pgTap output check
test/bin/check-tap-output.sh FILE.... A file counts as TAP output if it has a top-levelok N/not ok Nor1..Nline; all other files pass untouched. Nonexistent arguments are ignored, so an unmatched glob is harmless.1..0with no tests passes.--listprints which files count as TAP output and checks nothing.testrecipe runs it over$(TESTOUT)/results/*.outplus eachtest/installactual output, then still printsregression.diffs. If either check fails, the run fails, and both are reported.PGXNTOOL_VERIFY_RESULTS_MODEdecides whethermake testruns the check: it does in thepgtap(default) andpgtap-onlymodes, and skips it indiffsmode. The check lives in thetestrecipe rather thanTEST_DEPS, somake PGXNTOOL_ENABLE_VERIFY_RESULTS=no resultsstill works as an escape hatch.verify-results-pgtap.shuses the same script for its TAP phase, taking thetest/installoutputs as extra arguments.make resultstherefore gains the plan checks and thetest/installscan.pgtap-onlymodecheck-tap-output.sh's definition) passes on two things: the pgTap output check, and no SQL error line (the(^|: )ERROR:matchverify-results-pgtap.shalready uses). Itsregression.diffsblock is ignored. This applies tomake testandmake resultsalike;make testprints one NOTE line naming the TAP tests whose differences it ignored.make test, andmake resultsapplies the existingpgtaprules, including the allowance for a test with no expected output yet.test/installoutput gets the TAP check only.verify-results-pgtap.shtakes--pgtap-only, plus--for-testfor themake testrule that every remaining block fails. One regression.diffs parser serves both targets.pgtapanddiffsmodes behave as before.For review: where the mode lives
The mode is a third value of the existing
PGXNTOOL_VERIFY_RESULTS_MODE. That variable now governsmake test's pass criterion too, although its name is about verify-results. The alternative is a separate variable that governsmake test's pass criterion. This PR does not take that route; it is your call at review.test/installwith a separateTESTOUTpg_regress writes
test/installactual output to$(TESTOUT)/results/../install/<name>.outand diffs it against$(TESTDIR)/expected/../install/<name>.out. Those are the same file only whenTESTOUT = TESTDIR, somake test TESTOUT=dirfailed. Whentest/installis enabled andTESTOUTdiffers, a new_test-install-output-linkrule (an order-only prerequisite ofinstallcheck) makes$(TESTOUT)/installa symlink to$(TESTDIR)/install, on every PostgreSQL version. If something else is already at that path, the build fails rather than replace it. WithTESTOUT = TESTDIRnothing changes.test/installoutput therefore always lands in$(TESTDIR)/install/(gitignored).Reproduced with
make test TESTOUT=$(mktemp -d)on a repo built from the pgxntool-test template (PostgreSQL 17):cannot create .../results/../install/create_install_marker.out: Directory nonexistent, thenBail out!# All 4 tests passed., with$TESTOUT/install -> <repo>/test/install$TESTOUT/install: exit 2,ERROR: <dir>/install exists and is not a symlink to <repo>/test/installRejected alternatives:
TESTOUT != TESTDIR: leaves a documented variable broken whenevertest/installis used--expecteddir: PostgreSQL 16+ only$(TESTOUT): a diff on every runREADME.asc, CLAUDE.md and HISTORY.asc are updated. HISTORY lists the TAP check under breaking changes,
pgtap-onlyunder new features and theTESTOUTfix under bug fixes.