Skip to content

feat(worker): set MERGED state on successful uploads after merge #746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thomasrockhu-codecov wants to merge 1 commit into from
Closed

feat(worker): set MERGED state on successful uploads after merge #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/worker/services/processing/merging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ def update_uploads(

if result["successful"]:
update = {
"state_id": UploadState.PROCESSED.db_id,
"state": "processed",
"state_id": UploadState.MERGED.db_id,
"state": "merged",
}
report = reports.get(upload_id)
Comment on lines +132 to 135
Copy link
Contributor

@sentry sentry bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@thomasrockhu-codecov thomasrockhu-codecov Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: Test assertion updated to expect state='merged' and state_id=MERGED, matching the new update_uploads behavior.

if report is not None:
Expand Down
68 changes: 68 additions & 0 deletions apps/worker/services/tests/test_merging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import pytest

from database.tests.factories.core import (
CommitFactory,
ReportFactory,
RepositoryFactory,
UploadFactory,
)
from services.processing.merging import update_uploads
from services.processing.types import MergeResult, ProcessingResult
from shared.reports.enums import UploadState
from shared.yaml import UserYaml


@pytest.mark.django_db(databases={"default"})
class TestUpdateUploadsState:
def test_successful_uploads_set_to_merged(self, dbsession):
repository = RepositoryFactory.create()
commit = CommitFactory.create(repository=repository)
report = ReportFactory.create(commit=commit)
upload = UploadFactory.create(
report=report,
state="started",
state_id=UploadState.UPLOADED.db_id,
)
dbsession.add_all([repository, commit, report, upload])
dbsession.flush()

processing_results: list[ProcessingResult] = [
{"upload_id": upload.id_, "successful": True, "arguments": {}},
]
merge_result = MergeResult(
session_mapping={upload.id_: 0}, deleted_sessions=set()
)

update_uploads(dbsession, UserYaml({}), processing_results, [], merge_result)

dbsession.refresh(upload)
assert upload.state_id == UploadState.MERGED.db_id
assert upload.state == "merged"

def test_failed_uploads_set_to_error(self, dbsession):
repository = RepositoryFactory.create()
commit = CommitFactory.create(repository=repository)
report = ReportFactory.create(commit=commit)
upload = UploadFactory.create(
report=report,
state="started",
state_id=UploadState.UPLOADED.db_id,
)
dbsession.add_all([repository, commit, report, upload])
dbsession.flush()

processing_results: list[ProcessingResult] = [
{
"upload_id": upload.id_,
"successful": False,
"arguments": {},
"error": {"code": "report_empty", "params": {}},
},
]
merge_result = MergeResult(session_mapping={}, deleted_sessions=set())

update_uploads(dbsession, UserYaml({}), processing_results, [], merge_result)

dbsession.refresh(upload)
assert upload.state_id == UploadState.ERROR.db_id
assert upload.state == "error"
8 changes: 4 additions & 4 deletions apps/worker/tasks/tests/unit/test_upload_finisher_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,11 +1252,11 @@ def mock_process_reports(
.first()
)
assert updated_upload is not None, "Upload should exist"
assert updated_upload.state == "processed", (
f"Upload state should be 'processed', got '{updated_upload.state}'"
assert updated_upload.state == "merged", (
f"Upload state should be 'merged', got '{updated_upload.state}'"
)
assert updated_upload.state_id == UploadState.PROCESSED.db_id, (
f"Upload state_id should be {UploadState.PROCESSED.db_id}, got {updated_upload.state_id}"
assert updated_upload.state_id == UploadState.MERGED.db_id, (
f"Upload state_id should be {UploadState.MERGED.db_id}, got {updated_upload.state_id}"
)

mock_process = mocker.patch.object(
Expand Down
Loading