test(email): cover the outbox claim query end-to-end against real Postgres - #328
Merged
Conversation
…tgres Extract the delivery job's FOR UPDATE SKIP LOCKED claim query into EmailOutboxQueries.DueForDelivery as a single source of truth, and add an integration test that runs it through the same pooled OpenShockContext the Cron job uses. This exercises the real table/column names, the email_status enum mapping, LIMIT, the locking clause, and full entity materialization against a Testcontainers Postgres - so a schema rename or mapping regression fails the test rather than only surfacing in the Cron host at runtime. No behavior change to the job: identical SQL, server-side now(), same batch size.
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit 70acaf9 on Jul 1, 2026 11:02am UTC. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes the email outbox delivery job’s FOR UPDATE SKIP LOCKED claim SQL into a reusable query (EmailOutboxQueries.DueForDelivery) and adds an integration test that executes that exact SQL end-to-end against a real Postgres (via Testcontainers) using the same DI-resolved OpenShockContext path as the Cron host.
Changes:
- Extracted the outbox “due for delivery” claim SQL into
EmailOutboxQueries.DueForDelivery(batchSize)and introduced a sharedClaimBatchSizeconstant. - Updated
EmailOutboxDeliveryJobto use the extracted query as the single source of truth (no intended behavior change). - Added an integration test validating the query against the real schema + enum mapping using the pooled DI context.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Cron/Jobs/EmailOutboxDeliveryJob.cs |
Switches the Cron job to use the centralized claim query and shared batch size constant. |
Common/OpenShockDb/EmailOutboxQueries.cs |
New shared query helper encapsulating the raw claim SQL used by the delivery job. |
API.IntegrationTests/Tests/EmailOutboxPersistenceTests.cs |
Adds an end-to-end Postgres integration test that executes the job’s raw SQL via DI-resolved DbContext. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…lOutboxQueries The 'Apply suggestions from code review' commit added the batchSize guard but dropped the EmailOutboxQueries class's closing brace, breaking the build (CS1513: } expected). Restore it and fix the method indentation.
DueForDelivery is a Cron delivery-job query, so its end-to-end test belongs beside the other Cron delivery tests, not in API.IntegrationTests. Move it into a new EmailOutboxQueryTests in Cron.IntegrationTests, where it seeds via the Cron host's DbContext factory and resolves the pooled OpenShockContext from the Cron host's own DI - an even closer match to the production claim path than the API host was. The EmailOutboxMessage round-trip/claim-predicate persistence tests stay in the API suite. No coverage lost.
hhvrc
added a commit
that referenced
this pull request
Jul 1, 2026
…eal Postgres (#330) * test(cron): cover the control-log retention trim end-to-end against real Postgres Extract ClearOldShockerControlLogs' inline CTE DELETE into ShockerControlLogQueries.DeleteControlLogsBeyondPerUserLimitAsync (Common) as a single source of truth, parameterized by the per-user cap so tests can drive it with a small limit. The job now calls it with HardLimits.MaxShockerControlLogsPerUser. Add a Cron integration test (Cron.IntegrationTests, alongside the delivery tests) that seeds a user -> device -> shocker -> logs graph and runs the actual statement against a Testcontainers Postgres: it exercises the real table/column names, the shocker -> device -> owner join, the per-user window function, and newest-first ordering, asserting the newest N survive and the rest are deleted. A schema rename now fails the test rather than only the Cron host at runtime. Mirrors the outbox claim-query extraction (#328). No behavior change to the job. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(cron): make control-log trim test prove per-owner partitioning Per Copilot review: the single-owner setup (where the log's controller equalled the device owner) couldn't tell PARTITION BY d.owner_id apart from a buggy partition on controlled_by_user_id or a dropped PARTITION BY. Strengthen it: seed a second owner under the limit whose (older) logs must all survive, and attribute the over-limit owner's logs to two distinct controller users (neither over the limit). Now the assertions fail if the query ranks globally (would delete 10 and wipe the under-limit owner's logs) or partitions by controller (would delete 0) - only a correct per-owner trim deletes exactly the owner's oldest 5. Keeps the await-using async scope from the prior autofix commit. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
What
Extracts the email outbox delivery job's
FOR UPDATE SKIP LOCKEDclaim query intoEmailOutboxQueries.DueForDeliveryas a single source of truth, and adds an integration test that runs it end-to-end against a Testcontainers Postgres through the same pooledOpenShockContextthe Cron job uses.Why
The raw claim SQL binds the table/column names, the
email_statusenum, and the locking clause as text at runtime, so a schema rename (or an enum-mapping regression) would only surface in the Cron host in production. This test exercises that exact query — table/column names,email_statusenum mapping,LIMIT,FOR UPDATE SKIP LOCKED, and fullSELECT *entity materialization — so drift fails CI instead.Changes
Common/OpenShockDb/EmailOutboxQueries.cs(new) —DueForDelivery(batchSize)extension: the claim query as the single source of truth, plusClaimBatchSize.Cron/Jobs/EmailOutboxDeliveryJob.cs— callsDueForDelivery; no behavior change (identical SQL, server-sidenow(), same batch size).API.IntegrationTests/.../EmailOutboxPersistenceTests.cs— new test resolving the pooled context via DI (as the job does) and asserting the query selects a seeded due row.Testing
EmailOutboxPersistenceTests— 3/3 passing against Testcontainers Postgres.Notes
No DI/DbContext configuration changes. The originally-reported CRON
email_status=DbType=Objecterror did not reproduce against currentdevelop— the pooled-context path maps the enum correctly (now guarded by this test).