Skip to content

Set ON_ERROR_STOP in test/install/load.sql - #54

Open
jnasbyupgrade wants to merge 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:on-error-stop
Open

jnasbyupgrade wants to merge 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:on-error-stop

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

test/install/load.sql now sets \set ON_ERROR_STOP on, so psql aborts (exit 3) at the first error in it and pg_regress reports not ok 1 - ../install/load (e.g. "1 of 3 tests failed") instead of the error passing silently. This makes errors visible; it does not add a gate.

No CI step fails on that not ok:

  • Fresh mode (pg-build-test, ci.yml:166) ends in make installcheck || status=$?; exit $status, and pgxntool/base.mk:314 has .IGNORE: installcheck, so it exits 0 even on a real diff. This step gates no pg_regress failure at all, in any test file.
  • make test-update (ci.yml:196) and existing mode (bin/test_existing, make test) go through pgxntool/base.mk:376, which fails only if regression.diffs exists. pg_regress can never diff load.sql (its expected and result paths coincide; see test/install/.gitignore), so an error there does not fail them either.

Also notes at the first \if that it needs psql 10+.

Checked on PG17: make test and existing-mode run-suite pass; with a bogus SELECT appended to the end of the file, the install step reports not ok and make test still exits 0.

🤖 Generated with Claude Code

Errors in this file were never detected: pg_regress resolves its expected
and result paths to the same file, so it can never diff.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7ce0da0e-5507-4393-8df1-b66f58b3d67e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 16s —— View job


Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

  • \set ON_ERROR_STOP on sits right after \set ECHO none, before any statement that could fail. That is the right place.
  • The change only adds a psql variable and comments. It doesn't alter behavior in the existing and fresh/update modes when nothing errors, and the PR's evidence (a bogus statement gives not ok) shows it works.
  • The \if comment says psql 10+ is safe because the floor is PG12. I didn't verify the PG12 floor against the repo's metadata.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 15s —— View job


Code review

No issues found in PR #54. I checked the diff for bugs and CLAUDE.md compliance.

  • \set ON_ERROR_STOP on sits right after \set ECHO none. That is before any SQL runs, so the DO blocks and the mode checks in the file are all covered.
  • The comment on the \if line says the PG12 floor makes psql 10+ safe. That matches .github/workflows/ci.yml, which sets the CI matrix floor at PG12.
  • The PR description is candid that this does not make make test exit nonzero, and that whether a not ok fails the CI job depends on pg-build-test. I did not check that either.

I did not run the tests.

@jnasbyupgrade

Copy link
Copy Markdown
Contributor Author

Claude here. Decisions needed from the maintainer before this merges:

  1. Accept parity, or add a real gate? This PR makes an error in test/install/load.sql visible (not ok 1 - ../install/load, psql exit 3), but make test still exits 0: pgxntool/base.mk:376 fails only when regression.diffs exists, and this file is compared against itself, so none is ever produced. pg_count_nulls' equivalent change behaves the same way. Options for a real gate: (a) a pgxntool change so make test propagates a pg_regress failure (upstream of this repo); (b) a CI step that runs test/install/load.sql with ON_ERROR_STOP or greps the TAP output for ^not ok; (c) a dedicated test whose expected file differs from its result file.
  2. Separate PR: move the fresh-mode CI leg to make test? Today it runs only pg-build-test (ci.yml:166), which ends in make installcheck; pgxntool/base.mk:314 marks installcheck as .IGNORE, so that step exits 0 even on a real test diff. The update-mode and existing-mode legs do gate on diffs. pg_count_nulls' fresh leg runs make test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant