Add test coverage for parallel-build corruption, stale-expected check, base.mk fixes - #31
Merged
jnasbyupgrade merged 6 commits intoJul 27, 2026
Conversation
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ion, stale-expected check, base.mk fixes): - Fix versioned SQL file generation race that doubled file content - Add check-stale-expected to catch orphaned test/expected/*.out files - Fix EXTRA_CLEAN target, add PGXN_REMOTE override, add base.mk include guard New BATS coverage in test/standard/: - versioned-sql-race.bats: races two concurrent `make all` invocations against the same versioned-SQL target across 5 iterations, asserting the file is never doubled (issue Postgres-Extensions#19) - check-stale-expected.bats: orphaned-file failure, the pg_regress alternate-output-file (_N.out) false-positive exemption plus a case proving it doesn't over-exempt, and the test/build/expected pair (issue Postgres-Extensions#14) - base-mk-fixes.bats: EXTRA_CLEAN targets the real test/results/ dir, PGXN_REMOTE default and override for tag/rmtag, and a double-include Makefile confirming base.mk's new guard is warning-free (issues Postgres-Extensions#7, Postgres-Extensions#50, #53) Issue Postgres-Extensions#28 (variable rename) deliberately has no new test -- pure mechanical rename, no behavior change, already covered by the existing suite's DATA/build-output coverage. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…s a script, ordering fix, variable rename): - check-stale-expected logic moved to check-stale-expected.sh - Now depends on installcheck directly so it runs after pg_regress - EXTENSION_VERSION_FILES renamed to EXTENSION__CURRENT_VERSION__FILES - Replace the old "fails fast, before install/installcheck" test with one that verifies the new ordering via a make -n test dry-run (pg_regress's invocation must appear before check-stale-expected.sh), plus a test proving make test only fails on a stale file after pg_regress has produced real results (test/results/*.out) - Add pgxntool/check-stale-expected.sh to the dist file manifest Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ipt move, file-type check, enable toggles) - check-stale-expected.sh moved to test/bin/check-stale-expected.sh - expected/ now also validated for non-*.out files (distinct error/exit code) - PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED and PGXNTOOL_CHECK_EXPECTED_FILE_TYPES toggles added - Update dist-expected-files.txt for the new script path/directory - Add coverage for the non-.out file check (direct script invocation, since make's own exit status masks the child's real code) and for PGXNTOOL_CHECK_EXPECTED_FILE_TYPES=no - Fix the ordering test to exclude test-build's own pg_regress invocation when locating the real installcheck one, since the TEST_DEPS reordering in this commit changed their relative dry-run output order (no ordering relationship was ever guaranteed between them -- only the explicit check-stale-expected: installcheck edge matters) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 27, 2026 17:38
2650a32 to
a9c7ce9
Compare
…Extensions#14, Postgres-Extensions#19) Per maintainer feedback on this PR: avoid creating new test suites willy-nilly, since the goal is to reduce the amount of state changes needed in tests. - Delete test/standard/versioned-sql-race.bats (issue Postgres-Extensions#19); its two @test blocks (and the explanatory header on the parallel-build race) now live in test/standard/concurrent-make-test.bats, reusing that file's existing single-extension REPO1_ENV instead of building a second dedicated environment. - Delete test/standard/check-stale-expected.bats (issue Postgres-Extensions#14); its nine @test blocks (and the explanatory header) now live in test/standard/make-test.bats, whose existing `ensure_foundation` environment and cwd handling makes check-stale-expected.bats's own setup_file()/setup() redundant. This is also a better thematic home, since check-stale-expected is now literally a `make test` prerequisite. - Add `skip_if_no_postgres` to the moved "make test fails on a stale expected file, but only after pg_regress has already run" test, which runs a real `make test` and was missing the guard already used elsewhere in make-test.bats. Total unique test count is unchanged (224); full suite run clean. Changes only in pgxntool-test. No related changes in pgxntool. Co-Authored-By: Claude <noreply@anthropic.com>
…ensions#7, Postgres-Extensions#50, #53) Per maintainer feedback on this PR (continuing the fold started in 4b2c281): organize suites by what code they test, not by which bug report motivated the test. A suite of nothing but bug fixes is the anti-pattern being corrected. - EXTRA_CLEAN test(s) (Postgres-Extensions#7 - make clean must remove the real test/results/ directory) move into test/standard/make-test.bats, placed with that file's existing test/output directory lifecycle tests. - PGXN_REMOTE test(s) (#53 - the remote used by tag/rmtag/forcetag/ dist) move into test/standard/dist-clean.bats, which already exercises those targets directly and whose shared foundation environment already configures an "origin" remote. - The base.mk double-include guard test (Postgres-Extensions#50) has no natural existing home, since it tests base.mk's own inclusion mechanics rather than a user-facing feature. It moves into its own file, renamed from base-mk-fixes.bats to base-mk-include-guard.bats to name the tested behavior instead of referencing "fixes" or an issue number. - Delete test/standard/base-mk-fixes.bats. No other file references its old name. Also fix a stale comment in make-test.bats: the check-stale-expected header (carried over verbatim from check-stale-expected.bats in 4b2c281) said it runs "before install/installcheck, so no PostgreSQL is needed to observe the failure." That's no longer true -- an earlier review round on this PR added an explicit `check-stale-expected: installcheck` dependency edge so it runs AFTER pg_regress, which the file's own "runs after pg_regress, not before" test already verifies. The header now matches. Standard-suite @test count is unchanged (128) -- just relocated. Full suite (test-all) passes clean: 233/233. Changes only in pgxntool-test. No related changes in pgxntool. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
3 times, most recently
from
July 27, 2026 22:14
9dcb404 to
8290f85
Compare
…logic tests Add tests/updates for pgxntool commit 0fe9dfc (pass PGXNTOOL_CHECK_EXPECTED_FILE_TYPES to check-stale-expected.sh as an arg instead of an env var; add PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT so tests can stub out just the script path without faking the whole PGXNTOOL_DIR tree): - check-stale-expected.sh now takes the file-type check toggle as a second positional argument (default yes) rather than reading it from the environment; base.mk passes the make variable through as that arg. - New PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT variable names exactly which script the check-stale-expected recipe invokes (default $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh). - Add helpers.bash:make_stub_script, a general-purpose helper that writes a throwaway stub script (caller-chosen filename under BATS_TEST_TMPDIR, configurable exit code, stdout text, marker file) to prove a script was/wasn't invoked, or that its exit status/output was correctly propagated -- not specific to check-stale-expected.sh. Writes directly to BATS_TEST_TMPDIR/<filename> rather than mktemp, since bats-core already gives each test a fresh, unique tmpdir. - Split check-stale-expected's pure script-logic tests (orphan detection, the pg_regress _N.out alternate-file convention, the build/expected pair, the file-type sub-check, and argument validation) out of make-test.bats into check-stale-expected-script.bats, invoking the script directly against a bare scratch directory -- no foundation, make, or Postgres needed for those. - make-test.bats now owns only Makefile-integration concerns and no longer duplicates script decision logic: pg_regress ordering (dry-run), that the recipe passes the right positional args (dry-run), that PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no genuinely skips invoking the script (stub-based), that `make` correctly propagates a stub's exit status/output either way (stub-based, replacing two tests that duplicated the real script's decision logic), and one real end-to-end `make test` failure on a stale file. - Same layering fix applied to the EXTRA_CLEAN/PGXS `clean` test: replaced a real mkdir+touch+`make clean`+assert-deleted test (re-verifying PGXS's own established `clean`/EXTRA_CLEAN deletion mechanics) with a `make -n clean` dry-run check that test/results/ actually appears in the real clean recipe -- proving pgxntool's EXTRA_CLEAN entry percolates into the recipe without needing to invoke or verify PGXS's own mechanics. - Fix assert_contains/assert_not_contains in test/lib/assertions.bash to show the haystack/needle on failure instead of a bare exit status, matching the diagnostic quality assert_success/assert_failure already meet (gap found while researching bats-assert as prior art for the stubbing/layering work above; see pgxntool-test#44 for that research). - Generalize CLAUDE.md's layered-testing principle beyond the check-stale-expected script/Makefile split: don't re-test an underlying framework's own established behavior (PGXS's clean/EXTRA_CLEAN mechanics is a second concrete instance) -- only test that pgxntool's own code correctly wires into/configures it. Trimmed the inline "Layering" essay in make-test.bats to a one-line pointer at this CLAUDE.md section, and led the ordering test's comment with a plain-language summary line. Co-Authored-By: Claude <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 27, 2026 22:46
8290f85 to
48961fa
Compare
jnasbyupgrade
added a commit
to Postgres-Extensions/pgxntool
that referenced
this pull request
Jul 27, 2026
…checks (#61) - #19: versioned SQL file generation used `>` truncate then `>>` append across two recipe lines; a second firing (e.g. two overlapping `make` processes) doubled the file's content. Now a single atomic redirect. - #14: `make test` never caught a stale `test/expected/*.out` left behind after a test was renamed/removed. Added `check-stale-expected`, tolerant of pg_regress's `_N.out` alternate-output files, and made it depend on `installcheck` directly so it runs after pg_regress, not before. - #53: tagging targets hardcoded the `origin` remote. Now configurable via `PGXN_REMOTE ?= origin`. - #28: `EXTENSION_VERSION_FILES` renamed to `EXTENSION__CURRENT_VERSION__FILES` -- old name implied it held all version files; it only ever holds the current one per extension. - #50: `base.mk` had no include guard; double-inclusion redefined every target with warnings. Wrapped in `ifndef`/`endif`. - #7: `make clean`'s `EXTRA_CLEAN` targeted a nonexistent top-level `results/` instead of `$(TESTOUT)/results/`. Adds a `Configuration Variables` section to README.asc documenting all override variables, and HISTORY.asc entries for the user-visible changes above. Related pgxntool-test PR: Postgres-Extensions/pgxntool-test#31 Fixes #7, #14, #19, #28, #50, #53. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Related pgxntool PR: Postgres-Extensions/pgxntool#61
New BATS coverage in
test/standard/:versioned-sql-race.bats(Fix results ordering, control file whitespace, ENABLE_* overrides #19): races two genuinely concurrentmake allinvocations against the same versioned-SQL target across 5 iterations, asserting the resulting file is never doubled. Verified by hand first that the old two-line recipe reproduces doubling (~1/3 of trials); the fixed single-redirect recipe never doubled across 15+ manual trials.check-stale-expected.bats(Make branch alignment check bidirectional #14): orphaned-file failure (both the standalone target and viamake testbefore PostgreSQL is needed), the pg_regress alternate-output-file (_N.out) false-positive exemption plus a case proving the exemption doesn't over-apply when the alternate's own base.sqlis also missing, and the same coverage for thetest/build/test/build/expectedpair.base-mk-fixes.bats(Add tests for test-install, test-build, verify-results #7, ci: run full test-all suite instead of test #50, #53):EXTRA_CLEANtargets the realtest/results/directory andmake cleanactually removes it;PGXN_REMOTEdefaults tooriginand is overridable for bothtagandrmtag; a throwaway Makefile that includespgxntool/base.mktwice (directly and via an intermediate module) parses with no overriding-recipe warnings.#28 (variable rename) deliberately has no new test — pure mechanical rename, no behavior change, already exercised by the existing suite's
DATA/build-output coverage.Full suite passes 228/228, 0 skips.