Skip to content

Added blame info feature for "cmd results" command #4008

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

Open
cservakt wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added blame info feature for "cmd results" command #4008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72ec363
Added blame info feature for "cmd results" command
cservakt Sep 13, 2023
5fc5059
Added blame info feature for "cmd results" command
cservakt Oct 10, 2023
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
3 changes: 2 additions & 1 deletion web/api/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ container so `docker` needs to be installed to generate the stubs.
- [`py/codechecker_api/setup.py`](py/codechecker_api/setup.py)
- [`py/codechecker_api_shared/setup.py`](py/codechecker_api_shared/setup.py)
- [`js/codechecker-api-node/package.json`](js/codechecker-api-node/package.json)
- [`/web/server/vue-cli/package.json`](/web/server/vue-cli/package.json)
- Let's assume that the current API version is `6.39.0`. Run the
[change-api-version.sh](change-api-version.sh) script to increment the API
version: `change-api-version.sh 6.40.0`.
- Update the supported api versions to `6.40` in the server files:
- `web/codechecker_web/shared/version.py`
- `/web/codechecker_web/shared/version.py`
- Run the command `make build` to generate the Thrift API stubs and to create
new pypi and npm packages. It will modify the following files:
- [`py/codechecker_api/dist/codechecker_api.tar.gz`](py/codechecker_api/dist/codechecker_api.tar.gz)
Expand Down
Binary file removed web/api/js/codechecker-api-node/dist/codechecker-api-6.53.0.tgz
View file
Open in desktop
Binary file not shown.
Binary file added web/api/js/codechecker-api-node/dist/codechecker-api-6.54.0.tgz
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion web/api/js/codechecker-api-node/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "codechecker-api",
"version": "6.53.0",
"version": "6.54.0",
"description": "Generated node.js compatible API stubs for CodeChecker server.",
"main": "lib",
"homepage": "https://github.com/Ericsson/codechecker",
Expand Down
Binary file modified web/api/py/codechecker_api/dist/codechecker_api.tar.gz
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion web/api/py/codechecker_api/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
with open('README.md', encoding='utf-8', errors="ignore") as f:
long_description = f.read()

api_version = '6.53.0'
api_version = '6.54.0'

setup(
name='codechecker_api',
Expand Down
Binary file modified web/api/py/codechecker_api_shared/dist/codechecker_api_shared.tar.gz
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion web/api/py/codechecker_api_shared/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
with open('README.md', encoding='utf-8', errors="ignore") as f:
long_description = f.read()

api_version = '6.53.0'
api_version = '6.54.0'

setup(
name='codechecker_api_shared',
Expand Down
1 change: 1 addition & 0 deletions web/api/report_server.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ struct ReportData {
// of custom labels that describe some properties of a report. For example the
// timestamp in case of dynamic analyzers when the report was actually emitted.
18: optional map<string, string> annotations,
19: optional BlameInfo blameInfo, // Contains the git blame information of the report if it exists.
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is longer than 79 columns.

}
typedef list<ReportData> ReportDataList

Expand Down
2 changes: 1 addition & 1 deletion web/codechecker_web/shared/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
# The newest supported minor version (value) for each supported major version
# (key) in this particular build.
SUPPORTED_VERSIONS = {
6: 53
6: 54
}

# Used by the client to automatically identify the latest major and minor
Expand Down
53 changes: 49 additions & 4 deletions web/server/codechecker_server/api/report_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1901,9 +1901,15 @@ def getRunResults(self, run_ids, limit, offset, sort_types,

# Get report details if it is required.
report_details = {}
if get_details:
report_ids = [r[0].id for r in query_result]
blame_infos = {}
if get_details and len(query_result):
report_ids, blames = zip(*[
(
r[0].id,
(r[0].id, self.getBlameInfo(r[0].file_id))
Copy link
Copy Markdown
Contributor

@bruntib bruntib Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider caching fetched blame info in case there are many reports in a file.

) for r in query_result])
report_details = get_report_details(session, report_ids)
blame_infos = dict(blames)
Comment on lines +1904 to +1912
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRunResults is already stomach-churningly long. Can we put these lines a new function instead?


for row in query_result:
report = row[0]
Expand All @@ -1919,6 +1925,21 @@ def getRunResults(self, run_ids, limit, offset, sort_types,
report.review_status_date,
report.review_status_is_in_source)

blame_info = blame_infos.get(report.id)
if blame_info and blame_info.commits and blame_info.blame:
blame_data = [b for b in blame_info.blame
if report.line >= b.startLine
and report.line <= b.endLine]
commitHash = blame_data[0].commitHash \
if len(blame_data) else None
commitInfo = {cHash: commit for cHash, commit
in blame_info.commits.items()
if cHash == commitHash}
blame_info = BlameInfo(
commits=commitInfo,
blame=blame_data
)

Comment on lines +1928 to +1942
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here.

results.append(
ReportData(runId=report.run_id,
bugHash=report.bug_id,
Expand All @@ -1937,6 +1958,7 @@ def getRunResults(self, run_ids, limit, offset, sort_types,
fixedAt=str(report.fixed_at),
bugPathLength=report.path_length,
details=report_details.get(report.id),
blameInfo=blame_info,
analyzerName=report.analyzer_name,
annotations=annotations))
else: # not is_unique
Expand Down Expand Up @@ -1986,9 +2008,15 @@ def getRunResults(self, run_ids, limit, offset, sort_types,

# Get report details if it is required.
report_details = {}
if get_details:
report_ids = [r[0].id for r in query_result]
blame_infos = {}
if get_details and len(query_result):
report_ids, blames = zip(*[
(
r[0].id,
(r[0].id, self.getBlameInfo(r[0].file_id))
) for r in query_result])
report_details = get_report_details(session, report_ids)
blame_infos = dict(blames)

for row in query_result:
report = row[0]
Expand All @@ -2004,6 +2032,22 @@ def getRunResults(self, run_ids, limit, offset, sort_types,
report.review_status_date,
report.review_status_is_in_source)

blame_info = blame_infos[report.id] \
if report.id in blame_infos else None
if blame_info and blame_info.commits and blame_info.blame:
blame_data = [b for b in blame_info.blame
if report.line >= b.startLine
and report.line <= b.endLine]
commitHash = blame_data[0].commitHash \
if len(blame_data) else None
commitInfo = {cHash: commit for cHash, commit
in blame_info.commits.items()
if cHash == commitHash}
blame_info = BlameInfo(
commits=commitInfo,
blame=blame_data
)

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These code snippets are basically copy-paste from above, right? Can we do something about that?

Copy link
Copy Markdown
Collaborator Author

@cservakt cservakt Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more readable form.

results.append(
ReportData(runId=report.run_id,
bugHash=report.bug_id,
Expand All @@ -2023,6 +2067,7 @@ def getRunResults(self, run_ids, limit, offset, sort_types,
report.fixed_at else None,
bugPathLength=report.path_length,
details=report_details.get(report.id),
blameInfo=blame_info,
analyzerName=report.analyzer_name,
annotations=annotations))

Expand Down
12 changes: 6 additions & 6 deletions web/server/vue-cli/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion web/server/vue-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"@mdi/font": "^6.5.95",
"chart.js": "^2.9.4",
"chartjs-plugin-datalabels": "^0.7.0",
"codechecker-api": "file:../../api/js/codechecker-api-node/dist/codechecker-api-6.53.0.tgz",
"codechecker-api": "file:../../api/js/codechecker-api-node/dist/codechecker-api-6.54.0.tgz",
"codemirror": "^5.65.0",
"date-fns": "^2.28.0",
"js-cookie": "^3.0.1",
Expand Down
17 changes: 17 additions & 0 deletions web/tests/functional/report_viewer_api/test_get_run_results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,3 +447,20 @@ def test_get_checker_labels(self):
self.assertEqual(len(checker_labels), 2)
self.assertEqual(set(checker_labels[0]), div_zero_labels)
self.assertEqual(set(checker_labels[1]), set())

def test_report_blame_info(self):
"""
Get run results and check that blame info exists
when get_details is set.
"""
runid = self._runid
simple_filter = ReportFilter()
run_results = self._cc_client.getRunResults([runid],
100,
0,
None,
simple_filter,
None,
True)
Comment on lines +458 to +464
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name the parameters we're assign to here? LLVM Coding Standards has a well written guidince on this. By no means are we strictly abiding all of these, but they make a lot of sense :)

Copy link
Copy Markdown
Collaborator Author

@cservakt cservakt Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not name the parameters in the above functions either. If we want to do it, then in all the other cases we must also assigne them.


self.assertTrue(any(res.blameInfo for res in run_results))
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test the json output as well? At least that is doesn't crash.

Copy link
Copy Markdown
Collaborator Author

@cservakt cservakt Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is sufficient to check only the existence of the blame info. If someone modified the test files, the blame info whould be also changed, which whould break the test.