Repository navigation
Clarify that test/install only prepares the test environment - #144
jnasbyupgrade wants to merge 2 commits into
Conversation
test/install output is not a test result: it is not compared against expected output or validated as TAP. A failure in it must abort the run, which relies on ON_ERROR_STOP (the static check requires each file to include test/pgxntool/psql.sql or set/unset ON_ERROR_STOP itself).
|
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| === test/install | ||
| Runs setup files before the main test suite within the same `pg_regress` invocation. This allows expensive one-time operations (like extension installation) to set up state that persists into the regular test files. | ||
| `test/install` only prepares the environment for your real tests; its output is not a test result. If a file in it fails, the run aborts, but short of a failure nothing about its output is checked: it is not compared against expected output and not validated as TAP, so don't write tests there (put them in `test/sql`). | ||
|
|
There was a problem hiding this comment.
Overstated claim: "If a file in it fails, the run aborts"
This reads as unconditional, but abort-on-failure only happens when ON_ERROR_STOP is explicitly turned on. Per the project's own static check (and the unchanged WARNING a few lines down), a file that does \unset ON_ERROR_STOP or \set ON_ERROR_STOP off still passes the check — in that case psql prints the error, keeps going, and the file "passes" regardless:
Lines 212 to 214 in d2e9236
So this new sentence contradicts the existing WARNING in the same section. Consider something like "If a file in it fails and has ON_ERROR_STOP on, the run aborts," or otherwise clarify that the check only requires an explicit, deliberate choice — which could be to turn it off.
The identical overstatement was also introduced in this PR's CLAUDE.md edit, immediately followed by the correct qualifier ("...whatever the value"):
Lines 209 to 211 in d2e9236
| === test/install | ||
| Runs setup files before the main test suite within the same `pg_regress` invocation. This allows expensive one-time operations (like extension installation) to set up state that persists into the regular test files. | ||
| `test/install` only prepares the environment for your real tests; its output is not a test result. If a file in it fails, the run aborts, but short of a failure nothing about its output is checked: it is not compared against expected output and not validated as TAP, so don't write tests there (put them in `test/sql`). | ||
|
|
There was a problem hiding this comment.
New guidance conflicts with the existing WARNING's suggestion
This sentence says "don't write tests there (put them in test/sql)", but the unchanged WARNING later in this same section still offers writing pgTap assertions inside the install file as a sanctioned alternative:
Lines 212 to 214 in d2e9236
("...If you need actual output comparison, put that logic in test/sql instead (or use pgTap assertions from within the install file itself).")
Read together, the doc now gives contradictory guidance about whether pgTap assertions belong in test/install files. Worth reconciling — either drop the WARNING's parenthetical, or soften the new sentence (e.g. "don't rely on output comparison there" rather than a blanket "don't write tests there").
| - **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** only sets up the environment for the real tests; its output is not a test result (not compared against expected output, not validated as TAP), but a failure in it must abort the run. It 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. | ||
|
|
There was a problem hiding this comment.
Same overstatement as in the README.asc edit
"but a failure in it must abort the run" is stated unconditionally, but the very next clause (unchanged by this PR) says the check accepts a file with \set/\unset ON_ERROR_STOP "whatever the value" — i.e. explicitly turning it off also passes the check, and in that case a failure would not abort the run. Same issue as the README.asc edit in this PR:
Lines 168 to 170 in d2e9236
Consider qualifying this, e.g. "...but a failure in it must abort the run if ON_ERROR_STOP is on."
1 participant
test/installexists only to set up the environment for the real tests; its output is not a test result (not compared against expected output, not validated as TAP). A failure in it must abort the run, which relies onON_ERROR_STOP: the static check requires eachtest/install/*.sqlfile to includetest/pgxntool/psql.sqlor set/unsetON_ERROR_STOPitself.Docs only:
README.asc(+ regeneratedREADME.html) andCLAUDE.md. No behavior change, so noHISTORY.ascentry.