test: migrate DB-backed tests to transactional harness - #821
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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 |
taobojlen
force-pushed
the
test/migrate-db-tests-to-transactional-harness
branch
from
June 22, 2026 15:00
2e2aab7 to
8c728d5
Compare
taobojlen
changed the base branch from
main
to
test/harness-serialize-concurrent-queries
June 22, 2026 15:00
taobojlen
force-pushed
the
test/migrate-db-tests-to-transactional-harness
branch
from
June 22, 2026 15:20
8c728d5 to
fc381d8
Compare
taobojlen
force-pushed
the
test/harness-serialize-concurrent-queries
branch
from
June 22, 2026 15:54
a86cd70 to
2fad078
Compare
taobojlen
force-pushed
the
test/migrate-db-tests-to-transactional-harness
branch
from
June 22, 2026 15:55
fc381d8 to
4659da0
Compare
taobojlen
force-pushed
the
test/harness-serialize-concurrent-queries
branch
from
June 22, 2026 16:08
2fad078 to
6805c35
Compare
taobojlen
force-pushed
the
test/migrate-db-tests-to-transactional-harness
branch
from
June 22, 2026 16:09
4659da0 to
6c9bad7
Compare
taobojlen
force-pushed
the
test/harness-serialize-concurrent-queries
branch
4 times, most recently
from
June 29, 2026 16:19
e57b4b3 to
f6daa89
Compare
Base automatically changed from
test/harness-serialize-concurrent-queries
to
main
June 30, 2026 13:08
connect-pg-simple keeps a recurring pruneSessions timer that calls pool.query on its pool. makeApiServer created the store with the shared KyselyPgPool but never closed it, so the timer kept running after shutdown. In tests this leaked a setInterval per makeMockedServer() call that fired pool.query on the harness's already-closed pinned connection after each test, logging "Failed to prune sessions: Client was closed and is not queryable" indefinitely and hanging the test worker — the loop that forced the manual cancellation of CI run 27951870325. Hold a reference to the store instance and call its close() in the shutdown path. ownsPg is false (we pass in our own pool), so close() only stops the prune timer and won't end the shared pool. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Move the remaining DB-backed server tests onto the transaction-rollback harness from #732 so they get per-test isolation with no hand-written cleanup. - Convert the makeMockedServer/getBottle-based tests to makeTransactionalTestWithFixture, dropping manual org/user/queue/action deletes and KyselyPg.destroy() teardown: userKyselyPersistenceFindByEmailAndOrg, resolveSamlUser, and the MRT module tests (CommentOperations, JobRouting, QueueOperations, ReporterInvalidation). - Rewrite moderationConfigService and manualReviewToolService so every test is self-contained: each creates its own fresh org (rolled back automatically) rather than sharing a suite-scoped org. This removes the cross-test ordering/accumulator dependency in moderationConfigService and the hardcoded staging-seed-data dependency in manualReviewToolService. Scylla-touching tests (itemInvestigationService) keep uid-based isolation, since the Postgres-only harness can't roll those back. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…harness
Two DB-backed test suites were missed by the initial migration commit:
- manualReviewToolService/modules/UserReportSweep.test.ts: a sibling of
the four MRT module tests already migrated (CommentOperations, JobRouting,
QueueOperations, ReporterInvalidation). It used makeTestWithFixture +
getBottle with hand-written cleanup (createOrg/createUser/createMrtQueue
teardown + KyselyPg/KyselyPgReadReplica.destroy()). Converted to
makeTransactionalTestWithFixture; cleanup is now automatic rollback.
- userStrikeService/userStrikeService.test.ts: used beforeAll/afterAll with
a shared getBottle container and uid-based isolation. Converted to
makeTransactionalTestWithFixture for true per-test rollback isolation.
Also fixes a copy-paste describe block name ('Item Investigation Service'
-> 'User Strike Service').
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
taobojlen
force-pushed
the
test/migrate-db-tests-to-transactional-harness
branch
from
July 6, 2026 12:49
6c9bad7 to
ae78d94
Compare
Two files migrated by the prior commit had TypeScript errors that broke
`tsc` (and therefore `check_api_server` and the e2e server start):
- manualReviewToolService.test.ts: the 'records an AUTOMATIC_CLOSE decision
with no human reviewer' test was left as a bare `it(...)` referencing an
out-of-scope `mrtService` and hardcoded staging `orgId`/'queueId'
('e7c89ce7729'/'1') — the very seed-data dependency the migration was
meant to remove. Convert it to `testWithQueue()` like its siblings, using
the fixture's `mrtService`/`org.id`/`queue.id`.
- moderationConfigService.test.ts: the 'should return actions for a rule
scoped to the caller org' snapshot read `it.id` from
`getActionsForRuleId`, which returns `{ action, parameters }[]` —
should be `it.action.id` (as on main). Also add the now-present
`email` field role to the #createUserType inline snapshot.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The transactional JobRouting fixture accidentally replaced the pre-migration createUser() call with a bare uid(). createManualReviewQueue validates queue users, so setup failed after creating org/item-type rows but before the fixture returned. Because makeTestWithFixture cannot run cleanup when setup throws, the outer transaction stayed idle-in-transaction and later tests blocked on the item_type_versions materialized-view refresh, timing out check_api_server. Keep the transactional harness, but create a real user and pass user.id to the queue setup. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The rebase conflict resolution for QueueOperations.test.ts accidentally kept the pre-#872 branch side and dropped the two-org org-scoping regression tests that now exist on main. Restore testWithTwoOrgs and its cross-org access tests, using the transactional harness. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Replace the decision-reason test helpers' direct writes to manual_review_tool_settings with a behavior-shaped helper that uses the ManualReviewToolService update API. The tests still exercise persisted org settings through submitDecision, but no longer couple setup to table and column names. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Remove the comments added around the express-session store shutdown while keeping the shutdown behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
taobojlen
marked this pull request as ready for review
July 6, 2026 16:15
taobojlen
requested review from
cassidyjames,
dom-notion,
juanmrad,
julietshen and
vinaysrao1
as code owners
July 6, 2026 16:15
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
calebmcquaid
approved these changes
Jul 13, 2026
calebmcquaid
left a comment
calebmcquaid
left a comment
Contributor
There was a problem hiding this comment.
This is great, thank you!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context & Requests for Reviewers
This migrates the remaining tests to the new DB-transaction test harness that I added in #732.
This new helper (called
makeTransactionalTestWithFixture) runs tests inside a database transaction that's rolled back at the end of the test. This means that test authors no longer need to worry about cleaning up the database state afterwards (which was often forgotten already!).This leads to a) tests that are easier to write and b) better test hygiene, i.e. less chance of introducing flakiness or cross-test dependencies.
Important for reviewers: the diff looks pretty massive but it's mostly whitespace! Make sure to check "Hide whitespace" in GitHub's review UI.
In two test files, the moderationService + the MRT one, the diff is bigger because the diffing algorithm doesn't render the changes super cleanly. But the tests continue to assert the same things as before, just written in a cleaner way (and with some existing cross-test dependencies removed).
Tests
The entire PR 😉