From 78671e3d7b883b762207649f5583b2bfb6f5ad8a Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 14 Oct 2024 14:26:47 +0200 Subject: [PATCH] fix tests and more code removal related to partial sessions --- services/report/__init__.py | 16 +- .../languages/tests/unit/test_pycoverage.py | 42 -- services/report/raw_upload_processor.py | 25 +- services/report/tests/unit/test_process.py | 3 +- services/report/tests/unit/test_sessions.py | 542 +----------------- services/tests/test_report.py | 24 +- tasks/tests/unit/test_upload_finisher_task.py | 138 ----- .../tests/unit/test_upload_processing_task.py | 13 +- tasks/upload_finisher.py | 14 +- tasks/upload_processor.py | 2 +- 10 files changed, 49 insertions(+), 770 deletions(-) diff --git a/services/report/__init__.py b/services/report/__init__.py index 4d9d469bc..d21c34905 100644 --- a/services/report/__init__.py +++ b/services/report/__init__.py @@ -50,10 +50,7 @@ RAW_UPLOAD_RAW_REPORT_COUNT, RAW_UPLOAD_SIZE, ) -from services.report.raw_upload_processor import ( - SessionAdjustmentResult, - process_raw_upload, -) +from services.report.raw_upload_processor import process_raw_upload from services.repository import get_repo_provider_service from services.yaml.reader import get_paths_from_flags, read_yaml_field @@ -73,7 +70,7 @@ class ProcessingResult: session: Session report: Report | None = None error: ProcessingError | None = None - session_adjustment: SessionAdjustmentResult | None = None + deleted_sessions: set[int] | None = None @dataclass @@ -717,7 +714,7 @@ def build_report_from_raw_content( upload=upload, ) result.report = process_result.report - result.session_adjustment = process_result.session_adjustment + result.deleted_sessions = process_result.deleted_sessions log.info( "Successfully processed report", @@ -812,11 +809,10 @@ def update_upload_with_processing_result( # delete all the carryforwarded `Upload` records corresponding to `Session`s # which have been removed from the report. # we always have a `session_adjustment` in the non-error case. - assert processing_result.session_adjustment - deleted_sessions = ( - processing_result.session_adjustment.fully_deleted_sessions + assert processing_result.deleted_sessions is not None + delete_uploads_by_sessionid( + upload, list(processing_result.deleted_sessions) ) - delete_uploads_by_sessionid(upload, deleted_sessions) else: error = processing_result.error diff --git a/services/report/languages/tests/unit/test_pycoverage.py b/services/report/languages/tests/unit/test_pycoverage.py index 7455c7d25..ed1546b73 100644 --- a/services/report/languages/tests/unit/test_pycoverage.py +++ b/services/report/languages/tests/unit/test_pycoverage.py @@ -216,7 +216,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ) assert processed_report == { "archive": { @@ -228,7 +227,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 2, @@ -237,10 +235,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [ - (0, 1, None, ["test_another.py::test_fib_simple_case"]), - (0, 1, None, ["test_another.py::test_fib_bigger_cases"]), - ], ), ( 3, @@ -249,10 +243,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [ - (0, 1, None, ["test_another.py::test_fib_simple_case"]), - (0, 1, None, ["test_another.py::test_fib_bigger_cases"]), - ], ), ( 4, @@ -261,7 +251,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_another.py::test_fib_bigger_cases"])], ), ], "source.py": [ @@ -272,7 +261,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 3, @@ -281,7 +269,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 4, @@ -290,7 +277,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_source.py::test_some_code"])], ), ( 5, @@ -299,7 +285,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_source.py::test_some_code"])], ), ( 6, @@ -308,7 +293,6 @@ def test_process_pycoverage(self): [[0, 0, None, None, None]], None, None, - [], ), ( 9, @@ -317,7 +301,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 10, @@ -326,7 +309,6 @@ def test_process_pycoverage(self): [[0, 0, None, None, None]], None, None, - [], ), ], "test_another.py": [ @@ -337,7 +319,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 3, @@ -346,7 +327,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 4, @@ -355,7 +335,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_another.py::test_fib_simple_case"])], ), ( 5, @@ -364,7 +343,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_another.py::test_fib_simple_case"])], ), ( 7, @@ -373,7 +351,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 8, @@ -382,7 +359,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_another.py::test_fib_bigger_cases"])], ), ], "test_source.py": [ @@ -393,7 +369,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 4, @@ -402,7 +377,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 5, @@ -411,7 +385,6 @@ def test_process_pycoverage(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["test_source.py::test_some_code"])], ), ], }, @@ -488,7 +461,6 @@ def test_process_compressed_report(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["Th2dMtk4M_codecov"])], ), ( 2, @@ -497,10 +469,6 @@ def test_process_compressed_report(self): [[0, 1, None, None, None]], None, None, - [ - (0, 1, None, ["label_1"]), - (0, 1, None, ["label_2"]), - ], ), ( 3, @@ -509,10 +477,6 @@ def test_process_compressed_report(self): [[0, 1, None, None, None]], None, None, - [ - (0, 1, None, ["label_2"]), - (0, 1, None, ["label_3"]), - ], ), ( 4, @@ -521,7 +485,6 @@ def test_process_compressed_report(self): [[0, 0, None, None, None]], None, None, - [], ), ( 5, @@ -530,7 +493,6 @@ def test_process_compressed_report(self): [[0, 1, None, None, None]], None, None, - [(0, 1, None, ["label_5"])], ), ], "__init__.py": [ @@ -541,7 +503,6 @@ def test_process_compressed_report(self): [[0, 0, None, None, None]], None, None, - [], ), ( 3, @@ -550,7 +511,6 @@ def test_process_compressed_report(self): [[0, 0, None, None, None]], None, None, - [], ), ( 4, @@ -559,7 +519,6 @@ def test_process_compressed_report(self): [[0, 0, None, None, None]], None, None, - [], ), ( 5, @@ -568,7 +527,6 @@ def test_process_compressed_report(self): [[0, 0, None, None, None]], None, None, - [], ), ], }, diff --git a/services/report/raw_upload_processor.py b/services/report/raw_upload_processor.py index 3437152ec..382d676e4 100644 --- a/services/report/raw_upload_processor.py +++ b/services/report/raw_upload_processor.py @@ -17,16 +17,10 @@ log = logging.getLogger(__name__) -@dataclass -class SessionAdjustmentResult: - fully_deleted_sessions: list[int] - partially_deleted_sessions: list[int] - - @dataclass class UploadProcessingResult: report: Report # NOTE: this is just returning the input argument, and primarily used in tests - session_adjustment: SessionAdjustmentResult + deleted_sessions: set[int] @sentry_sdk.trace @@ -121,17 +115,16 @@ def process_raw_upload( # Because we know that the processing was successful _sessionid, session = report.add_session(session, use_id_from_session=True) # Adjust sessions removed carryforward sessions that are being replaced + deleted_sessions = set() if session.flags: - session_adjustment = clear_carryforward_sessions( + deleted_sessions = clear_carryforward_sessions( report, session.flags, commit_yaml ) - else: - session_adjustment = SessionAdjustmentResult([], []) report.merge(temporary_report, joined=joined) session.totals = temporary_report.totals - return UploadProcessingResult(report=report, session_adjustment=session_adjustment) + return UploadProcessingResult(report=report, deleted_sessions=deleted_sessions) @sentry_sdk.trace @@ -139,19 +132,19 @@ def clear_carryforward_sessions( original_report: Report, to_merge_flags: list[str], current_yaml: UserYaml, -) -> SessionAdjustmentResult: +) -> set[int]: + session_ids_to_fully_delete = set() to_fully_overwrite_flags = { f for f in to_merge_flags if current_yaml.flag_has_carryfoward(f) } - session_ids_to_fully_delete = [] if to_fully_overwrite_flags: for session_id, session in original_report.sessions.items(): if session.session_type == SessionType.carriedforward and session.flags: if any(f in to_fully_overwrite_flags for f in session.flags): - session_ids_to_fully_delete.append(session_id) + session_ids_to_fully_delete.add(session_id) if session_ids_to_fully_delete: - original_report.delete_multiple_sessions(session_ids_to_fully_delete) + original_report.delete_multiple_sessions(list(session_ids_to_fully_delete)) - return SessionAdjustmentResult(sorted(session_ids_to_fully_delete), []) + return session_ids_to_fully_delete diff --git a/services/report/tests/unit/test_process.py b/services/report/tests/unit/test_process.py index 172cf8931..0686d1778 100644 --- a/services/report/tests/unit/test_process.py +++ b/services/report/tests/unit/test_process.py @@ -1109,8 +1109,7 @@ def test_process_raw_upload_with_carryforwarded_flags(self): session=session, ) report = result.report - assert result.session_adjustment.fully_deleted_sessions == [1] - assert result.session_adjustment.partially_deleted_sessions == [] + assert result.deleted_sessions == {1} assert sorted(report.sessions.keys()) == [0, session.id] assert session.id == 2 assert report.sessions[session.id] == session diff --git a/services/report/tests/unit/test_sessions.py b/services/report/tests/unit/test_sessions.py index 2ec2a1254..99376c574 100644 --- a/services/report/tests/unit/test_sessions.py +++ b/services/report/tests/unit/test_sessions.py @@ -3,15 +3,9 @@ from shared.reports.resources import LineSession, ReportLine, Session, SessionType from shared.yaml import UserYaml -from services.report.raw_upload_processor import ( - SessionAdjustmentResult, - clear_carryforward_sessions, -) +from services.report.raw_upload_processor import clear_carryforward_sessions from test_utils.base import BaseTestCase -# Not calling add_sessions here on purpose, so it doesnt -# interfere with this logic - class TestAdjustSession(BaseTestCase): @pytest.fixture @@ -36,15 +30,16 @@ def sample_first_report(self): ) first_file = EditableReportFile("first_file.py") c = 0 - for sessionid in range(4): - first_file.append( - c % 7 + 1, - self.create_sample_line( - coverage=c, - sessionid=sessionid, - ), - ) - c += 1 + for _ in range(5): + for sessionid in range(4): + first_file.append( + c % 7 + 1, + self.create_sample_line( + coverage=c, + sessionid=sessionid, + ), + ) + c += 1 second_file = EditableReportFile("second_file.py") first_report.append(first_file) first_report.append(second_file) @@ -62,11 +57,6 @@ def sample_first_report(self): ], None, None, - [ - (0, 0, None, ["one_label"]), - (2, 14, None, ["another_label", "one_label"]), - (3, 7, None, ["another_label"]), - ], ), ( 2, @@ -79,12 +69,6 @@ def sample_first_report(self): ], None, None, - [ - (0, 8, None, ["another_label"]), - (0, 8, None, ["one_label"]), - (1, 1, None, ["one_label"]), - (3, 15, None, ["another_label", "one_label"]), - ], ), ( 3, @@ -97,12 +81,6 @@ def sample_first_report(self): ], None, None, - [ - (0, 16, None, ["Th2dMtk4M_codecov"]), - (1, 9, None, ["another_label"]), - (1, 9, None, ["one_label"]), - (2, 2, None, ["one_label"]), - ], ), ( 4, @@ -115,12 +93,6 @@ def sample_first_report(self): ], None, None, - [ - (1, 17, None, ["Th2dMtk4M_codecov"]), - (2, 10, None, ["another_label"]), - (2, 10, None, ["one_label"]), - (3, 3, None, ["one_label"]), - ], ), ( 5, @@ -133,12 +105,6 @@ def sample_first_report(self): ], None, None, - [ - (0, 4, None, ["another_label"]), - (2, 18, None, ["Th2dMtk4M_codecov"]), - (3, 11, None, ["another_label"]), - (3, 11, None, ["one_label"]), - ], ), ( 6, @@ -151,11 +117,6 @@ def sample_first_report(self): ], None, None, - [ - (0, 12, None, ["another_label", "one_label"]), - (1, 5, None, ["another_label"]), - (3, 19, None, ["Th2dMtk4M_codecov"]), - ], ), ( 7, @@ -164,10 +125,6 @@ def sample_first_report(self): [[2, 6, None, None, None], [1, 13, None, None, None]], None, None, - [ - (1, 13, None, ["another_label", "one_label"]), - (2, 6, None, ["another_label"]), - ], ), ] } @@ -182,9 +139,12 @@ def create_sample_line(self, *, coverage, sessionid=None): def test_adjust_sessions_no_cf(self, sample_first_report): first_value = self.convert_report_to_better_readable(sample_first_report) current_yaml = UserYaml({}) - assert clear_carryforward_sessions( - sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([], []) + assert ( + clear_carryforward_sessions( + sample_first_report, ["enterprise"], current_yaml + ) + == set() + ) assert first_value == self.convert_report_to_better_readable( sample_first_report ) @@ -199,7 +159,7 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): ) assert clear_carryforward_sessions( sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([0], []) + ) == {0} assert self.convert_report_to_better_readable(sample_first_report) == { "archive": { @@ -211,10 +171,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[3, 7, None, None, None], [2, 14, None, None, None]], None, None, - [ - (2, 14, None, ["another_label", "one_label"]), - (3, 7, None, ["another_label"]), - ], ), ( 2, @@ -223,10 +179,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[1, 1, None, None, None], [3, 15, None, None, None]], None, None, - [ - (1, 1, None, ["one_label"]), - (3, 15, None, ["another_label", "one_label"]), - ], ), ( 3, @@ -235,11 +187,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[2, 2, None, None, None], [1, 9, None, None, None]], None, None, - [ - (1, 9, None, ["another_label"]), - (1, 9, None, ["one_label"]), - (2, 2, None, ["one_label"]), - ], ), ( 4, @@ -252,12 +199,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): ], None, None, - [ - (1, 17, None, ["Th2dMtk4M_codecov"]), - (2, 10, None, ["another_label"]), - (2, 10, None, ["one_label"]), - (3, 3, None, ["one_label"]), - ], ), ( 5, @@ -266,11 +207,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[3, 11, None, None, None], [2, 18, None, None, None]], None, None, - [ - (2, 18, None, ["Th2dMtk4M_codecov"]), - (3, 11, None, ["another_label"]), - (3, 11, None, ["one_label"]), - ], ), ( 6, @@ -279,10 +215,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[1, 5, None, None, None], [3, 19, None, None, None]], None, None, - [ - (1, 5, None, ["another_label"]), - (3, 19, None, ["Th2dMtk4M_codecov"]), - ], ), ( 7, @@ -291,10 +223,6 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): [[2, 6, None, None, None], [1, 13, None, None, None]], None, None, - [ - (1, 13, None, ["another_label", "one_label"]), - (2, 6, None, ["another_label"]), - ], ), ] }, @@ -371,437 +299,3 @@ def test_adjust_sessions_full_cf_only(self, sample_first_report): "diff": None, }, } - - def test_adjust_sessions_partial_cf_only_no_changes( - self, sample_first_report, mocker - ): - current_yaml = UserYaml( - { - "flag_management": { - "individual_flags": [ - { - "name": "enterprise", - "carryforward_mode": "labels", - "carryforward": True, - } - ] - } - } - ) - first_value = self.convert_report_to_better_readable(sample_first_report) - assert clear_carryforward_sessions( - sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([], [0]) - after_result = self.convert_report_to_better_readable(sample_first_report) - assert after_result == first_value - - def test_adjust_sessions_partial_cf_only_no_changes_encoding_labels( - self, sample_first_report - ): - current_yaml = UserYaml( - { - "flag_management": { - "individual_flags": [ - { - "name": "enterprise", - "carryforward_mode": "labels", - "carryforward": True, - } - ] - } - } - ) - first_value = self.convert_report_to_better_readable(sample_first_report) - assert clear_carryforward_sessions( - sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([], [0]) - after_result = self.convert_report_to_better_readable(sample_first_report) - assert after_result == first_value - - def test_adjust_sessions_partial_cf_only_some_changes(self, sample_first_report): - current_yaml = UserYaml( - { - "flag_management": { - "individual_flags": [ - { - "name": "enterprise", - "carryforward_mode": "labels", - "carryforward": True, - } - ] - } - } - ) - assert clear_carryforward_sessions( - sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([], [0]) - - assert self.convert_report_to_better_readable(sample_first_report) == { - "archive": { - "first_file.py": [ - ( - 1, - 14, - None, - [[3, 7, None, None, None], [2, 14, None, None, None]], - None, - None, - [ - (2, 14, None, ["another_label", "one_label"]), - (3, 7, None, ["another_label"]), - ], - ), - ( - 2, - 15, - None, - [ - [1, 1, None, None, None], - [0, 8, None, None, None], - [3, 15, None, None, None], - ], - None, - None, - [ - (0, 8, None, ["another_label"]), - (1, 1, None, ["one_label"]), - (3, 15, None, ["another_label", "one_label"]), - ], - ), - ( - 3, - 16, - None, - [ - [2, 2, None, None, None], - [1, 9, None, None, None], - [0, 16, None, None, None], - ], - None, - None, - [ - (0, 16, None, ["Th2dMtk4M_codecov"]), - (1, 9, None, ["another_label"]), - (1, 9, None, ["one_label"]), - (2, 2, None, ["one_label"]), - ], - ), - ( - 4, - 17, - None, - [ - [3, 3, None, None, None], - [2, 10, None, None, None], - [1, 17, None, None, None], - ], - None, - None, - [ - (1, 17, None, ["Th2dMtk4M_codecov"]), - (2, 10, None, ["another_label"]), - (2, 10, None, ["one_label"]), - (3, 3, None, ["one_label"]), - ], - ), - ( - 5, - 18, - None, - [ - [0, 4, None, None, None], - [3, 11, None, None, None], - [2, 18, None, None, None], - ], - None, - None, - [ - (0, 4, None, ["another_label"]), - (2, 18, None, ["Th2dMtk4M_codecov"]), - (3, 11, None, ["another_label"]), - (3, 11, None, ["one_label"]), - ], - ), - ( - 6, - 19, - None, - [[1, 5, None, None, None], [3, 19, None, None, None]], - None, - None, - [ - (1, 5, None, ["another_label"]), - (3, 19, None, ["Th2dMtk4M_codecov"]), - ], - ), - ( - 7, - 13, - None, - [[2, 6, None, None, None], [1, 13, None, None, None]], - None, - None, - [ - (1, 13, None, ["another_label", "one_label"]), - (2, 6, None, ["another_label"]), - ], - ), - ] - }, - "report": { - "files": { - "first_file.py": [ - 0, - [0, 7, 7, 0, 0, "100", 0, 0, 0, 0, 0, 0, 0], - None, - None, - ] - }, - "sessions": { - "0": { - "t": None, - "d": None, - "a": None, - "f": ["enterprise"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "carriedforward", - "se": {}, - }, - "1": { - "t": None, - "d": None, - "a": None, - "f": ["enterprise"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "uploaded", - "se": {}, - }, - "2": { - "t": None, - "d": None, - "a": None, - "f": ["unit"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "carriedforward", - "se": {}, - }, - "3": { - "t": None, - "d": None, - "a": None, - "f": ["unrelated"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "uploaded", - "se": {}, - }, - }, - }, - "totals": { - "f": 1, - "n": 7, - "h": 7, - "m": 0, - "p": 0, - "c": "100", - "b": 0, - "d": 0, - "M": 0, - "s": 4, - "C": 0, - "N": 0, - "diff": None, - }, - } - - def test_adjust_sessions_partial_cf_only_full_deletion_due_to_lost_labels( - self, sample_first_report - ): - current_yaml = UserYaml( - { - "flag_management": { - "individual_flags": [ - { - "name": "enterprise", - "carryforward_mode": "labels", - "carryforward": True, - } - ] - } - } - ) - - assert clear_carryforward_sessions( - sample_first_report, ["enterprise"], current_yaml - ) == SessionAdjustmentResult([0], []) - res = self.convert_report_to_better_readable(sample_first_report) - - assert res["report"]["sessions"] == { - "1": { - "t": None, - "d": None, - "a": None, - "f": ["enterprise"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "uploaded", - "se": {}, - }, - "2": { - "t": None, - "d": None, - "a": None, - "f": ["unit"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "carriedforward", - "se": {}, - }, - "3": { - "t": None, - "d": None, - "a": None, - "f": ["unrelated"], - "c": None, - "n": None, - "N": None, - "j": None, - "u": None, - "p": None, - "e": None, - "st": "uploaded", - "se": {}, - }, - } - - assert self.convert_report_to_better_readable(sample_first_report)[ - "archive" - ] == { - "first_file.py": [ - ( - 1, - 14, - None, - [[3, 7, None, None, None], [2, 14, None, None, None]], - None, - None, - [ - (2, 14, None, ["another_label", "one_label"]), - (3, 7, None, ["another_label"]), - ], - ), - ( - 2, - 15, - None, - [[1, 1, None, None, None], [3, 15, None, None, None]], - None, - None, - [ - (1, 1, None, ["one_label"]), - (3, 15, None, ["another_label", "one_label"]), - ], - ), - ( - 3, - 9, - None, - [[2, 2, None, None, None], [1, 9, None, None, None]], - None, - None, - [ - (1, 9, None, ["another_label"]), - (1, 9, None, ["one_label"]), - (2, 2, None, ["one_label"]), - ], - ), - ( - 4, - 17, - None, - [ - [3, 3, None, None, None], - [2, 10, None, None, None], - [1, 17, None, None, None], - ], - None, - None, - [ - (1, 17, None, ["Th2dMtk4M_codecov"]), - (2, 10, None, ["another_label"]), - (2, 10, None, ["one_label"]), - (3, 3, None, ["one_label"]), - ], - ), - ( - 5, - 18, - None, - [[3, 11, None, None, None], [2, 18, None, None, None]], - None, - None, - [ - (2, 18, None, ["Th2dMtk4M_codecov"]), - (3, 11, None, ["another_label"]), - (3, 11, None, ["one_label"]), - ], - ), - ( - 6, - 19, - None, - [[1, 5, None, None, None], [3, 19, None, None, None]], - None, - None, - [ - (1, 5, None, ["another_label"]), - (3, 19, None, ["Th2dMtk4M_codecov"]), - ], - ), - ( - 7, - 13, - None, - [[2, 6, None, None, None], [1, 13, None, None, None]], - None, - None, - [ - (1, 13, None, ["another_label", "one_label"]), - (2, 6, None, ["another_label"]), - ], - ), - ] - } diff --git a/services/tests/test_report.py b/services/tests/test_report.py index 1999c64bd..51277e9a5 100644 --- a/services/tests/test_report.py +++ b/services/tests/test_report.py @@ -20,10 +20,7 @@ ReportService, ) from services.report import log as report_log -from services.report.raw_upload_processor import ( - SessionAdjustmentResult, - clear_carryforward_sessions, -) +from services.report.raw_upload_processor import clear_carryforward_sessions from test_utils.base import BaseTestCase @@ -1424,9 +1421,7 @@ def fake_possibly_shift(report, base, head): assert sorted(report.sessions.keys()) == [2, 3, 4] assert clear_carryforward_sessions( report, ["enterprise"], UserYaml(yaml_dict) - ) == SessionAdjustmentResult( - fully_deleted_sessions=[2, 3], partially_deleted_sessions=[] - ) + ) == {2, 3} assert sorted(report.sessions.keys()) == [4] readable_report = self.convert_report_to_better_readable(report) expected_results = { @@ -1496,9 +1491,7 @@ def test_get_existing_report_for_commit_already_carriedforward_add_sessions( assert sorted(report.sessions.keys()) == [0, 1, 2, 3, 4] assert clear_carryforward_sessions( report, ["enterprise"], UserYaml(yaml_dict) - ) == SessionAdjustmentResult( - fully_deleted_sessions=[2, 3], partially_deleted_sessions=[] - ) + ) == {2, 3} assert sorted(report.sessions.keys()) == [0, 1, 4] readable_report = self.convert_report_to_better_readable(report) expected_sessions_dict = { @@ -1570,10 +1563,8 @@ def test_get_existing_report_for_commit_already_carriedforward_add_sessions( second_to_merge_session = Session(flags=["unit"]) report.add_session(second_to_merge_session) assert sorted(report.sessions.keys()) == [0, 1, 3, 4] - assert clear_carryforward_sessions( - report, ["unit"], UserYaml(yaml_dict) - ) == SessionAdjustmentResult( - fully_deleted_sessions=[], partially_deleted_sessions=[] + assert ( + clear_carryforward_sessions(report, ["unit"], UserYaml(yaml_dict)) == set() ) assert sorted(report.sessions.keys()) == [0, 1, 3, 4] new_readable_report = self.convert_report_to_better_readable(report) @@ -3576,10 +3567,7 @@ def test_update_upload_with_processing_result_success(self, mocker, dbsession): dbsession.add(upload_obj) dbsession.flush() assert len(upload_obj.errors) == 0 - processing_result = ProcessingResult( - session=Session(), - session_adjustment=SessionAdjustmentResult([], []), - ) + processing_result = ProcessingResult(session=Session(), deleted_sessions={}) assert ( ReportService({}).update_upload_with_processing_result( upload_obj, processing_result diff --git a/tasks/tests/unit/test_upload_finisher_task.py b/tasks/tests/unit/test_upload_finisher_task.py index 32c311a18..4dd2df606 100644 --- a/tasks/tests/unit/test_upload_finisher_task.py +++ b/tasks/tests/unit/test_upload_finisher_task.py @@ -426,7 +426,6 @@ def test_finish_reports_processing_with_pull(self, dbsession, mocker): "app.tasks.notify.Notify": mocker.MagicMock(), "app.tasks.pulls.Sync": mocker.MagicMock(), "app.tasks.compute_comparison.ComputeComparison": mocker.MagicMock(), - "app.tasks.upload.UploadCleanLabelsIndex": mocker.MagicMock(), }, ) repository = RepositoryFactory.create( @@ -481,70 +480,6 @@ def test_finish_reports_processing_with_pull(self, dbsession, mocker): mocked_app.tasks[ "app.tasks.compute_comparison.ComputeComparison" ].apply_async.assert_called_once() - mocked_app.tasks[ - "app.tasks.upload.UploadCleanLabelsIndex" - ].apply_async.assert_not_called() - - def test_finish_reports_processing_call_clean_labels(self, dbsession, mocker): - commit_yaml = { - "flag_management": { - "individual_flags": [ - { - "name": "smart-tests", - "carryforward": True, - "carryforward_mode": "labels", - }, - { - "name": "just-tests", - "carryforward": True, - }, - ] - } - } - mocked_app = mocker.patch.object(UploadFinisherTask, "app") - commit = CommitFactory.create( - message="dsidsahdsahdsa", - commitid="abf6d4df662c47e32460020ab14abf9303581429", - repository__owner__unencrypted_oauth_token="testulk3d54rlhxkjyzomq2wh8b7np47xabcrkx8", - repository__owner__username="ThiagoCodecov", - repository__yaml=commit_yaml, - ) - processing_results = { - "processings_so_far": [ - {"successful": True, "arguments": {"flags": "smart-tests"}} - ] - } - dbsession.add(commit) - dbsession.flush() - - checkpoints = _create_checkpoint_logger(mocker) - res = UploadFinisherTask().finish_reports_processing( - dbsession, - commit, - UserYaml(commit_yaml), - processing_results, - None, - checkpoints, - ) - assert res == {"notifications_called": True} - mocked_app.tasks["app.tasks.notify.Notify"].apply_async.assert_any_call( - kwargs={ - "commitid": commit.commitid, - "current_yaml": commit_yaml, - "repoid": commit.repoid, - _kwargs_key(UploadFlow): ANY, - }, - ) - mocked_app.tasks[ - "app.tasks.upload.UploadCleanLabelsIndex" - ].apply_async.assert_called_with( - kwargs={ - "repoid": commit.repoid, - "commitid": commit.commitid, - "report_code": None, - }, - ) - assert mocked_app.send_task.call_count == 0 @pytest.mark.parametrize( "notify_error", @@ -630,76 +565,3 @@ def test_upload_finisher_task_calls_save_commit_measurements_task( "dataset_names": None, } ) - - -class TestShouldCleanLabelsIndex(object): - @pytest.mark.parametrize( - "processing_results, expected", - [ - ( - { - "processings_so_far": [ - {"successful": True, "arguments": {"flags": "smart-tests"}} - ] - }, - True, - ), - ( - { - "processings_so_far": [ - {"successful": True, "arguments": {"flags": "just-tests"}} - ] - }, - False, - ), - ( - { - "processings_so_far": [ - { - "successful": True, - "arguments": {"flags": "just-tests,smart-tests"}, - } - ] - }, - True, - ), - ( - { - "processings_so_far": [ - {"successful": False, "arguments": {"flags": "smart-tests"}} - ] - }, - False, - ), - ( - { - "processings_so_far": [ - {"successful": True, "arguments": {"flags": "just-tests"}}, - {"successful": True, "arguments": {"flags": "smart-tests"}}, - ] - }, - True, - ), - ], - ) - def test_should_clean_labels_index(self, processing_results, expected): - commit_yaml = UserYaml( - { - "flag_management": { - "individual_flags": [ - { - "name": "smart-tests", - "carryforward": True, - "carryforward_mode": "labels", - }, - { - "name": "just-tests", - "carryforward": True, - }, - ] - } - } - ) - task = UploadFinisherTask() - result = task.should_clean_labels_index(commit_yaml, processing_results) - assert result == expected diff --git a/tasks/tests/unit/test_upload_processing_task.py b/tasks/tests/unit/test_upload_processing_task.py index fe51da7af..96f9ad37c 100644 --- a/tasks/tests/unit/test_upload_processing_task.py +++ b/tasks/tests/unit/test_upload_processing_task.py @@ -22,10 +22,7 @@ from services.archive import ArchiveService from services.report import ProcessingError, RawReportInfo, ReportService from services.report.parser.legacy import LegacyReportParser -from services.report.raw_upload_processor import ( - SessionAdjustmentResult, - UploadProcessingResult, -) +from services.report.raw_upload_processor import UploadProcessingResult from tasks.upload_processor import UploadProcessorTask here = Path(__file__) @@ -574,9 +571,7 @@ def test_upload_task_call_with_expired_report( false_report_file.append(18, ReportLine.create(1, [])) false_report.append(false_report_file) mocked_2.side_effect = [ - UploadProcessingResult( - report=false_report, session_adjustment=SessionAdjustmentResult([], []) - ), + UploadProcessingResult(report=false_report, deleted_sessions={}), ReportExpiredException(), ] # Mocking retry to also raise the exception so we can see how it is called @@ -730,9 +725,7 @@ def test_upload_task_call_with_empty_report( false_report_file.append(18, ReportLine.create(1, [])) false_report.append(false_report_file) mocked_2.side_effect = [ - UploadProcessingResult( - report=false_report, session_adjustment=SessionAdjustmentResult([], []) - ), + UploadProcessingResult(report=false_report, deleted_sessions={}), ReportEmptyError(), ] mocker.patch.object(UploadProcessorTask, "app", celery_app) diff --git a/tasks/upload_finisher.py b/tasks/upload_finisher.py index 56439b069..9853eeb75 100644 --- a/tasks/upload_finisher.py +++ b/tasks/upload_finisher.py @@ -37,10 +37,7 @@ from services.processing.state import ProcessingState, should_trigger_postprocessing from services.redis import get_redis_connection from services.report import ReportService, delete_uploads_by_sessionid -from services.report.raw_upload_processor import ( - SessionAdjustmentResult, - clear_carryforward_sessions, -) +from services.report.raw_upload_processor import clear_carryforward_sessions from services.yaml import read_yaml_field from tasks.base import BaseCodecovTask from tasks.parallel_verification import parallel_verification_task @@ -585,9 +582,9 @@ def merge_report(cumulative_report: Report, obj): session, use_id_from_session=True ) - session_adjustment = SessionAdjustmentResult([], []) + deleted_sessions = set() if flags := session.flags: - session_adjustment = clear_carryforward_sessions( + deleted_sessions = clear_carryforward_sessions( cumulative_report, flags, UserYaml(commit_yaml) ) @@ -605,9 +602,8 @@ def merge_report(cumulative_report: Report, obj): upload.state_id = UploadState.PROCESSED.db_id upload.state = "processed" upload.order_number = new_sessionid - delete_uploads_by_sessionid( - upload, session_adjustment.fully_deleted_sessions - ) + if deleted_sessions: + delete_uploads_by_sessionid(upload, list(deleted_sessions)) db_session.flush() return cumulative_report diff --git a/tasks/upload_processor.py b/tasks/upload_processor.py index e5a2c888b..cba80e43f 100644 --- a/tasks/upload_processor.py +++ b/tasks/upload_processor.py @@ -43,7 +43,7 @@ def UPLOAD_PROCESSING_LOCK_NAME(repoid: int, commitid: str) -> str: Only a single processing task may possess this lock at a time, because merging reports requires exclusive access to the report. - This is used by the Upload, Notify and UploadCleanLabelsIndex tasks as well to + This is used by the Upload and Notify tasks as well to verify if an upload for the commit is currently being processed. """ return f"upload_processing_lock_{repoid}_{commitid}"