Repository navigation
Test that base.mk recipe loops fail when any iteration fails - #108
Draft
jnasbyupgrade wants to merge 10 commits into
Draft
jnasbyupgrade wants to merge 10 commits into
jnasbyupgrade wants to merge 10 commits into
Conversation
Add a multi-extension test that makes pgtle.sh fail for the first extension only (a directory sits where its output file goes) and asserts that `make pgtle` itself exits non-zero. With the old `;`-joined recipe only the last extension's status counted, so this failure was masked. Covers Postgres-Extensions/pgxntool#140; needs the recipe fix in Postgres-Extensions/pgxntool#142. 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 |
Add two tests to test/standard/make-test.bats that make a `cp` into the expected-output directory fail by putting a directory where cp writes, which fails even as root. The `make results` test makes a copy other than the last one fail, and runs `make -o verify-results results` so only the copy loop runs. The `results-build` test runs the real target. Without the fix, both targets exit 0 after the failed copy. Covers Postgres-Extensions/pgxntool#140; needs the recipe fix in Postgres-Extensions/pgxntool#142.
make echoes results-build's unprefixed `$(MAKE) -C . test-build || ...` recipe line, and that text contains the message the assertion checked was absent. The `cp` error assertion already proves the copy loop ran.
jnasbyupgrade
changed the title
make-loops.bats fails whenever pgxntool's *.mk files have a shell loop or $(foreach) that test/lib/pgxntool-loops.txt doesn't list, or stop having one it lists. A recipe loop hides the failure of every iteration but the last unless each iteration ends in `|| exit 1`, so the failure message says to first write a test that makes a non-last iteration fail, and only then add the entry naming that test. The test also checks that every named test exists. Loops are keyed by file, enclosing context (rule target, variable, define body, or top level) and kind, never by line number or nearby text, so unrelated edits next to a loop don't trip it. test/lib/scan-make-loops.awk finds them by reading the source rather than `make -p`, which omits recipes in inactive conditional branches. A second test runs the scanner on a fixture holding the loop shapes it must find and the comment and prose shapes it must not count. The install-schedule and print-pgtle loops have no failure test yet and are listed as untested. Related to Postgres-Extensions/pgxntool#140.
A PATH-shadowed cat fails only on the first extension's pg_tle/<range>/<ext>.sql and passes every other call to the real cat, since the pgtle prerequisite runs pgtle.sh, which also uses cat. Without the `|| exit 1` after print-pgtle's per-extension cat, the loop goes on to the next extension and make exits 0. The print-pgtle entry in test/lib/pgxntool-loops.txt now names this test instead of being untested. Related to Postgres-Extensions/pgxntool#140.
make-loops.bats now also fails when the logical line containing a listed loop changes in anything but whitespace, so a loop can't lose its `|| exit 1`, gain a `-` prefix, or gain a command after `done` that replaces its exit status without someone rechecking it. Each entry in test/lib/pgxntool-loops.txt gains a fingerprint field: the cksum CRC of that line, with continuations joined and each run of blanks collapsed to one space. There is no check for `|| exit 1` or && itself; any change to the text trips the test. Loops are still matched by file, context and kind. A key with as many loops as entries but different fingerprints is reported as a changed loop, with the line as it is now and, in order, what to recheck before updating the fingerprint. The new-loop message also prints the fingerprint a new entry needs. scan-make-loops.awk prints the normalized line as a fourth field. New fixture tests check that the fingerprint ignores whitespace and indentation, changes when `|| exit 1` is removed from a continuation line or a `-` prefix is added, and ignores edits to the neighbouring recipe lines, and that the comparison reports a changed loop rather than a new and a stale one. Related to Postgres-Extensions/pgxntool#140.
scan-make-loops.awk reported text like `echo "while building foo"` or
`$(error bad value (for instance; see docs))` as a loop, and the
inventory had no legal entry for it. Outside define bodies, a
for/while/until at command position now counts only when a `do`
follows it in the searched text, after one of ;&)} or right after
`for NAME`: a recipe line, assignment value or rule line is one shell
command line, so a real loop's `do` is on it. Define bodies keep the
old rule, since one shell may run a whole define with `do` on a later
line.
Text that still matches but isn't a loop gets the new
`not-a-loop:<why>` coverage, which is rejected for a $(foreach). The
new-loop failure message lists it and make-time: as the only
exceptions to writing a failure test first, and is now the one place
the steps are written out; the headers of make-loops.bats and
pgxntool-loops.txt point to it.
The scanner fixture gains five such prose lines, which must not be
reported; four loop shapes, which must (`while ...; do ... < list`,
`until`, `for f do`, `while { ...; } do`); and a define body line with
"while" in an echo, which is reported because define bodies stay
strict.
Related to Postgres-Extensions/pgxntool#140.
"make generates versioned SQL files" now also checks that each pg_tle/<range>/ directory make pgtle creates holds both ext_alpha.sql and ext_beta.sql, and that it creates at least one. Fixes Postgres-Extensions#71.
The scanner fixture gains `while (false) do` and `for ((i=0;i<2;i++)) do`, which must be reported, so dropping `)` from the characters a loop's `do` may follow now fails it. The comment on that character class lists them rather than calling them separators. In multi-extension.bats, load_first_ext_and_pgtle_dir also sets pgtle_version, which the print-pgtle test passes to make instead of repeating 1.4.0, and that test's comment keeps only its ordering constraint. Related to Postgres-Extensions/pgxntool#140.
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.
Covers Postgres-Extensions/pgxntool#140. The fix itself is in Postgres-Extensions/pgxntool#142.
Adds regression tests for three
base.mktargets that ignored a failure partway through a loop. Each test forces the failure by putting a directory where a file would be written. Permission-based failures don't work when the tests run as root, but this one does.test/standard/multi-extension.bats, "make pgtle fails when pgtle.sh fails for a non-last extension": takes the first extension frommake print-_PGXNTOOL_EXTENSIONS, removespg_tle/, and puts a directory where pgtle.sh writes that extension's file in an existing range directory. Then it asserts thatmake pgtlefails. The old recipe joined the per-extensionpgtle.shcalls with;, so only the last extension's exit status counted.test/standard/make-test.bats, "make results fails when a copy other than the last one fails (pgxntool#140)": adds two straytest/results/*.outfiles and makescpfail for the first one, withcp: cannot overwrite directory ... with non-directory. Then it asserts thatmake resultsfails. It runsmake -o verify-results results, which skips the test run, so only the copy loop runs and no Postgres server is needed.test/standard/make-test.bats, "results-build fails when a copy fails (pgxntool#140)": the samecpfailure undertest/build/expected/, running the realmake results-build. That recipe's skipped-file check after the loop discarded the loop's status, so even a failed last copy was ignored.Verified locally: the two
make-test.batstests fail against pgxntool#142's previous head (00284c5, wheremakeexits 0 after printing thecperror) and pass with its current head. Thepgtletest passes against the&&recipe. It fails against the old;recipe (checked when the test was added).pgxntool#142 also fixes
print-pgtleand thetest/install/schedulerule. They have no test here because neither failure can be forced portably.print-pgtlealready rejects a missing or non-regular file beforecatruns, so only a read error would reach the ignored case. The schedule loop appends to the file the line before it has just created, so only a write error partway through the loop would.Merge order: merge Postgres-Extensions/pgxntool#142 first. These tests need the fixed recipes. If this PR merges first, CI breaks for unrelated pgxntool-test PRs that test against pgxntool master.