From 0ca663e52e115e6877282b02058cdb09868a440b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= Date: Wed, 26 Jul 2023 15:23:26 +0200 Subject: [PATCH] [fix] Refine when a report is regarded as outstanding for tags Despite only two lines of code, this patch is many things all in once: * We fix a bug where diffing tags returned unexpected (and percieved to be incorrect) results, * We redefine what we expect from diffing tags, * We redefine the expected heaviour around review status rules. Previously, we regarded review status rules are a timeless property, but what is even more true, we didn't really know what we expect from them when it came to diffing tags. This lead to confusion on the developer side and on the user side as well, and lead to whack-a-mole issues and patches like #3675, that was more driven by what users expected from this feature than a comprehensive plan. This is okay -- the review status feature and the tag feature grew in their own world, and nobody can be faulted for being on top of these features having a very solid specifications right out of the gate. This patch solved this issue. From this point on, our stance is the following: when we diff runs, we always check whether a report is outstanding _at the time of the query_, and for diffing tags or timestamps, we check whether a report is outstanding _at the time of the tag/timestamp_. A user-facing documentation is written in #4006, and can be previewed here: https://github.com/Szelethus/codechecker/blob/diff_docs_rewrite/docs/web/diff.md --- .../codechecker_server/api/report_server.py | 12 +++- .../diff_cmdline/test_diff_cmdline.py | 12 ++-- .../diff_remote/test_diff_remote.py | 60 ++++++++++++++++--- 3 files changed, 66 insertions(+), 18 deletions(-) diff --git a/web/server/codechecker_server/api/report_server.py b/web/server/codechecker_server/api/report_server.py index 0ba5189291..79b33ac6f2 100644 --- a/web/server/codechecker_server/api/report_server.py +++ b/web/server/codechecker_server/api/report_server.py @@ -70,6 +70,14 @@ SQLITE_MAX_COMPOUND_SELECT = 500 +def dumpSql(q): + try: + import sqlparse + print(sqlparse.format(str(q), reindent=True)) + except ImportError: + LOG.error("Failed to dump SQL query, couldn't import sqlparse!") + + class CommentKindValue: USER = 0 SYSTEM = 1 @@ -588,9 +596,11 @@ def get_open_reports_date_filter_query_old(tbl=Report, date=RunHistory.time): def get_diff_bug_id_query(session, run_ids, tag_ids, open_reports_date): """ Get bug id query for diff. """ q = session.query(Report.bug_id.distinct()) - q = q.filter(Report.fixed_at.is_(None)) + if run_ids: q = q.filter(Report.run_id.in_(run_ids)) + if not tag_ids and not open_reports_date: + q = q.filter(Report.fixed_at.is_(None)) if tag_ids: q = q.outerjoin(RunHistory, diff --git a/web/tests/functional/diff_cmdline/test_diff_cmdline.py b/web/tests/functional/diff_cmdline/test_diff_cmdline.py index 0efb525531..ffd076f753 100644 --- a/web/tests/functional/diff_cmdline/test_diff_cmdline.py +++ b/web/tests/functional/diff_cmdline/test_diff_cmdline.py @@ -755,10 +755,8 @@ def get_run_diff_count(diff_type: DiffType): ["run1:tag1"], ["run1:tag2"]) return len(reports) - # FIXME: The FP suppression appeared from one tag to the next, so it - # should be in the RESOLVED set. self.assertEqual(get_run_diff_count(DiffType.NEW), 0) - self.assertEqual(get_run_diff_count(DiffType.RESOLVED), 0) + self.assertEqual(get_run_diff_count(DiffType.RESOLVED), 1) self.assertEqual(get_run_diff_count(DiffType.UNRESOLVED), 0) def get_run_diff_count_reverse(diff_type: DiffType): @@ -767,9 +765,7 @@ def get_run_diff_count_reverse(diff_type: DiffType): ["run1:tag2"], ["run1:tag1"]) return len(reports) - # FIXME: The FP suppression appeared from one tag to the next, so it - # should be in the NEW set when the diff sets are reversed. - self.assertEqual(get_run_diff_count_reverse(DiffType.NEW), 0) + self.assertEqual(get_run_diff_count_reverse(DiffType.NEW), 1) self.assertEqual(get_run_diff_count_reverse(DiffType.RESOLVED), 0) self.assertEqual(get_run_diff_count_reverse(DiffType.UNRESOLVED), 0) @@ -809,7 +805,7 @@ def get_run_diff_count(diff_type: DiffType): return len(reports) self.assertEqual(get_run_diff_count(DiffType.NEW), 0) - self.assertEqual(get_run_diff_count(DiffType.RESOLVED), 0) + self.assertEqual(get_run_diff_count(DiffType.RESOLVED), 1) self.assertEqual(get_run_diff_count(DiffType.UNRESOLVED), 0) def get_run_diff_count_reverse(diff_type: DiffType): @@ -818,7 +814,7 @@ def get_run_diff_count_reverse(diff_type: DiffType): ["run1:tag2"], ["run1:tag1"]) return len(reports) - self.assertEqual(get_run_diff_count_reverse(DiffType.NEW), 0) + self.assertEqual(get_run_diff_count_reverse(DiffType.NEW), 1) self.assertEqual(get_run_diff_count_reverse(DiffType.RESOLVED), 0) self.assertEqual(get_run_diff_count_reverse(DiffType.UNRESOLVED), 0) diff --git a/web/tests/functional/diff_remote/test_diff_remote.py b/web/tests/functional/diff_remote/test_diff_remote.py index 1fe1ae8902..4c94f5718f 100644 --- a/web/tests/functional/diff_remote/test_diff_remote.py +++ b/web/tests/functional/diff_remote/test_diff_remote.py @@ -119,8 +119,11 @@ def setup_class(self): # Extend the checker configuration with the server access. codechecker_cfg.update(server_access) - # Base analysis + # ===-------------------------------------------------------------=== # + # Baseline analysis. + # ===-------------------------------------------------------------=== # + # ===-------------------------- Analysis -------------------------=== # altered_file = os.path.join(test_proj_path_base, "call_and_message.cpp") project.insert_suppression(altered_file) @@ -138,18 +141,24 @@ def setup_class(self): codechecker_cfg['reportdir'] = os.path.join(test_proj_path_base, 'reports') + # ===------------------------- Store 1. --------------------------=== # test_project_name_base = project_info['name'] + '_' + uuid.uuid4().hex ret = codechecker.store(codechecker_cfg, test_project_name_base) if ret: sys.exit(1) + # ===------------------------- Store 2. --------------------------=== # # Store with a literal ':' in the name. ret = codechecker.store(codechecker_cfg, test_project_name_base + ":base") if ret: sys.exit(1) - # New analysis + # ===-------------------------------------------------------------=== # + # New analysis (as opposed to baseline, this is the newer version). + # ===-------------------------------------------------------------=== # + + # ===-------------------------- Analysis -------------------------=== # altered_file = os.path.join(test_proj_path_new, "call_and_message.cpp") project.insert_suppression(altered_file) codechecker_cfg['reportdir'] = os.path.join(test_proj_path_new, @@ -166,24 +175,30 @@ def setup_class(self): codechecker_cfg['reportdir'] = os.path.join(test_proj_path_new, 'reports') + # ===------------------------- Store 1. --------------------------=== # test_project_name_new = project_info['name'] + '_' + uuid.uuid4().hex ret = codechecker.store(codechecker_cfg, test_project_name_new) if ret: sys.exit(1) + # ===------------------------- Store 2. --------------------------=== # # Store with a literal ':' in the name. ret = codechecker.store(codechecker_cfg, test_project_name_new + ":new") if ret: sys.exit(1) + # ===-------------------------------------------------------------=== # + # Another round of analyses for tags -- tag1 + # ===-------------------------------------------------------------=== # + + # ===-------------------------- Analysis -------------------------=== # # Analyze multiple times to store results with multiple tags. codechecker_cfg['reportdir'] = os.path.join(test_proj_path_update, 'reports') test_project_name_update = \ project_info['name'] + '_' + uuid.uuid4().hex - codechecker_cfg['tag'] = 't1' codechecker_cfg['checkers'] = ['-d', 'core.CallAndMessage', '-e', 'core.StackAddressEscape' ] @@ -196,12 +211,18 @@ def setup_class(self): if ret: sys.exit(1) + # ===--------------------------- Store ---------------------------=== # # Store update with t1 tag. + codechecker_cfg['tag'] = 't1' ret = codechecker.store(codechecker_cfg, test_project_name_update) if ret: sys.exit(1) - codechecker_cfg['tag'] = 't2' + # ===-------------------------------------------------------------=== # + # Another round of analyses for tags -- tag2 + # ===-------------------------------------------------------------=== # + + # ===-------------------------- Analysis -------------------------=== # codechecker_cfg['checkers'] = ['-e', 'core.CallAndMessage', '-d', 'core.StackAddressEscape' ] @@ -209,22 +230,35 @@ def setup_class(self): if ret: sys.exit(1) + # ===--------------------------- Store ---------------------------=== # # Store update with t2 tag. + codechecker_cfg['tag'] = 't2' ret = codechecker.store(codechecker_cfg, test_project_name_update) if ret: sys.exit(1) - codechecker_cfg['tag'] = 't3' + # ===-------------------------------------------------------------=== # + # Another round of analyses for tags -- tag3 + # Mind that the analysis config from tag2 to tag3 is unchanged. + # ===-------------------------------------------------------------=== # + + # ===-------------------------- Analysis -------------------------=== # ret = codechecker.log_and_analyze(codechecker_cfg, test_proj_path_update) if ret: sys.exit(1) + # ===--------------------------- Store ---------------------------=== # # Store update with t3 tag. + codechecker_cfg['tag'] = 't3' ret = codechecker.store(codechecker_cfg, test_project_name_update) if ret: sys.exit(1) + # ===-------------------------------------------------------------=== # + # Done with the analyses and stores. + # ===-------------------------------------------------------------=== # + # Order of the test run names matter at comparison! codechecker_cfg['run_names'] = [test_project_name_base, test_project_name_new, @@ -811,7 +845,8 @@ def get_run_tags(tag_name): cmp_data, False) - # 5 new core.CallAndMessage issues. + # 5 new core.CallAndMessage issues (as the checker was enabled from + # tag1 to tag2). self.assertEqual(len(diff_res), 5) cmp_data.diffType = DiffType.RESOLVED @@ -822,7 +857,10 @@ def get_run_tags(tag_name): tag_filter, cmp_data, False) - self.assertEqual(len(diff_res), 0) + + # 3 core.StackAddressEscape reports are resolved (as the checker was + # disabled from tag1 to tag2). + self.assertEqual(len(diff_res), 3) cmp_data.diffType = DiffType.UNRESOLVED diff_res = self._cc_client.getRunResults([run_id], @@ -880,7 +918,8 @@ def get_run_tags(tag_name): cmp_data, False) - # 5 new core.CallAndMessage issues. + # 5 new core.CallAndMessage issues (as the checker was enabled from + # tag1 to tag2). self.assertEqual(len(diff_res), 5) cmp_data.diffType = DiffType.RESOLVED @@ -891,7 +930,10 @@ def get_run_tags(tag_name): tag_filter, cmp_data, False) - self.assertEqual(len(diff_res), 0) + + # 3 core.StackAddressEscape reports are resolved (as the checker was + # disabled from tag1 to tag2). + self.assertEqual(len(diff_res), 3) cmp_data.diffType = DiffType.UNRESOLVED diff_res = self._cc_client.getRunResults([run_id],