test(nextly): fail integration tests that leave a transaction aborted - #412
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds PostgreSQL aborted-transaction detection, shared sighting storage, adapter transaction instrumentation, centralized test failure reporting, and public testing helpers with unit and integration coverage. ChangesAborted-transaction guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestCallback
participant AdapterTransaction
participant SavepointProbe
participant SightingsBuffer
participant AfterEachHook
TestCallback->>AdapterTransaction: run transaction callback
AdapterTransaction->>SavepointProbe: probe with savepoint
SavepointProbe->>SightingsBuffer: record aborted transaction
AfterEachHook->>SightingsBuffer: describe and consume sightings
AfterEachHook-->>TestCallback: fail test when diagnosis exists
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@codex please review this PR |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a95008866
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a48f7a61fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`current transaction is aborted` is never the real error. It is PostgreSQL reporting that an earlier statement inside the transaction already failed and was swallowed, so everything after it fails too. The shape that produces it is an existence check written as "run a query and catch the failure" — valid on SQLite and MySQL, fatal on PostgreSQL, and therefore invisible to a suite that passes on the first two. That class has been found repeatedly by reading code and never once by the tests, because the symptom surfaces far from its cause and no individual test knows to look for it. The harness now records any sighting and the shared setup asserts after EVERY test, so the failure is attributed to the test that caused it rather than to whichever happens to run last. Shipped with a test that induces the state deliberately — swallow a query against a missing relation inside a transaction, then issue another — and asserts the guard saw it. A guard that has never been observed failing is only a green check, and this one is verified by construction rather than by whether it happens to catch a bug someone already knows about. The test consumes its own sighting, which is both the assertion and the cleanup: the shared hook reads the same buffer and would otherwise fail it for demonstrating exactly what it demonstrates. PostgreSQL only, since the other two dialects do not poison a transaction on a failed statement, which is the whole reason this is needed.
`src/__tests__/setup.ts` is the setup file for BOTH vitest configs, so importing the integration harness there loaded the DI registry, the adapters and the event bus into every unit test. Seven schema-pipeline and database suites failed on CI as a result, while the integration legs stayed green locally — the import was only harmful where the harness was never meant to go. The sightings buffer moves to its own dependency-free module, which both the setup file and the harness import. The assertion still runs for every test; it just no longer drags the harness with it.
The guard only saw aborts that propagated out of `transaction()`. A callback that catches its own failure and returns normally never produces one: PostgreSQL accepts the COMMIT, downgrades it to a rollback, and the call resolves over a transaction that kept nothing. The bulk write paths do exactly this for per-item errors while `stopOnError` is false, so the swallow-and- continue class the guard targets could stay green. Probe the transaction context once the callback resolves, so the aborted state itself answers rather than the error that caused it. Report through `expect.fail` instead of a bare `Error`.
a48f7a6 to
b16a7d7
Compare
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b16a7d73cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The buffer lived in module scope, and there is more than one instance of the module: a suite importing the harness through `nextly/testing` gets the copy bundled into `dist/testing.mjs`, while the shared setup reads the source file. An abort recorded in one array was invisible to the assertion reading the other, so the guard failed open and reported green. Hold it on `globalThis`, which is how `init/schema-snapshot-cache.ts` already handles the same problem. Detect the abort by SQLSTATE `25P02` rather than only by its English text, which PostgreSQL translates according to `lc_messages`. The adapter preserves the driver's code on the errors it classifies, and the cause chain is walked because the code can sit on either the wrapper or the original.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d18157ff42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Publish the recording through `nextly/testing`. `createTestNextly` reaches plugin authors via `@nextlyhq/plugin-sdk/testing`, where the recording had no reader, so a swallowed abort still reported green. `describeAbortedTransactions` returns the message instead of throwing: shipped code cannot import vitest, and a NextlyError would replace the sightings with its generic public message. Both this package's setup file and a consumer's now share one buffer and one diagnosis, each asserting with its own runner. Install the wrapper once per adapter. An adapter handed to `createTestNextly` across boots was wrapped again each time, so one abort reported once per boot. Propagate probe failures that are not the abort signature. A `statement_timeout` from the `timeoutMs` option would otherwise be swallowed after the probe had already aborted an untouched transaction, which is the silent discard this guards against. Drop the changeset: this only instruments the test harness.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b941f165e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`nextly/testing` ships this module, so a raw statement in it puts SQL where product code is not allowed to keep it. An aborted transaction rejects `SAVEPOINT` with the same code it rejects a query by, so the adapter's own savepoint method answers the question and the string goes away. Released immediately when the transaction is healthy, which leaves it as the callback left it. Forward the readers through `@nextlyhq/plugin-sdk/testing` as well. That is the surface plugin authors are pointed at, and until now the recording had no import there, so a swallowed abort still reported green. The error matcher stays behind: a plugin classifying driver errors by hand means something else has gone wrong.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
The class of bug this catches
current transaction is aborted, commands ignored until end of transaction blockis never the real error. It is PostgreSQL reporting that an earlier statement inside the transaction already failed and was swallowed — so every statement after it fails too, far from the cause.The shape that produces it is an existence check written as "run a query and catch the failure":
Perfectly valid on SQLite and MySQL. On PostgreSQL, inside a transaction, it poisons the connection — the probe catches its own error and reports "absent", and the write that follows dies.
A suite can be green on two dialects and quietly broken on the third. During #382 that shape was found four times by reading code and zero times by the tests.
What this adds
transactionand records any aborted-transaction error before it propagates.src/__tests__/setup.tsasserts after every test, so the failure is attributed to the test that caused it rather than to whichever runs last.It ships with its own proof
A guard nobody has seen fail is only a green check.
aborted-transaction-guard.integration.test.tsinduces the state deliberately — swallow a query against a missing relation inside a transaction, then issue another — and asserts the guard recorded it, plus a companion case asserting silence when nothing aborts.The test consumes its own sighting, which is both the assertion and the cleanup: the shared
afterEachreads the same buffer and would otherwise fail it for demonstrating exactly what it set out to demonstrate.PostgreSQL only — the other two dialects do not poison a transaction on a failed statement, which is the whole reason this is needed.
Verification
3-dialect on freshly recreated databases: sqlite 768, postgres17 888, mysql 832 — all
exit=0.check-typesandlintclean.Notably the guard finds nothing on current
main, which is the honest result: the one known instance (the read-path companion join) has no test exercising it, since that reproduction was removed from #382 when it could not pass. So this lands green and starts catching regressions from here, and the follow-up that fixes the read path will land its reproduction against a suite that can already see the failure.Summary by CodeRabbit
Bug Fixes
New Features
Tests