test(cron): cover the control-log retention trim end-to-end against real Postgres - #330
Merged
Merged
Conversation
…eal 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.
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit fd78421 on Jul 1, 2026 11:15am UTC. |
Contributor
There was a problem hiding this comment.
Pull request overview
Extracts the Cron job’s raw SQL retention-trim for shocker_control_logs into a shared OpenShockContext extension method and adds an integration test that executes the real statement against Postgres to catch schema/query drift in CI instead of at Cron runtime.
Changes:
- Moved the retention-trim
DELETESQL intoShockerControlLogQueries.DeleteControlLogsBeyondPerUserLimitAsync(...)for reuse by both the Cron job and tests. - Updated
ClearOldShockerControlLogsto call the shared query helper (no intended behavior change). - Added an end-to-end Postgres-backed integration test that seeds control logs and asserts the newest N are retained.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Cron/Jobs/ClearOldShockerControlLogs.cs | Switches Cron job from inline SQL to shared query helper. |
| Common/OpenShockDb/ShockerControlLogQueries.cs | Introduces the shared, parameterized retention-trim SQL extension method. |
| Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs | Adds Postgres integration test executing the shared retention-trim statement end-to-end. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+64
to
+67
| Id = logId, | ||
| ShockerId = shockerId, | ||
| ControlledByUserId = userId, | ||
| Intensity = 50, |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
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
Mirrors the outbox claim-query extraction (#328) for the other piece of raw SQL in the Cron jobs — the
ClearOldShockerControlLogsretention trim.DELETEintoCommon/OpenShockDb/ShockerControlLogQueries.cs→DeleteControlLogsBeyondPerUserLimitAsync(this OpenShockContext, int maxPerUser), a single source of truth the job and tests share. Parameterized by the per-user cap so tests can drive it with a small limit.ClearOldShockerControlLogsnow calls that extension withHardLimits.MaxShockerControlLogsPerUser. No behavior change (identical SQL).Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs(beside the other Cron delivery tests): seeds a user → device → shocker → logs graph and runs the actual statement against a Testcontainers Postgres — exercising the real table/column names, theshocker → device → ownerjoin, the per-user window function, and newest-first ordering. Asserts the newest N survive and the older rows are deleted.Why
Same rationale as #328: inline raw SQL means a table/column rename only surfaces in the Cron host at runtime. Extracted and covered, a schema drift fails the test instead. The test lives in
Cron.IntegrationTestsbecause it exercises a Cron job's query, resolving the pooledOpenShockContextfrom the Cron host's own DI.Notes