feat(worker): set MERGED state on successful uploads after merge#746
feat(worker): set MERGED state on successful uploads after merge#746thomasrockhu-codecov wants to merge 1 commit intotomhu/finisher-passes-db-sessionfrom
Conversation
960f041 to
af9c86e
Compare
debeee6 to
90240f9
Compare
af9c86e to
270a3e2
Compare
| "state_id": UploadState.MERGED.db_id, | ||
| "state": "merged", | ||
| } | ||
| report = reports.get(upload_id) |
There was a problem hiding this comment.
Bug: A unit test was not updated after a change in update_uploads, causing assertions to fail by expecting state "processed" instead of the new "merged" state.
Severity: MEDIUM
Suggested Fix
Update the assertions in the test test_coverage_notifications_not_blocked_by_test_results_uploads to expect the "merged" state and the corresponding UploadState.MERGED.db_id for successful uploads, aligning the test with the new behavior of the update_uploads function.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/worker/services/processing/merging.py#L132-L135
Potential issue: The function `update_uploads` was modified to set the state of
successful uploads to `"merged"` instead of `"processed"`. However, the unit test
`test_coverage_notifications_not_blocked_by_test_results_uploads` was not updated to
reflect this change. The test contains assertions that explicitly check for the old
`"processed"` state. As a result, when the test calls the real `update_uploads` function
via a mock, the assertions `assert updated_upload.state == "processed"` and `assert
updated_upload.state_id == UploadState.PROCESSED.db_id` will fail, breaking the CI
build.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Fixed: Test assertion updated to expect state='merged' and state_id=MERGED, matching the new update_uploads behavior.
90240f9 to
f87f322
Compare
270a3e2 to
b2ec191
Compare
f87f322 to
2204abd
Compare
b2ec191 to
e0f5114
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
e0f5114 to
7ed5926
Compare
2204abd to
3b5f4ba
Compare
7ed5926 to
948cf56
Compare
3b5f4ba to
9baf0ce
Compare
948cf56 to
b40cb67
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tomhu/finisher-passes-db-session #746 +/- ##
=================================================================
Coverage 92.21% 92.22%
=================================================================
Files 1304 1304
Lines 47933 47933
Branches 1628 1628
=================================================================
+ Hits 44203 44206 +3
+ Misses 3421 3418 -3
Partials 309 309
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9baf0ce to
ae13f48
Compare
b40cb67 to
e812cb9
Compare
Change update_uploads() to write state_id=MERGED, state="merged" for successful uploads instead of PROCESSED. This completes the semantic distinction: PROCESSED means "processor done, waiting for merge" while MERGED means "incorporated into the master report." Safe because the finisher's idempotency check already recognizes the "merged" state (done in the previous PR). Made-with: Cursor
e812cb9 to
b1fb518
Compare
Summary
update_uploads()inmerging.pyto writestate_id=MERGED (6), state="merged"for successful uploads instead ofstate_id=PROCESSED (2), state="processed".PROCESSEDnow means "processor done, waiting for merge" whileMERGEDmeans "incorporated into the master report."test_merging.pywith tests verifying successful uploads get MERGED state and failed uploads get ERROR state.Stacked on #745. Safe because the finisher's idempotency check already recognizes
"merged"state.Test plan
test_successful_uploads_set_to_mergedverifies MERGED statetest_failed_uploads_set_to_errorverifies ERROR state unchangedMade with Cursor
Note
Medium Risk
Changes the persisted final state for successful uploads from
PROCESSEDtoMERGED, which can affect downstream logic that keys off upload state during finishing/notification flows. Scope is small and covered by new/updated tests, but it touches core processing state transitions.Overview
Successful uploads updated via
update_uploads()are now persisted asMERGED(state_id/state) instead ofPROCESSED, aligning the DB state with “incorporated into the master report” semantics.Adds a focused unit test (
test_merging.py) asserting the new success state and existing error behavior, and updates the upload finisher unit test to expectmerged/UploadState.MERGEDafter the merge step.Written by Cursor Bugbot for commit b1fb518. This will update automatically on new commits. Configure here.