Skip to content

Commit

Permalink
[cli] Fix gerrit output
Browse files Browse the repository at this point in the history 
- Use source line in the gerrit output instead of the line number at the
end of the report message.
- If a report not found in the changed files list, add report to the main
message.
  • Loading branch information
@csordasmarton
csordasmarton committed Jul 12, 2021
1 parent 53fc3be commit c01dc2e
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 91 deletions.
43 changes: 23 additions & 20 deletions codechecker_common/output/gerrit.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def __convert_reports(reports: List[Report],
review_comments = {}

report_count = 0
report_messages_in_unchanged_files = []
for report in reports:
bug_line = report.line
bug_col = report.col
Expand All @@ -86,16 +87,10 @@ def __convert_reports(reports: List[Report],
severity = severity_map.get(check_name, "UNSPECIFIED")
file_name = report.file_path
check_msg = report.description
source_line = report.line

# Skip the report if it is not in the changed files.
if changed_file_path and not \
any([file_name.endswith(c) for c in changed_files]):
LOG.debug("Skip report from '%s' file because this file has not "
"changed.", file_name)
continue
source_line = report.source_line

report_count += 1

# file_name can be without a path in the report.
rel_file_path = os.path.relpath(file_name, repo_dir) \
if repo_dir and os.path.dirname(file_name) != "" else file_name
Expand All @@ -106,8 +101,15 @@ def __convert_reports(reports: List[Report],
if rel_file_path not in review_comments:
review_comments[rel_file_path] = []

review_comment_msg = "[{0}] {1}: {2} [{3}]\n{4}".format(
severity, checked_file, check_msg, check_name, source_line)
review_comment_msg = \
f"[{severity}] {checked_file}: {check_msg} [{check_name}]\n" \
f"{source_line}"

# Skip the report if it is not in the changed files.
if changed_file_path and not \
any([file_name.endswith(c) for c in changed_files]):
report_messages_in_unchanged_files.append(review_comment_msg)
continue

review_comments[rel_file_path].append({
"range": {
Expand All @@ -117,8 +119,13 @@ def __convert_reports(reports: List[Report],
"end_character": bug_col},
"message": review_comment_msg})

message = "CodeChecker found {0} issue(s) in the code.".format(
report_count)
message = f"CodeChecker found {report_count} issue(s) in the code."

if report_messages_in_unchanged_files:
message += ("\n\nThere following reports are introduced in files "
"which are not changed and can't be shown as individual "
"reports:\n{0}\n".format('\n'.join(
report_messages_in_unchanged_files)))

if report_url:
message += " See: '{0}'".format(report_url)
Expand All @@ -144,19 +151,17 @@ def __get_changed_files(changed_file_path: Union[None, str]) -> List[str]:
changed_files = []

if not changed_file_path or not os.path.exists(changed_file_path):
LOG.warning("CC_CHANGED_FILES is specified but the file (%s) doesn't "
"exist.", changed_file_path)
return changed_files

with open(changed_file_path, encoding='utf-8', errors='ignore') as f:
content = f.read()
with open(changed_file_path,
encoding='utf-8',
errors='ignore') as changed_file:
content = changed_file.read()

# The file can contain some garbage values at start, so we use
# regex search to find a json object.
match = re.search(r'\{[\s\S]*\}', content)
if not match:
LOG.debug("The content of the given changed file (%s) is invalid!",
changed_file_path)
return changed_files

for filename in json.loads(match.group(0)):
Expand All @@ -165,6 +170,4 @@ def __get_changed_files(changed_file_path: Union[None, str]) -> List[str]:

changed_files.append(filename)

LOG.debug("Changed file paths: %s", changed_files)

return changed_files
4 changes: 4 additions & 0 deletions codechecker_common/tests/unit/test_files/lib.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
double foo(int param)
{
return 1 / param;
}
4 changes: 4 additions & 0 deletions codechecker_common/tests/unit/test_files/main.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
int main()
{
sizeof(42);
}
139 changes: 68 additions & 71 deletions codechecker_common/tests/unit/test_gerrit_converter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ def test_report_to_gerrit_conversion(self):
"""Conversion without directory path just the source filename."""

main = {
"location": {"file": 0, "line": 10, "col": 10},
"location": {"file": 0, "line": 3, "col": 3},
"description": "some description",
"check_name": "my_checker",
"issue_hash_content_of_line_in_context": "dummy_hash",
"notes": [],
"macro_expansions": [],
}
bugpath = {}
files = {0: "main.cpp"}
files = {0: "test_files/main.cpp"}
metadata = {}

report_to_convert = Report(main, bugpath, files, metadata)
Expand All @@ -46,16 +46,16 @@ def test_report_to_gerrit_conversion(self):
"message": "CodeChecker found 1 issue(s) in the code.",
"labels": {"Code-Review": -1, "Verified": -1},
"comments": {
"main.cpp": [
"test_files/main.cpp": [
{
"range": {
"start_line": 10,
"start_character": 10,
"end_line": 10,
"end_character": 10,
"start_line": 3,
"start_character": 3,
"end_line": 3,
"end_character": 3,
},
"message": "[LOW] main.cpp:10:10: "
"some description [my_checker]\n10",
"message": "[LOW] test_files/main.cpp:3:3: "
"some description [my_checker]\n sizeof(42);\n",
}
]
},
Expand All @@ -68,19 +68,22 @@ def test_report_to_gerrit_conversion_abs_filepath(self):
main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
"line": 3,
"col": 3,
},
"description": "some description",
"check_name": "my_checker",
"issue_hash_content_of_line_in_context": "dummy_hash",
"notes": [],
"macro_expansions": [],
}

bugpath = {}
files = {0: "/home/src/lib/main.cpp"}
metadata = {}

file_path = os.path.abspath("test_files/main.cpp")
files = {0: file_path}

report_to_convert = Report(main, bugpath, files, metadata)

got = gerrit.convert([report_to_convert], self.severity_map)
Expand All @@ -89,16 +92,17 @@ def test_report_to_gerrit_conversion_abs_filepath(self):
"message": "CodeChecker found 1 issue(s) in the code.",
"labels": {"Code-Review": -1, "Verified": -1},
"comments": {
"/home/src/lib/main.cpp": [
file_path: [
{
"range": {
"start_line": 10,
"start_character": 10,
"end_line": 10,
"end_character": 10,
"start_line": 3,
"start_character": 3,
"end_line": 3,
"end_character": 3,
},
"message": "[LOW] /home/src/lib/main.cpp:10:10: "
"some description [my_checker]\n10",
"message": "[LOW] {0}:3:3: some description "
"[my_checker]\n sizeof(42);\n".format(
file_path),
}
]
},
Expand All @@ -111,8 +115,8 @@ def test_report_to_gerrit_conversion_repo_dir(self):
main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
"line": 3,
"col": 3,
},
"description": "some description",
"check_name": "my_checker",
Expand All @@ -121,11 +125,13 @@ def test_report_to_gerrit_conversion_repo_dir(self):
"macro_expansions": [],
}
bugpath = {}
files = {0: "/home/src/lib/main.cpp"}
metadata = {}

file_path = os.path.abspath("test_files/main.cpp")
files = {0: file_path}

report_to_convert = Report(main, bugpath, files, metadata)
os.environ["CC_REPO_DIR"] = "/home/src/lib"
os.environ["CC_REPO_DIR"] = os.path.dirname(os.path.realpath(__file__))

got = gerrit.convert([report_to_convert], self.severity_map)
os.environ.pop("CC_REPO_DIR")
Expand All @@ -135,16 +141,18 @@ def test_report_to_gerrit_conversion_repo_dir(self):
"message": "CodeChecker found 1 issue(s) in the code.",
"labels": {"Code-Review": -1, "Verified": -1},
"comments": {
"main.cpp": [
"test_files/main.cpp": [
{
"range": {
"start_line": 10,
"start_character": 10,
"end_line": 10,
"end_character": 10,
"start_line": 3,
"start_character": 3,
"end_line": 3,
"end_character": 3,
},
"message": "[LOW] main.cpp:10:10: "
"some description [my_checker]\n10",
"message": "[LOW] test_files/main.cpp:3:3: "
"some description [my_checker]\n"
" sizeof(42);\n".format(
file_path),
}
]
},
Expand All @@ -157,8 +165,8 @@ def test_report_to_gerrit_conversion_report_url(self):
main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
"line": 3,
"col": 3,
},
"description": "some description",
"check_name": "my_checker",
Expand All @@ -167,7 +175,7 @@ def test_report_to_gerrit_conversion_report_url(self):
"macro_expansions": [],
}
bugpath = {}
files = {0: "/home/src/lib/main.cpp"}
files = {0: "test_files/main.cpp"}
metadata = {}

report_to_convert = Report(main, bugpath, files, metadata)
Expand All @@ -183,16 +191,16 @@ def test_report_to_gerrit_conversion_report_url(self):
"See: 'localhost:8080/index.html'",
"labels": {"Code-Review": -1, "Verified": -1},
"comments": {
"/home/src/lib/main.cpp": [
"test_files/main.cpp": [
{
"range": {
"start_line": 10,
"start_character": 10,
"end_line": 10,
"end_character": 10,
"start_line": 3,
"start_character": 3,
"end_line": 3,
"end_character": 3,
},
"message": "[LOW] /home/src/lib/main.cpp:10:10: "
"some description [my_checker]\n10",
"message": "[LOW] test_files/main.cpp:3:3: "
"some description [my_checker]\n sizeof(42);\n",
}
]
},
Expand All @@ -202,7 +210,7 @@ def test_report_to_gerrit_conversion_report_url(self):
def test_report_to_gerrit_conversion_filter_changed_files(self):
"""Conversion report with changed files filter.
Reports from the other.cpp file should be not in the converted list.
Reports from the lib.cpp file should be not in the converted list.
"""

reports_to_convert = []
Expand All @@ -214,16 +222,18 @@ def test_report_to_gerrit_conversion_filter_changed_files(self):
main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
"line": 3,
"col": 3,
},
"description": "some description",
"check_name": "my_checker",
"issue_hash_content_of_line_in_context": "dummy_hash",
"notes": [],
"macro_expansions": [],
}
files = {0: "/home/src/lib/main.cpp"}

main_file_path = os.path.abspath("test_files/main.cpp")
files = {0: main_file_path}

main_report = Report(main, bugpath, files, metadata)
reports_to_convert.append(main_report)
Expand All @@ -232,37 +242,22 @@ def test_report_to_gerrit_conversion_filter_changed_files(self):
main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
"line": 3,
"col": 3,
},
"description": "some description",
"check_name": "my_checker",
"issue_hash_content_of_line_in_context": "dummy_hash",
"notes": [],
"macro_expansions": [],
}
files = {0: "/home/src/lib/lib.cpp"}

lib_file_path = os.path.abspath("test_files/lib.cpp")
files = {0: lib_file_path}

lib_report = Report(main, bugpath, files, metadata)
reports_to_convert.append(lib_report)

main = {
"location": {
"file": 0,
"line": 10,
"col": 10,
},
"description": "some description",
"check_name": "my_checker",
"issue_hash_content_of_line_in_context": "dummy_hash",
"notes": [],
"macro_expansions": [],
}
files = {0: "/home/src/lib/other.cpp"}

other_report = Report(main, bugpath, files, metadata)
reports_to_convert.append(other_report)

dummy_changed_files_content = {
"/COMMIT_MSG": {
"status": "A",
Expand All @@ -275,8 +270,7 @@ def test_report_to_gerrit_conversion_filter_changed_files(self):
"lines_deleted": 1,
"size_delta": 1,
"size": 100,
},
"lib.cpp": {"lines_inserted": 1, "size_delta": 1, "size": 100},
}
}
fd, changed_files_file = tempfile.mkstemp()
os.write(fd, json.dumps(dummy_changed_files_content).encode("utf-8"))
Expand All @@ -296,9 +290,12 @@ def test_report_to_gerrit_conversion_filter_changed_files(self):
self.assertEquals(len(review_comments), 2)

# Two reports in the main.cpp file.
self.assertEquals(len(review_comments["/home/src/lib/main.cpp"]), 2)
self.assertEquals(len(review_comments[main_file_path]), 2)

self.assertIn(
"CodeChecker found 3 issue(s) in the code.", got["message"])
self.assertIn(
"following reports are introduced in files which are not changed",
got["message"])

self.assertEquals(
"CodeChecker found 3 issue(s) in the code.", got["message"]
)
self.assertNotIn("/home/src/lib/other.cpp", review_comments.keys())
self.assertIn(lib_file_path, review_comments.keys())

0 comments on commit c01dc2e

Please sign in to comment.