Skip to content
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

WIP: Implement reporter for code issues in a PR #5929

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

Conversation

p12tic
Copy link
Member

@p12tic p12tic commented Mar 19, 2021

This is a WIP PR that demonstrates a reporter that takes a list of issues reported by e.g. pylint, determines if they are made in code that has been changed in a PR and for each such issue submits a comment on specific line on a github PR.

See an example screenshot below.

20210319_github_pr_comments

Further work: we probably need to automatically remove all comments that have not been commented on when a new test run completes. Otherwise it introduces additional effort needed by the developers to close no longer relevant issues when a PR is updated.

The current design is as follows:

  • GitDiffInfo step creates information of diff between e.g. master and the PR branch. Saves it as build data.
  • Pylint submits the code issues as test results.
  • There is GitHubCodeCommentPush reporter. It has BuildCodeIssueCommentsGenerator attached to it. On build end event it asks the generator to produce a list of comments.
  • The BuildCodeIssueCommentsGenerator generator analyzes the test results produced by pylint, looks into the diff information produced by GitDiffInof, determines which of the code issues are in new code and produces a list of comments to create.
  • GitHubCodeCommentPush posts the comments.

@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #5929 (4553e6c) into master (de1a41a) will decrease coverage by 0.30%.
The diff coverage is 23.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5929      +/-   ##
==========================================
- Coverage   91.68%   91.38%   -0.31%     
==========================================
  Files         339      340       +1     
  Lines       36260    36404     +144     
==========================================
+ Hits        33246    33268      +22     
- Misses       3014     3136     +122     
Impacted Files Coverage Δ
master/buildbot/reporters/github.py 65.71% <14.49%> (-25.07%) ⬇️
master/buildbot/reporters/generators/comment.py 29.33% <29.33%> (ø)
master/buildbot/reporters/message.py 98.23% <100.00%> (ø)
master/buildbot/scripts/cleanupdb.py 86.79% <0.00%> (-13.21%) ⬇️
master/buildbot/__init__.py 90.76% <0.00%> (-1.54%) ⬇️
worker/buildbot_worker/__init__.py 91.04% <0.00%> (-1.50%) ⬇️
master/buildbot/db/connector.py 98.83% <0.00%> (-1.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de1a41a...4553e6c. Read the comment docs.

Copy link
Member

@tardyp tardyp left a comment

Choose a reason for hiding this comment

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

I think this feature is really cool!


result_line = result['line']

preceding_change_i = bisect.bisect(changes_in_path, (result_line + 1, -1))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if bisect is very well known module of python.

I wasn't aware of it.

Also, the fact that we are comparing tuple like this looks suspicious.

I think we need a proper helper function to compare code line intervals (with unit tests)

result_line < preceding_change_start + preceding_change_length

@defer.inlineCallbacks
def _get_usable_results_in_changed_lines(self, master, target_changes_by_path, result_sets):
Copy link
Member

Choose a reason for hiding this comment

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

an error could be in an untouched line (removal of used import for example).

gitlab allows you to comment on untouched lines.

So for this generic code, I believe we shouldn't filterout

maybe we could split two sets of results something like:

touched_line_issues
untouched_lines_issues

@link2xt
Copy link

link2xt commented Apr 12, 2021

GitHub has "annotations" which can be seen in https://github.com/actions-rs/clippy-check screenshot and described in https://github.community/t/what-are-annotations/16173

Submitting these comments as a review has a disadvantage that if you push additional commits closing these problems, review comments will not go away and have to be resolved manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants