Combine make-test/make-results/verify-results into one suite - #65
Merged
jnasbyupgrade merged 3 commits intoJul 31, 2026
Merged
Conversation
`test`, `results`, and `verify-results` are one tightly-coupled pipeline -- results/verify-results only exist to safely update the output test checks against -- so test/standard/make-test.bats, make-results.bats, and test-verify-results.bats are merged into one file (make-test.bats survives; the other two are deleted) sharing a single foundation-copied environment instead of three, per CLAUDE.md's directive to minimize state changes/setup cost in the suite. The two separate mismatch-creation steps the old files used (test/expected/pgxntool-test.out vs base.out) are consolidated into one shared mismatch; the two entry points (`make results` and `make verify-results` directly) and all three pgtap-detection cases stay as distinct tests since they're genuinely separate coverage. Surveyed the rest of test/standard/*.bats for the same "fragmented pipeline" pattern: none qualify -- the closest candidates (test-test-build.bats vs test-test-install.bats, dist-clean.bats vs 02-dist.bats, and check-stale-expected-script.bats vs make-test.bats) are independently toggleable features or deliberate layer splits (script decision-logic tests vs Makefile-wiring tests), not fragments of one pipeline. Deleted 5 dead tests (test/output/test/input directory handling) left behind when pgxntool dropped .source file support (issues Postgres-Extensions#9/Postgres-Extensions#22): grepped base.mk and confirmed it never references test/output or test/input anywhere -- $(TESTOUT) defaults to test/results, a different path. These tests, and the defensive `rm -rf test/output` added for issue Postgres-Extensions#49, were purely working around pg_regress's own incidental optional *.source-file directory convention, which is pg_regress's behavior to own, not pgxntool's to re-verify. Added a real structural proof for issue #79 (installcheck must run after install): `make -p -n installcheck` dumps Make's parsed prerequisite database independent of runtime scheduling, and confirms `install` is listed as installcheck's prerequisite. Verified this check actually discriminates: it lists `install` against the fixed base.mk and omits it against the unfixed one. The previous behavioral test (`make uninstall && make test`) is kept as a secondary sanity check, since Make's actual scheduling order for two unrelated same-target prerequisites could have made it pass even against a broken base.mk purely by luck -- it doesn't prove the fix on its own, only the make -p check does. Trimmed two overly long inline comments per review: the issue Postgres-Extensions#49 exit-code assertion now just says "Ensure make exited non-zero"; the confusing pseudo-syntax quote in the results-blocked test was replaced with one section-header comment explaining results/verify-results/$(TEST_DEPS) once. Also fixed a comment that was already factually wrong post-fix (claimed verify-results depends on `test`, which base.mk no longer does). Fixed the same missing `\set ECHO none` issue as the main template (see the paired pgxntool-test fix-79-49 branch) in template-multi-extension/test/sql/base.sql -- latent since Postgres-Extensions#49's bug hid it, caught now that make test actually reports failure. This branch is based on current master and needs no pgxntool code change of its own, but its new/updated assertions exercise base.mk behavior that only exists on the pgxntool fix-79-49 branch (Postgres-Extensions/pgxntool#83) -- this repo's own CI falls back to pgxntool master when no same-named paired branch exists, so this PR's CI will not go green until pgxntool#83 merges. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
fix-dependent content The reorg in 40c7918 accidentally pulled in test content that only makes sense once pgxntool PR #83 (fix-79-49) lands, defeating the purpose of keeping this reorg mergeable against current pgxntool master on its own: - Delete the two tests asserting the `installcheck: install` edge (issue #79), which doesn't exist in base.mk yet. - Revert "make test shows diff..." to not assert a non-zero exit (issue Postgres-Extensions#49's fix, unmerged); current master's `test` recipe still never exits non-zero on a regression.diffs mismatch. - Revert the verify-results/results section's comments and one test name back to describing current master's real wiring (`verify-results: test`), not the `verify-results: $(TEST_DEPS)` wiring #83 would introduce. Verified against a fresh clone of real, unmodified Postgres-Extensions/pgxntool master: full suite green except one unrelated, pre-existing failure in test/sequential/04-pgtle.bats caused by this branch's base being behind pgxntool-test's own upstream master (which already carries bea9b18, a fix for pgxntool's unrelated PGTLE_VERSION->PGXNTOOL_PGTLE_VERSION rename in #85).
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool-test
that referenced
this pull request
Jul 31, 2026
…stgres-Extensions#49) Layers the issue #79/Postgres-Extensions#49/Postgres-Extensions#35 test additions on top of the make-test/ make-results/verify-results consolidation (PR Postgres-Extensions#65), building against pgxntool's fix-79-49 branch (base.mk's installcheck: install edge and test's real exit-status propagation, pgxntool PR #83): - "installcheck's parsed prerequisite list includes install (issue #79, structural proof)": make -p -n installcheck dump proves the installcheck: install edge exists, independent of Make's scheduling order for unrelated same-target prerequisites. - "make test succeeds from a genuinely uninstalled state (issue #79)": make uninstall && make test end-to-end sanity check. - "make test shows diff with modified expected output, and exits non-zero (issue Postgres-Extensions#49)": now asserts make test's own exit status instead of relying on .IGNORE: installcheck swallowing it. - Updates the verify-results/results section comments and test names to reflect that verify-results depends on $(TEST_DEPS) directly, not on the test target -- important now that test's recipe can exit 1 before verify-results gets a chance to inspect regression.diffs. This content was previously built and validated as part of an earlier, overly-broad version of the make-test.bats reorg (consolidate-make-test- suites branch, commit 40c7918) and then deliberately split back out (82c0c7c) so that reorg could land as a pure, dependency-free PR (Postgres-Extensions#65). This commit re-applies exactly that validated delta on top of the corrected reorg. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 task
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
1 participant
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.
Summary
test,results, andverify-resultsare one tightly-coupled pipeline --results/verify-resultsonly exist to safely update the outputtestchecks against. Per review feedback on #59, combinestest/standard/make-test.bats,make-results.bats, andtest-verify-results.batsinto one file (make-test.batssurvives, the other two are deleted), sharing a single foundation-copied environment instead of three.This is a pure reorg -- verified green against real, current
Postgres-Extensions/pgxntoolmaster, with no dependency on any unmerged pgxntool change.test/output/test/inputdirectory handling) left behind when pgxntool dropped.sourcefile support (issues Remove .source file support; update template and tests #9/Add CI workflows, /ci skill, and test fixes #22) -- confirmed viagrepthatbase.mknever references either path.make results/make verify-resultsentry points and all three pgtap-detection cases as distinct tests (genuinely separate coverage).test/standard/*.batsfor the same fragmented-pipeline pattern: no other files qualify (the closest candidates are independently-toggleable features or deliberate, already-documented layer splits).\set ECHO noneissue intemplate-multi-extension/test/sql/base.sql(pg_regress'spsql -aecho mode requires it) -- unrelated to any of the above, just a real independent bug in a test fixture.#59 (issues #79/#49/#35 fix coverage) is being rebased to build on top of this branch, since its new test coverage depends on the paired pgxntool#83 base.mk fix that this reorg does not.
Test plan
test-all) run against real, currentPostgres-Extensions/pgxntoolmaster (freshly cloned, confirmed unfixed): 236/236 passed, 0 failed, 0 skipped.🤖 Generated with Claude Code