Skip to content
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

Jump to bottom

Cache and move load_commit_diff out of Report lock #784

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Cache and move load_commit_diff out of Report lock #784

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
2 changes: 1 addition & 1 deletion 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 @@ -649,7 +649,7 @@ def test_retry_on_report_lock(self, dbsession, mock_redis):
dbsession,
[
{
"processings_so_far": [{"successful": True}],
"processings_so_far": [{"successful": True, "arguments": {}}],
"parallel_incremental_result": {"upload_pk": 1},
}
],
Expand Down
141 changes: 47 additions & 94 deletions tasks/tests/unit/test_upload_processing_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@
SessionAdjustmentResult,
UploadProcessingResult,
)
from tasks.upload_processor import UploadProcessorTask
from tasks.upload_processor import (
UploadProcessorTask,
load_commit_diff,
save_report_results,
)

here = Path(__file__)

Expand Down Expand Up @@ -484,7 +488,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 +1014,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 +1038,38 @@ 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 = {
result = save_report_results(ReportService({}), commit, report, None, None)

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_no_bot(
self, mocker, mock_configuration, dbsession, mock_repo_provider, mock_storage
def test_load_commit_diff_no_diff(
self, mocker, mock_configuration, dbsession, mock_repo_provider
):
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
commit = CommitFactory.create()
dbsession.add(commit)
dbsession.flush()
mock_repo_provider.get_commit_diff.side_effect = TorngitObjectNotFoundError(
"response", "message"
)
diff = load_commit_diff(commit, None, None)
assert diff is None

def test_load_commit_diff_no_bot(self, mocker, mock_configuration, dbsession):
commit = CommitFactory.create()
dbsession.add(commit)
dbsession.flush()
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 = {
"url": f"v4/repos/{chunks_archive_service.storage_hash}/commits/{commit.commitid}/chunks.txt"
}
assert expected_result == result
assert report.diff_totals is None
diff = load_commit_diff(commit, None, None)
assert diff 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 +1093,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 +1114,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 +1133,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 +1149,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 +1170,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 +1190,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 +1200,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 +1208,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
19 changes: 6 additions & 13 deletions tasks/upload_finisher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
from tasks.upload_processor import (
MAX_RETRIES,
UPLOAD_PROCESSING_LOCK_NAME,
UploadProcessorTask,
load_commit_diff,
save_report_results,
)

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -158,6 +159,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)

try:
report_lock = (
Expand Down Expand Up @@ -188,18 +191,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
Loading