Skip to content

Commit

Permalink
Cache and move load_commit_diff out of Report lock
Browse files Browse the repository at this point in the history 
Moves the `load_commit_diff` portion out of `save_report_results`.

This way, the whole commit diff loading can be moved outside of the Report merge lock.
Additionally, the function was decorated with a cache, as the diff for a commit will never change.
  • Loading branch information
@Swatinem
Swatinem committed Oct 15, 2024
1 parent 3853a7d commit 14ee84b
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 202 deletions.
133 changes: 27 additions & 106 deletions tasks/tests/unit/test_upload_processing_task.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,11 @@
from shared.reports.enums import UploadState
from shared.reports.resources import Report, ReportFile, ReportLine, ReportTotals
from shared.storage.exceptions import FileNotInStorageError
from shared.torngit.exceptions import TorngitObjectNotFoundError
from shared.upload.constants import UploadErrorCode

from database.models import CommitReport, ReportDetails, UploadError
from database.tests.factories import CommitFactory, UploadFactory
from helpers.exceptions import (
ReportEmptyError,
ReportExpiredException,
RepositoryWithoutValidBotError,
)
from helpers.exceptions import ReportEmptyError, ReportExpiredException
from helpers.parallel import ParallelProcessing
from rollouts import USE_LABEL_INDEX_IN_REPORT_PROCESSING_BY_REPO_ID
from services.archive import ArchiveService
Expand All @@ -27,7 +22,7 @@
SessionAdjustmentResult,
UploadProcessingResult,
)
from tasks.upload_processor import UploadProcessorTask
from tasks.upload_processor import UploadProcessorTask, save_report_results

here = Path(__file__)

Expand Down Expand Up @@ -484,7 +479,8 @@ def test_upload_task_call_exception_within_individual_upload(
"parse_raw_report_from_storage",
return_value="ParsedRawReport()",
)
mocker.patch.object(UploadProcessorTask, "save_report_results")
mocker.patch("tasks.upload_processor.load_commit_diff")
mocker.patch("tasks.upload_processor.save_report_results")

mocked_post_process = mocker.patch.object(
UploadProcessorTask, "postprocess_raw_reports"
Expand Down Expand Up @@ -1009,7 +1005,7 @@ def test_upload_task_call_celeryerror(
assert commit.state == "pending"

def test_save_report_results_apply_diff_not_there(
self, mocker, mock_configuration, dbsession, mock_repo_provider, mock_storage
self, mocker, mock_configuration, dbsession, mock_storage
):
commit = CommitFactory.create(
message="",
Expand All @@ -1033,68 +1029,15 @@ def test_save_report_results_apply_diff_not_there(
report.append(report_file_1)
report.append(report_file_2)
chunks_archive_service = ArchiveService(commit.repository)
mock_repo_provider.get_commit_diff.side_effect = TorngitObjectNotFoundError(
"response", "message"
)
result = UploadProcessorTask().save_report_results(
db_session=dbsession,
report_service=ReportService({}),
repository=commit.repository,
commit=commit,
report=report,
pr=None,
)
expected_result = {
"url": f"v4/repos/{chunks_archive_service.storage_hash}/commits/{commit.commitid}/chunks.txt"
}
assert expected_result == result
assert report.diff_totals is None
result = save_report_results(ReportService({}), commit, report, None, None)

def test_save_report_results_apply_diff_no_bot(
self, mocker, mock_configuration, dbsession, mock_repo_provider, mock_storage
):
commit = CommitFactory.create(
message="",
repository__owner__unencrypted_oauth_token="testulk3d54rlhxkjyzomq2wh8b7np47xabcrkx8",
repository__owner__username="ThiagoCodecov",
repository__yaml={
"codecov": {"max_report_age": "1y ago"}
}, # Sorry for the timebomb
)
mock_get_repo_service = mocker.patch(
"tasks.upload_processor.get_repo_provider_service"
)
mock_get_repo_service.side_effect = RepositoryWithoutValidBotError()
dbsession.add(commit)
dbsession.flush()
report = Report()
report_file_1 = ReportFile("path/to/first.py")
report_file_2 = ReportFile("to/second/path.py")
report_line_1 = ReportLine.create(coverage=1, sessions=[[0, 1]])
report_line_2 = ReportLine.create(coverage=0, sessions=[[0, 0]])
report_line_3 = ReportLine.create(coverage=1, sessions=[[0, 1]])
report_file_1.append(10, report_line_1)
report_file_1.append(12, report_line_2)
report_file_2.append(12, report_line_3)
report.append(report_file_1)
report.append(report_file_2)
chunks_archive_service = ArchiveService(commit.repository)
result = UploadProcessorTask().save_report_results(
db_session=dbsession,
report_service=ReportService({}),
repository=commit.repository,
commit=commit,
report=report,
pr=None,
)
expected_result = {
assert result == {
"url": f"v4/repos/{chunks_archive_service.storage_hash}/commits/{commit.commitid}/chunks.txt"
}
assert expected_result == result
assert report.diff_totals is None

def test_save_report_results_apply_diff_valid(
self, mocker, mock_configuration, dbsession, mock_repo_provider, mock_storage
self, mocker, mock_configuration, dbsession, mock_storage
):
commit = CommitFactory.create(
message="",
Expand All @@ -1118,7 +1061,7 @@ def test_save_report_results_apply_diff_valid(
report.append(report_file_1)
report.append(report_file_2)
chunks_archive_service = ArchiveService(commit.repository)
f = {
diff = {
"files": {
"path/to/first.py": {
"type": "modified",
Expand All @@ -1139,20 +1082,11 @@ def test_save_report_results_apply_diff_valid(
}
}
}
mock_repo_provider.get_commit_diff.return_value = f
result = UploadProcessorTask().save_report_results(
db_session=dbsession,
report_service=ReportService({}),
commit=commit,
repository=commit.repository,
report=report,
pr=None,
)
expected_result = {
result = save_report_results(ReportService({}), commit, report, diff, None)
assert result == {
"url": f"v4/repos/{chunks_archive_service.storage_hash}/commits/{commit.commitid}/chunks.txt"
}
assert expected_result == result
expected_diff_totals = ReportTotals(
assert report.diff_totals == ReportTotals(
files=1,
lines=1,
hits=1,
Expand All @@ -1167,10 +1101,9 @@ def test_save_report_results_apply_diff_valid(
complexity_total=0,
diff=0,
)
assert report.diff_totals == expected_diff_totals

def test_save_report_results_empty_report(
self, mocker, mock_configuration, dbsession, mock_repo_provider, mock_storage
self, mocker, mock_configuration, dbsession, mock_storage
):
commit = CommitFactory.create(
message="",
Expand All @@ -1184,7 +1117,7 @@ def test_save_report_results_empty_report(
dbsession.flush()
report = Report()
chunks_archive_service = ArchiveService(commit.repository)
f = {
diff = {
"files": {
"path/to/first.py": {
"type": "modified",
Expand All @@ -1205,20 +1138,12 @@ def test_save_report_results_empty_report(
}
}
}
mock_repo_provider.get_commit_diff.return_value = f
result = UploadProcessorTask().save_report_results(
db_session=dbsession,
report_service=ReportService({}),
commit=commit,
repository=commit.repository,
report=report,
pr=None,
)
expected_result = {
result = save_report_results(ReportService({}), commit, report, diff, None)

assert result == {
"url": f"v4/repos/{chunks_archive_service.storage_hash}/commits/{commit.commitid}/chunks.txt"
}
assert expected_result == result
expected_diff_totals = ReportTotals(
assert report.diff_totals == ReportTotals(
files=0,
lines=0,
hits=0,
Expand All @@ -1233,7 +1158,6 @@ def test_save_report_results_empty_report(
complexity_total=None,
diff=0,
)
assert report.diff_totals == expected_diff_totals
assert commit.state == "error"

@pytest.mark.parametrize(
Expand All @@ -1244,7 +1168,6 @@ def test_save_report_results_pr_values(
mocker,
mock_configuration,
dbsession,
mock_repo_provider,
mock_storage,
pr_value,
expected_pr_result,
Expand All @@ -1253,17 +1176,15 @@ def test_save_report_results_pr_values(
dbsession.add(commit)
dbsession.flush()
report = mocker.Mock()
mock_repo_provider.get_commit_diff.return_value = {
"files": {"path/to/first.py": {}}
}
diff = {"files": {"path/to/first.py": {}}}
mock_report_service = mocker.Mock(save_report=mocker.Mock(return_value="aaaa"))
result = UploadProcessorTask().save_report_results(
db_session=dbsession,
report_service=mock_report_service,
commit=commit,
repository=commit.repository,
report=report,
pr=pr_value,
result = save_report_results(
mock_report_service,
commit,
report,
diff,
pr_value,
)
assert "aaaa" == result

assert result == "aaaa"
assert commit.pullid == expected_pr_result
22 changes: 9 additions & 13 deletions tasks/upload_finisher.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@
from tasks.base import BaseCodecovTask
from tasks.parallel_verification import parallel_verification_task
from tasks.upload_clean_labels_index import task_name as clean_labels_index_task_name
from tasks.upload_processor import UPLOAD_PROCESSING_LOCK_NAME, UploadProcessorTask
from tasks.upload_processor import (
UPLOAD_PROCESSING_LOCK_NAME,
load_commit_diff,
save_report_results,
)

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -153,6 +157,8 @@ def run_impl(
int(upload["upload_pk"])
for upload in processing_results["parallel_incremental_result"]
]
pr = processing_results["processings_so_far"][0]["arguments"].get("pr")
diff = load_commit_diff(commit, pr, self.name)

report_lock = (
acquire_report_lock(repoid, commitid, self.hard_time_limit_task)
Expand Down Expand Up @@ -182,18 +188,8 @@ def run_impl(
)

if parallel_processing is ParallelProcessing.PARALLEL:
pr = processing_results["processings_so_far"][0]["arguments"].get(
"pr"
)
processor_task = UploadProcessorTask()
processor_task.save_report_results(
db_session,
report_service,
repository,
commit,
report,
pr,
report_code,
save_report_results(
report_service, commit, report, diff, pr, report_code
)
state.mark_uploads_as_merged(upload_ids)

Expand Down
Loading

0 comments on commit 14ee84b

Please sign in to comment.