Repository navigation
feat(pgsql-test): PgTestClient.expectError for expected failures; skills use it - #1925
Conversation
…lls use it instead of hand-rolled savepoints
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
This PR introduces a new
Reviewed commit: ddd29b5 |
There was a problem hiding this comment.
Adds an expectError helper to the pgsql-test client that runs a callback inside a savepoint, catches expected DB errors, and restores the transaction via rollback-to-savepoint.
Key findings
- 🟡 Guard
expectErrorcleanup so it cannot mask the caught error — test-client.ts:144
| } finally { | ||
| await this.rollback(EXPECT_ERROR_SAVEPOINT); | ||
| await this.client.query(`RELEASE SAVEPOINT "${EXPECT_ERROR_SAVEPOINT}";`); | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Guard expectError cleanup so it cannot mask the caught error
In PgTestClient.expectError the finally block awaits this.rollback(EXPECT_ERROR_SAVEPOINT) and the RELEASE SAVEPOINT query with no error guard (postgres/pgsql-test/src/test-client.ts:144-147).
If either cleanup statement fails — e.g. run() committed, rolled back, or killed the shared connection, destroying the savepoint — that error replaces the original caught error, and the expected match/assertion logic at lines 149-155 never runs.
The test author then sees an opaque savepoint does not exist or connection error instead of the failure being verified, defeating expectError's diagnostic purpose.
📋 Prompt for AI Agents
In postgres/pgsql-test/src/test-client.ts, PgTestClient.expectError around lines 144-147: the finally block awaits this.rollback(EXPECT_ERROR_SAVEPOINT) and RELEASE SAVEPOINT "pgsql_test_expect_error" unguarded, so any cleanup failure (e.g. run() committed/rolled back the shared transaction, destroying the savepoint, or the connection died inside run()) replaces the original caught error and skips the expected-match checks. Wrap both cleanup statements in a try/catch that swallows or records the cleanup error (e.g. as caught.cleanupError), ensuring the code after the try/finally still evaluates failed and matchesExpected(caught, expected) and throws the diagnostic error with cause: caught.
Summary
pgsql-test only offered the
savepoint()/rollback()primitives, and the agent skills taught a three-linesavepoint → expect().rejects → rollbacksequence pasted at every call site. Agents therefore kept writing their ownexpectRejects/expectRefused/expectRaisewrappers, some with rawSAVEPOINTSQL and nofinally. This PR adds the helper to the library and points every skill at it.PgTestClient.expectError(run, expected?):expectedis a message substring, aRegExp, or{ code?: string; message?: string | RegExp }, wherecodeis the SQLSTATE.SAVEPOINT→run()→ always (infinally)ROLLBACK TO+RELEASE. The call therefore leaves nothing behind: you can repeat it, nest it under a caller's savepoint of the same name, and an unexpected success leaves no rows.[pgsql-test] expectError: expected X, but the statement succeeded, orexpected X, got <code>: <message>withcauseset to the original error.expect), so the same call works under Jest, Vitest and Mocha.Skills and docs changed:
constructive-testing/references/pgsql-test-exceptions.mdis rewritten aroundexpectError. The old version also stated two wrong rules: that savepoints can't be reused after a rollback, and that names must be unique. Both are false, becauseROLLBACK TOkeeps the savepoint.pgpm/SKILL.md: the troubleshooting row for "transaction aborted" pointed atbeforeEach/afterEach, which is the per-test pattern, not the expected-failure one. It now points atexpectError.testing.mdandtroubleshooting.md, now useexpectError.AGENTS.mdandpostgres/pgsql-test/AGENTS.mdadd an anti-pattern rule: no hand-rolledSAVEPOINT/ROLLBACK TOand no local wrapper. The pgsql-test README gains an "Expected failures" section.Tests:
postgres-test.expect-error.test.tshas 8 cases: the transaction stays usable, repeated calls, code/message matching, a successful statement fails the call and is rolled back, a mismatch fails the call and the transaction stays usable, nesting with a same-name caller savepoint, and transaction-local settings revert. The full pgsql-test suite passes locally (117 tests).Link to Devin session: https://app.devin.ai/sessions/fac9ca826fe6497ab29b806da19de770
Open in Devin Desktop: https://app.devin.ai/desktop/session/fac9ca826fe6497ab29b806da19de770?variant=devin
Requested by: @pyramation