Skip to content

feat(pgsql-test): PgTestClient.expectError for expected failures; skills use it - #1925

Merged
pyramation merged 1 commit into
mainfrom
feat/pgsql-test-expect-error
Oct 23, 2026
Merged

pyramation merged 1 commit into
mainfrom
feat/pgsql-test-expect-error

Conversation

@pyramation

Copy link
Copy Markdown
Contributor

Summary

pgsql-test only offered the savepoint()/rollback() primitives, and the agent skills taught a three-line savepoint → expect().rejects → rollback sequence pasted at every call site. Agents therefore kept writing their own expectRejects/expectRefused/expectRaise wrappers, some with raw SAVEPOINT SQL and no finally. This PR adds the helper to the library and points every skill at it.

await db.expectError(() => db.query(`INSERT ...`), /permission denied/);
await db.expectError(() => db.query(`INSERT ...`), { code: '23505' });
const err = await db.expectError(() => db.query(`SELECT app.f(-1)`)); // resolves to the error

PgTestClient.expectError(run, expected?):

  • expected is a message substring, a RegExp, or { code?: string; message?: string | RegExp }, where code is the SQLSTATE.
  • It runs SAVEPOINT → run() → always (in finally) 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.
  • It throws [pgsql-test] expectError: expected X, but the statement succeeded, or expected X, got <code>: <message> with cause set to the original error.
  • It doesn't depend on a test framework (no Jest expect), so the same call works under Jest, Vitest and Mocha.

Skills and docs changed:

  • constructive-testing/references/pgsql-test-exceptions.md is rewritten around expectError. 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, because ROLLBACK TO keeps the savepoint.
  • pgpm/SKILL.md: the troubleshooting row for "transaction aborted" pointed at beforeEach/afterEach, which is the per-test pattern, not the expected-failure one. It now points at expectError.
  • Examples: the RLS, pgsql-test, supabase-test and drizzle-orm-test references, plus pgpm's testing.md and troubleshooting.md, now use expectError.
  • Rules: the root AGENTS.md and postgres/pgsql-test/AGENTS.md add an anti-pattern rule: no hand-rolled SAVEPOINT/ROLLBACK TO and no local wrapper. The pgsql-test README gains an "Expected failures" section.

Tests: postgres-test.expect-error.test.ts has 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

…lls use it instead of hand-rolled savepoints
@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@tenki-reviewer

tenki-reviewer Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Review complete. 🟡 1 medium

💬 Inline comments (1)

  • 🟡 Guard expectError cleanup so it cannot mask the caught error — test-client.ts:144
🧹 Nitpicks (1) — 🟢 1 low
  • 🟢 Reset lastIndex for stateful /g matchers in matchesText (test-client.ts:47) — matchesText calls pattern.test(text) directly on the caller-supplied RegExp (postgres/pgsql-test/src/test-client.ts:47-48).

This PR introduces a new expectError API on PgTestClient so tests can assert that a statement fails with a specific SQLSTATE code, message, or regex, without aborting the surrounding test transaction. The helper wraps the callback in a named SAVEPOINT, catches the thrown error, matches it against the expected shape, and unwinds with ROLLBACK TO SAVEPOINT plus RELEASE so the client stays usable afterward. Documentation across the monorepo skills, READMEs, and AGENTS.md is updated to teach the new pattern and deprecate hand-rolled savepoint/expectRejects helpers. A dedicated test file covers the new matcher semantics.

Files Change
postgres/pgsql-test/src/test-client.ts Adds expectError with savepoint-based cleanup and code/message/regex matcher support.
postgres/pgsql-test/__tests__/postgres-test.expect-error.test.ts New tests covering matcher variants and post-error transaction reuse.
postgres/pgsql-test/README.md, postgres/pgsql-test/AGENTS.md, AGENTS.md Documents the expectError helper and the anti-pattern it replaces.
.agents/skills/**, .agents/skills/pgpm/** Updates testing/troubleshooting skill references to use expectError.

Reviewed commit: ddd29b5

@tenki-reviewer tenki-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 expectError cleanup so it cannot mask the caught error — test-client.ts:144

Comment on lines +144 to +147
} finally {
await this.rollback(EXPECT_ERROR_SAVEPOINT);
await this.client.query(`RELEASE SAVEPOINT "${EXPECT_ERROR_SAVEPOINT}";`);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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.

@pyramation
pyramation merged commit 9997fdf into main Oct 23, 2026
20 checks passed
@pyramation
pyramation deleted the feat/pgsql-test-expect-error branch October 23, 2026 02:20
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