Skip to content

Add test coverage for the test/install ON_ERROR_STOP rule #86

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jnasbyupgrade merged 6 commits into from
Oct 6, 2026
Merged

Add test coverage for the test/install ON_ERROR_STOP rule #86

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d43113
Add test coverage for order-sensitive ON_ERROR_STOP check
jnasbyupgrade Sep 16, 2026
6cd3308
Cover psql.sql-plus-turn-off failure in ON_ERROR_STOP check tests
jnasbyupgrade Oct 4, 2026
977c37f
Merge remote-tracking branch 'upstream/master' into fix-error-stop-or…
jnasbyupgrade Oct 4, 2026
4c99c47
Match check-test-install-error-stop tests to the any-mention rule
jnasbyupgrade Oct 6, 2026
8b2ff90
Merge remote-tracking branch 'upstream/master' into fix-error-stop-or…
jnasbyupgrade Oct 6, 2026
38a1fe8
Test that psql.sql actually makes psql stop on error
jnasbyupgrade Oct 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 72 additions & 5 deletions test/standard/check-test-install-error-stop-script.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
#
# These tests exercise test/bin/check-test-install-error-stop.sh directly
# against a bare scratch directory -- no foundation environment, no `make`,
# no PostgreSQL. They cover the script's own pass/fail logic: direct
# ON_ERROR_STOP, sourcing test/pgxntool/psql.sql, and files with neither.
# and no PostgreSQL except the one psql.sql test at the end. The rule: a file fails only if it neither includes
# test/pgxntool/psql.sql nor has any `\set`/`\unset ON_ERROR_STOP` command,
# whatever the value or order.
#
Comment on lines 6 to 10

@github-actions github-actions Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header's stated rule contradicts the PR description's stated rule — and the new tests encode the looser one.

This comment says a file passes if it "includes test/pgxntool/psql.sql" or "has any \set/\unset ON_ERROR_STOP command... whatever the value or order." But the PR description says the rule is stricter: a file must explicitly enable ON_ERROR_STOP (a later disable is then fine), or include psql.sql and never disable it — and explicitly lists "psql.sql plus \set ON_ERROR_STOP off: fails", "psql.sql plus \unset ON_ERROR_STOP: fails", and "off only: fails" as cases this PR covers.

The four new tests below assert the opposite of those three bullets:

  • pgxntool-test/test/standard/check-test-install-error-stop-script.bats

    Lines 45 to 52 in 8b2ff90

    @test "check-test-install-error-stop.sh: passes when a file sources psql.sql and turns ON_ERROR_STOP off" {
    printf '\\i test/pgxntool/psql.sql\nCREATE TABLE foo AS SELECT 1;\n\\set ON_ERROR_STOP OFF\n' > "$TESTDIR/install/foo.sql"
    run "$SCRIPT" "$TESTDIR"
    assert_success
    }
    — psql.sql + \set ON_ERROR_STOP OFF → asserts assert_success (PR description: "fails")

  • pgxntool-test/test/standard/check-test-install-error-stop-script.bats

    Lines 52 to 59 in 8b2ff90

    @test "check-test-install-error-stop.sh: passes when a file sources psql.sql via \\ir and unsets ON_ERROR_STOP" {
    printf '\\ir ../pgxntool/psql.sql\nCREATE TABLE foo AS SELECT 1;\n\\unset ON_ERROR_STOP\n' > "$TESTDIR/install/foo.sql"
    run "$SCRIPT" "$TESTDIR"
    assert_success
    }
    — psql.sql via \ir + \unset ON_ERROR_STOP → asserts assert_success (PR description: "fails")

  • pgxntool-test/test/standard/check-test-install-error-stop-script.bats

    Lines 59 to 66 in 8b2ff90

    @test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is only ever turned off" {
    printf '\\set ON_ERROR_STOP 0\nCREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"
    run "$SCRIPT" "$TESTDIR"
    assert_success
    }
    — \set ON_ERROR_STOP 0 only, no psql.sql → asserts assert_success (PR description: "off only: fails")

  • pgxntool-test/test/standard/check-test-install-error-stop-script.bats

    Lines 66 to 73 in 8b2ff90

    @test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is only ever unset" {
    printf '\\unset ON_ERROR_STOP\nCREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"
    run "$SCRIPT" "$TESTDIR"
    assert_success
    }
    — \unset ON_ERROR_STOP only, no psql.sql → same "off only" case, asserts assert_success

I traced handles_error_stop() in the paired check-test-install-error-stop.sh (pgxntool#122): it returns success on any \set/\unset ON_ERROR_STOP line regardless of value, or any psql.sql include line, with no order/disable tracking at all. So these four assertions are consistent with what the script actually does — but that actual behavior is the opposite of the "never disables it" / "explicitly enables" rule both this PR's description and pgxntool#122's description say is being implemented. As written, a test/install/*.sql file that only ever turns ON_ERROR_STOP off (or includes psql.sql and then disables it) passes this check — even though the check's entire purpose (per the comments a few lines below) is to guarantee a hard SQL error can't be silently swallowed.

Worth resolving before merge: either check-test-install-error-stop.sh is missing the disable-tracking logic its own PR description claims, and these four tests should assert failure instead — or the stricter rule described in both PR summaries was never the intended design, and the description/summary text (here and in pgxntool#122) needs correcting to match what's actually implemented and tested.

@jnasbyupgrade jnasbyupgrade Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude here: not changing this. The header and tests match the intended rule; the PR descriptions were stale when this review ran. Per Postgres-Extensions/pgxntool#122 (comment), a file fails only if it has no \set/\unset ON_ERROR_STOP command and does not include psql.sql, with no on-versus-off distinction. Both PR descriptions now state that rule.

# Tests that need real Make integration (TEST_DEPS wiring,
# PGXNTOOL_ENABLE_TEST_INSTALL_ERROR_STOP_CHECK=no skipping the target) stay
Expand Down Expand Up @@ -42,13 +43,69 @@ setup() {
assert_success
}

@test "check-test-install-error-stop.sh: fails when a file has neither" {
@test "check-test-install-error-stop.sh: passes when a file sources psql.sql and turns ON_ERROR_STOP off" {
printf '\\i test/pgxntool/psql.sql\nCREATE TABLE foo AS SELECT 1;\n\\set ON_ERROR_STOP OFF\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when a file sources psql.sql via \\ir and unsets ON_ERROR_STOP" {
printf '\\ir ../pgxntool/psql.sql\nCREATE TABLE foo AS SELECT 1;\n\\unset ON_ERROR_STOP\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is only ever turned off" {
printf '\\set ON_ERROR_STOP 0\nCREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is only ever unset" {
printf '\\unset ON_ERROR_STOP\nCREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is turned on then off" {
printf '\\set ON_ERROR_STOP on\nCREATE TABLE foo AS SELECT 1;\n\\set ON_ERROR_STOP off\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when ON_ERROR_STOP is turned off then on" {
printf '\\set ON_ERROR_STOP off\nCREATE TABLE foo AS SELECT 1;\n\\set ON_ERROR_STOP on\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: passes when the \\set is the last line with no trailing newline" {
printf 'CREATE TABLE foo AS SELECT 1;\n\\set ON_ERROR_STOP on' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_success
}

@test "check-test-install-error-stop.sh: fails when a file never mentions ON_ERROR_STOP" {
printf 'CREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_failure_with_status 1
assert_contains "$output" "foo.sql"
assert_contains "$output" "ON_ERROR_STOP"
assert_contains "$output" "foo.sql: never sets ON_ERROR_STOP"
}

@test "check-test-install-error-stop.sh: fails when ON_ERROR_STOP appears only in a comment" {
printf -- '-- ON_ERROR_STOP is set by the caller\nCREATE TABLE foo AS SELECT 1;\n' > "$TESTDIR/install/foo.sql"

run "$SCRIPT" "$TESTDIR"
assert_failure_with_status 1
assert_contains "$output" "foo.sql: never sets ON_ERROR_STOP"
}

@test "check-test-install-error-stop.sh: reports every offending file, not just the first" {
Expand All @@ -74,3 +131,13 @@ setup() {
run "$SCRIPT" "$TESTDIR" extra
assert_failure
}

# Guards the assumption check-test-install-error-stop.sh makes when it accepts
# files that include psql.sql. -X keeps ~/.psqlrc from masking a regression.
@test "test/pgxntool/psql.sql makes psql stop on error" {
skip_if_no_postgres

run psql -X -f "$PGXNREPO/test/pgxntool/psql.sql" -c 'SELECT 1/0' -c '\echo SENTINEL'
assert_failure
[[ "$output" != *SENTINEL* ]]
}
Loading