-
Notifications
You must be signed in to change notification settings - Fork 383
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.
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
Add severity to CodeClimate export #3356
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this patch 😊 I have some comments on it. If these are fixed and test cases are passed we can merge it.
@@ -638,7 +638,7 @@ def parse_convert_reports(input_dirs: List[str], | |||
|
|||
number_of_reports = len(all_reports) | |||
if out_format == "codeclimate": | |||
return codeclimate.convert(all_reports), number_of_reports | |||
return codeclimate.convert(all_reports, severity_map), number_of_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pylint complains that this line is too long:
codechecker_analyzer/cmd/parse.py:641:80: E501 line too long (80 > 79 characters)
make[1]: *** [pycodestyle] Error 1
Could you please fix this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
'UNDEFINED': 'info', | ||
'LOW': 'minor', | ||
'STYLE': 'minor', | ||
'MEDIUM': 'high', | ||
'HIGH': 'critical', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the following severity levels:
- Critical
- High
- Medium
- Low
- Style
- Unspecified
So I recommend to change this map to this one:
'UNDEFINED': 'info', | |
'LOW': 'minor', | |
'STYLE': 'minor', | |
'MEDIUM': 'high', | |
'HIGH': 'critical', | |
'CRITICAL': 'critical', | |
'HIGH': 'major', | |
'MEDIUM': 'minor', | |
'LOW': 'minor', | |
'STYLE': 'info', | |
'UNSPECIFIED': 'info' |
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks. I have no idea where I got the high
from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
"""Convert a Report to Code Climate format.""" | ||
return { | ||
"type": "issue", | ||
"check_name": report.check_name, | ||
"description": report.description, | ||
"categories": ["Bug Risk"], | ||
"fingerprint": report.report_hash, | ||
"severity": __codeclimate_severity_map.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test cases are failed.
- https://github.com/Ericsson/codechecker/blob/master/analyzer/tests/functional/analyze_and_parse/test_analyze_and_parse.py#L310 - Missing severity level.
- Traceback (most recent call last):
File "/home/runner/work/codechecker/codechecker/web/tests/functional/diff_local_remote/test_diff_local_remote.py", line 465, in test_diff_codeclimate_output
self.assertTrue(os.path.exists(codeclimate_issues_file))
Could you please fix these test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, though I couldn't run test_functional all the way. I don't have db set up for it.
@@ -638,7 +638,7 @@ def parse_convert_reports(input_dirs: List[str], | |||
|
|||
number_of_reports = len(all_reports) | |||
if out_format == "codeclimate": | |||
return codeclimate.convert(all_reports), number_of_reports | |||
return codeclimate.convert(all_reports, severity_map), number_of_reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the convert function here with the serverity map:
cc_reports = codeclimate.convert(reports) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
ca9f933
to
45dfd69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you very much for your time and your patch. We really appreciate it 😊
The severity field is needed by Gitlab-CI to process codeclimate report. This commit patch the current version of codechecker with the change introduced upstream with this pull request: Ericsson/codechecker#3356
As requested in #3342 (comment)