server: Add end-to-end item submission integration test and harness#488
Open
julietshen wants to merge 4 commits into
Open
server: Add end-to-end item submission integration test and harness#488julietshen wants to merge 4 commits into
julietshen wants to merge 4 commits into
Conversation
Adds a real-infra integration test harness under server/test/integ/ that boots the IoC container, starts the express app, and runs the ItemProcessingWorker inline so submissions flow end-to-end without mocks. - setupIntegrationServer.ts — harness - wait.ts — poll helpers for Scylla and ClickHouse - items-submission.integ.test.ts — first scenario (#339) - README.md — how to run, layout, conventions Server typecheck passes. Not yet validated against a running stack. CI workflow to be wired in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three corrections from running against the real docker-compose stack:
- setupIntegrationServer: import dotenv/config so the IoC container sees
env vars. test:integ does not preload dotenv like server:start does.
- setupIntegrationServer: swallow the benign "Connection is closed"
error during shutdown. BullMQ's Worker.close() closes the shared
ioredis connection, then closeSharedResourcesForShutdown errors when
it tries to quit() it again.
- wait: switch the ClickHouse query from {name:Type} substitutions to ?
placeholders + positional binds, matching coop's formatClickhouseQuery
convention.
Also drops three unnecessary optional chains in afterAll that ESLint
flagged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
julietshen
requested review from
cassidyjames,
dom-notion,
juanmrad and
vinaysrao1
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds integration-test infrastructure: a real-server test harness with background item-processing worker and shutdown, polling helpers for eventual-consistency checks against Scylla and ClickHouse, an integration-test README, and an end-to-end item-submission integration test. ChangesIntegration Test Infrastructure
Sequence DiagramssequenceDiagram
participant Test
participant makeIntegrationServer
participant IoC as IoC Container
participant Server
participant ItemProcessingWorker
participant Cleanup as closeSharedResources
Test->>makeIntegrationServer: call()
makeIntegrationServer->>IoC: initialize dependencies
makeIntegrationServer->>Server: create and bind
makeIntegrationServer->>ItemProcessingWorker: start background task
makeIntegrationServer-->>Test: IntegrationServer{deps, request, shutdown}
Test->>Server: make requests via supertest agent
Server-->>Test: responses
Test->>IntegrationServer: shutdown()
IntegrationServer->>ItemProcessingWorker: signal abort
IntegrationServer->>ItemProcessingWorker: await shutdown
IntegrationServer->>Server: await shutdown
IntegrationServer->>Cleanup: call to release resources
Cleanup-->>IntegrationServer: cleanup complete
sequenceDiagram
participant Test
participant waitFor
participant Check as check_callback
participant Timeout
Test->>waitFor: call with check and timeout
loop Until result or deadline
waitFor->>Check: invoke
Check-->>waitFor: null/undefined or value
alt Value returned
waitFor-->>Test: return value
else Null/undefined
waitFor->>waitFor: sleep intervalMs
end
end
alt Deadline exceeded
waitFor->>Timeout: throw timeout Error labeled with what
Timeout-->>Test: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/test/integ/items-submission.integ.test.ts`:
- Around line 24-27: The teardown callbacks orgCleanup and itemTypeCleanup (and
any similar vars like harness/afterAll usage) can be undefined if beforeAll
fails; update the cleanup logic so afterAll only calls them when they exist
(e.g., check if typeof orgCleanup === "function" or if orgCleanup is truthy
before awaiting), or initialize them to a no-op async function at declaration;
ensure the afterAll block guards each call to orgCleanup() and itemTypeCleanup()
to avoid masking beforeAll failures.
In `@server/test/integ/setupIntegrationServer.ts`:
- Around line 43-57: The shutdown() method should be made best-effort so one
thrown error doesn't stop later teardown steps; wrap each async teardown
(workerAbort.abort(), deps.ItemProcessingWorker.shutdown(), shutdownServer(),
deps.closeSharedResourcesForShutdown()) in its own try/catch (or use
Promise.allSettled) to ensure all teardown calls run, log or collect errors
instead of rethrowing immediately, and still handle the known benign redis error
from deps.closeSharedResourcesForShutdown() as currently done; update the
shutdown() implementation to perform each step regardless of earlier failures
and surface a combined/logged result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf5b4443-9a6a-427d-9301-92863cda8ed5
📒 Files selected for processing (4)
server/test/integ/README.mdserver/test/integ/items-submission.integ.test.tsserver/test/integ/setupIntegrationServer.tsserver/test/integ/wait.ts
cassidyjames
changed the title
juanmrad
requested changes
May 19, 2026
Comment on lines
+34
to
+51
| export async function waitForItemInScylla( | ||
| deps: Pick<Dependencies, 'ItemInvestigationService'>, | ||
| opts: { | ||
| orgId: string; | ||
| itemIdentifier: ItemIdentifier; | ||
| timeoutMs?: number; | ||
| }, | ||
| ) { | ||
| return waitFor( | ||
| `item ${opts.itemIdentifier.id} in Scylla`, | ||
| async () => | ||
| deps.ItemInvestigationService.getItemByIdentifier({ | ||
| orgId: opts.orgId, | ||
| itemIdentifier: opts.itemIdentifier, | ||
| }), | ||
| { timeoutMs: opts.timeoutMs }, | ||
| ); | ||
| } |
Member
There was a problem hiding this comment.
Sadly due to how we had to get things working we have a fallback and ignore scylla errors. Given scylla data is ephemeral and only up to 6 months.
If we want a true e2e integration test we need to query scylla directly to avoid fallback showing a false positive.
export async function waitForItemInScylla(
deps: Pick<Dependencies, 'Scylla'>,
opts: { orgId: string; itemIdentifier: ItemIdentifier; timeoutMs?: number },
) {
return waitFor(
`item ${opts.itemIdentifier.id} in Scylla item_submission_by_thread`,
async () => {
const rows = await deps.Scylla
.select({
from: 'item_submission_by_thread',
select: '*',
where: [['item_identifier', '=', itemIdentifierToScyllaItemIdentifier(opts.itemIdentifier)]],
})
.catch(() => []);
return rows.length > 0 ? rows[0] : null;
},
{ timeoutMs: opts.timeoutMs },
);
}
Something like this would be best.
Member
Author
There was a problem hiding this comment.
gotcha, going to add that into waitForItemInScylla
- waitForItemInScylla now queries Scylla directly via deps.Scylla.select rather than ItemInvestigationService.getItemByIdentifier. The service falls back to the partial-items endpoint and the data warehouse when Scylla returns nothing, which would let a real Scylla write failure pass the test silently. (juanmrad) - Cleanup callbacks in items-submission.integ.test.ts are now optional and guarded with optional chaining + try/finally so a beforeAll failure surfaces the root cause instead of an "X is not a function" in afterAll. (juanmrad, coderabbit) - makeIntegrationServer().shutdown() is now best-effort: each teardown step runs through a runStep helper, errors are collected, and the combined failures are surfaced via AggregateError so one bad step doesn't leak the server or shared resources. (coderabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/test/integ/wait.ts`:
- Around line 41-67: The query in waitForItemInScylla omits the orgId, causing
wrong partitions to be queried; update deps.Scylla.select (inside
waitForItemInScylla) to include an additional WHERE predicate for 'org_id' =
opts.orgId alongside the existing 'item_identifier' predicate (use the same
itemIdentifierToScyllaItemIdentifier(opts.itemIdentifier) value), so the select
filters by both org_id and item_identifier and returns rows only from the
correct partition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77c929bf-5ef5-4f13-abbb-da4d25c9f227
📒 Files selected for processing (3)
server/test/integ/items-submission.integ.test.tsserver/test/integ/setupIntegrationServer.tsserver/test/integ/wait.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/test/integ/items-submission.integ.test.ts
- server/test/integ/setupIntegrationServer.ts
waitForItemInScylla had a .catch(() => []) that turned any Scylla ResponseError into a 30s waitFor timeout — masking structural problems (bad query, schema drift) as flaky tests. cassandra-driver returns empty rows for "no match", not an exception, so the catch was never buying us "not yet" semantics anyway; it was only ever hiding bugs. Document why we don't add org_id to the WHERE clause: partition key is (org_id, synthetic_thread_id), so a partial-PK restriction would need ALLOW FILTERING. The secondary index on item_identifier is what the production lookup path uses, and the test already asserts org_id on the returned row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 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
Implements #339 (sub-issue of #288). Adds a real-infra integration test harness under
server/test/integ/and the first scenario: end-to-end item submission landing in Scylla and ClickHouse.The harness is intentionally generic — the four remaining #288 sub-issues (#340 rule change, #341 report flow, #342 background jobs, #343 outage) can build on top of it without rebuilding the bootstrap.
What's new:
setupIntegrationServer.ts— boots the real IoC container, starts the express app, and runsItemProcessingWorkerinline; exposes{ deps, request, shutdown }.wait.ts— polling helpers (waitForItemInScylla,waitForItemInClickHouse, genericwaitFor).items-submission.integ.test.ts— the first scenario:POST /api/v1/items/async→ row in Scyllaitem_submission_by_threadand ClickHouseanalytics.CONTENT_API_REQUESTS.README.md— how to run, layout, conventions.Reuses existing
server/test/fixtureHelpers/(createOrg,createContentItemTypes).Findings worth flagging (also posted on #339):
test:integdoesn't preloaddotenvlikeserver:startdoes — the harness importsdotenv/configso the IoC container sees env vars.?placeholders + positional binds (formatClickhouseQueryconvention), not the native{name:Type}substitution.Worker.close()closes the shared ioredis connection;closeSharedResourcesForShutdownthen errors trying toquit()it again. Harness swallows that one specific error — a cleaner fix lives upstream (idempotentquitorsharedConnection: truefor BullMQ workers).Not in this PR (filed as follow-ups):
npm run test:integ. Local cycle works today vianpm run up && npm run db:update && npm run test:integ.fixtureHelpers/andwait.ts.Tests
The test itself is the test. Locally:
Output:
Reviewer checks:
npm run up && npm run db:update -- --env stagingsucceeds (Node ≥22 required — db-migrator usesfs/promises'sglobexport)(cd server && npm run test:integ)reports 1 passing test(cd server && npm run lint)— no new errors; 3 lint warnings in the integ test file were cleaned up before push(cd server && npx tsc --noEmit)— exit 0(Optional) Rollout Plan
None — test-only change. No production code touched, no schema changes, no deps added. Test does not run in CI yet (follow-up issue to wire it in).
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests