From d2c1449fe4a4f33345019e5326d66f7d75293592 Mon Sep 17 00:00:00 2001 From: Greg Lazarev Date: Fri, 19 May 2017 14:14:14 -0700 Subject: [PATCH] Report linter errors as the main PR review comment When a linter encounters an unexpected error, like invalid config syntax, we will receive it in the `error` attribute in the payload. Then we'll report it to the user via the main comment that goes into the PR review body. --- app/models/build.rb | 5 ++++ app/models/pull_request.rb | 23 ++++++++++++++-- app/services/complete_build.rb | 4 +-- app/services/complete_file_review.rb | 22 ++++++++++------ ...0170505220736_add_error_to_file_reviews.rb | 5 ++++ db/schema.rb | 4 +-- lib/github_api.rb | 3 ++- spec/lib/github_api_spec.rb | 15 +++++++++-- spec/models/build_spec.rb | 16 ++++++++++++ spec/models/pull_request_spec.rb | 10 +++++-- spec/requests/builds_spec.rb | 14 ++++++++-- spec/services/complete_build_spec.rb | 26 ++++++++++++++++--- spec/services/complete_file_review_spec.rb | 6 +++-- spec/support/fake_github.rb | 13 ++-------- spec/support/helpers/github_api_helper.rb | 4 +-- 15 files changed, 130 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20170505220736_add_error_to_file_reviews.rb diff --git a/app/models/build.rb b/app/models/build.rb index 42ecba9bf..909b1f19b 100644 --- a/app/models/build.rb +++ b/app/models/build.rb @@ -18,6 +18,11 @@ def user_token (user && user.token) || Hound::GITHUB_TOKEN end + def review_errors + file_reviews.where.not(error: [nil, ""]).pluck("DISTINCT error"). + uniq { |error| error.lines.first } + end + private def generate_uuid diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index 61b59d696..ef4caf578 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -13,9 +13,14 @@ def commit_files @commit_files ||= modified_commit_files end - def comment_on_violations(violations) + def make_comments(violations, errors) comments = violations.map { |violation| build_comment(violation) } - hound_github.create_pull_request_review(repo_name, number, comments) + hound_github.create_pull_request_review( + repo_name, + number, + comments, + format_errors(errors), + ) end def repository_owner_name @@ -62,6 +67,20 @@ def build_comment(violation) } end + def format_errors(errors) + if errors.any? + error_details = errors.map { |error| build_error_details(error) } + ["Some files could not be reviewed due to errors:"]. + concat(error_details).join + end + end + + def build_error_details(error) + summary = error.lines.first.strip.truncate(80) + details = error.lines.map(&:rstrip).join(COMMENT_LINE_DELIMITER) + "
#{summary}
#{details}
" + end + def user_github @user_github ||= GithubApi.new(token) end diff --git a/app/services/complete_build.rb b/app/services/complete_build.rb index 57acb946e..7cfc7f9ab 100644 --- a/app/services/complete_build.rb +++ b/app/services/complete_build.rb @@ -13,8 +13,8 @@ def call new_violations = priority_violations.select do |violation| commenting_policy.comment_on?(violation) end - if new_violations.any? - pull_request.comment_on_violations(new_violations) + if new_violations.any? || build.review_errors.any? + pull_request.make_comments(new_violations, build.review_errors) end set_commit_status track_subscribed_build_completed diff --git a/app/services/complete_file_review.rb b/app/services/complete_file_review.rb index b6e24fede..8bb07de44 100644 --- a/app/services/complete_file_review.rb +++ b/app/services/complete_file_review.rb @@ -6,7 +6,7 @@ def initialize(attributes) end def call - create_violations! + complete_file_review CompleteBuild.call( pull_request: pull_request, @@ -19,18 +19,24 @@ def call attr_reader :attributes - def create_violations! + def complete_file_review + build_file_review_violations + file_review.error = attributes["error"] + file_review.complete + file_review.save! + increment_build_violations_count + end + + def build_file_review_violations attributes.fetch("violations").each do |violation| line = commit_file.line_at(violation.fetch("line")) file_review.build_violation(line, violation.fetch("message")) end + end - file_review.complete - file_review.save! - file_review.build.increment!( - :violations_count, - file_review.violations.map(&:messages_count).sum, - ) + def increment_build_violations_count + count = file_review.violations.map(&:messages_count).sum + file_review.build.increment!(:violations_count, count) end def pull_request diff --git a/db/migrate/20170505220736_add_error_to_file_reviews.rb b/db/migrate/20170505220736_add_error_to_file_reviews.rb new file mode 100644 index 000000000..c410b2633 --- /dev/null +++ b/db/migrate/20170505220736_add_error_to_file_reviews.rb @@ -0,0 +1,5 @@ +class AddErrorToFileReviews < ActiveRecord::Migration[5.0] + def change + add_column :file_reviews, :error, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index c65da4081..b28c32b8a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1,4 +1,3 @@ -# encoding: UTF-8 # This file is auto-generated from the current state of the database. Instead # of editing this file, please use the migrations feature of Active Record to # incrementally modify your database, and then regenerate this schema definition. @@ -11,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161021231021) do +ActiveRecord::Schema.define(version: 20170505220736) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -61,6 +60,7 @@ t.datetime "updated_at", null: false t.string "filename", null: false t.string "linter_name", null: false + t.string "error" end add_index "file_reviews", ["build_id"], name: "index_file_reviews_on_build_id", using: :btree diff --git a/lib/github_api.rb b/lib/github_api.rb index e1d678b19..830a03472 100644 --- a/lib/github_api.rb +++ b/lib/github_api.rb @@ -65,11 +65,12 @@ def pull_request_comments(full_repo_name, pull_request_number) client.pull_request_comments(full_repo_name, pull_request_number) end - def create_pull_request_review(repo_name, pr_number, comments) + def create_pull_request_review(repo_name, pr_number, comments, body) client.post( "#{Octokit::Repository.path(repo_name)}/pulls/#{pr_number}/reviews", accept: PREVIEW_API_HEADER, event: "COMMENT", + body: body, comments: comments, ) end diff --git a/spec/lib/github_api_spec.rb b/spec/lib/github_api_spec.rb index b8f6a0edb..8b24424f2 100644 --- a/spec/lib/github_api_spec.rb +++ b/spec/lib/github_api_spec.rb @@ -164,9 +164,20 @@ { path: "test/test.rb", position: 10, body: "test comment 1" }, { path: "test/test.rb", position: 15, body: "test comment 2" }, ] - request = stub_review_request(repo_name, pull_request_number, comments) + body = "something bad happened" + request = stub_review_request( + repo_name, + pull_request_number, + comments, + body, + ) - api.create_pull_request_review(repo_name, pull_request_number, comments) + api.create_pull_request_review( + repo_name, + pull_request_number, + comments, + body, + ) expect(request).to have_been_requested end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index 5506a9b82..a797e7326 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -37,4 +37,20 @@ end end end + + describe "#review_errors" do + it "returns a list of unique linter errors" do + file_review1 = create(:file_review, error: "invalid config\n some file1") + file_review2 = create(:file_review, error: "invalid config\n some file2") + file_review3 = create(:file_review, error: "rule is invalid\n some file1") + file_reviews = [file_review1, file_review2, file_review3] + build = create(:build, file_reviews: file_reviews) + + result = build.review_errors + + expect(result.size).to eq 2 + expect(result.first).to start_with("invalid config") + expect(result.second).to start_with("rule is invalid") + end + end end diff --git a/spec/models/pull_request_spec.rb b/spec/models/pull_request_spec.rb index 772d24c02..fdc5448b1 100644 --- a/spec/models/pull_request_spec.rb +++ b/spec/models/pull_request_spec.rb @@ -59,14 +59,15 @@ end end - describe "#comment_on_violations" do + describe "#make_comments" do it "posts a review with comments to GitHub as the Hound user" do payload = payload_stub github = instance_double("GithubApi", create_pull_request_review: nil) pull_request = pull_request_stub(github, payload) violation = violation_stub + review_errors = ["invalid config", "foo\n bar"] - pull_request.comment_on_violations([violation]) + pull_request.make_comments([violation], review_errors) expect(github).to have_received(:create_pull_request_review).with( "org/repo", @@ -78,6 +79,11 @@ body: violation.messages.join, }, ], + "Some files could not be reviewed due to errors:" \ + "
invalid config" \ + "
invalid config
" \ + "
foo" \ + "
foo
  bar
", ) end end diff --git a/spec/requests/builds_spec.rb b/spec/requests/builds_spec.rb index 8971e7ebf..be117e5b5 100644 --- a/spec/requests/builds_spec.rb +++ b/spec/requests/builds_spec.rb @@ -17,10 +17,19 @@ new_violation2 = { line: 9, message: "Avoid empty else-clauses." } violations = [new_violation1, existing_comment_violation, new_violation2] create(:repo, :active, github_id: repo_id, name: repo_name) - stub_review_job(RubocopReviewJob, violations: violations) + stub_review_job( + RubocopReviewJob, + violations: violations, + error: "invalid config syntax", + ) post builds_path, params: { payload: payload } + expect(FakeGithub.review_body).to eq( + "Some files could not be reviewed due to errors:" \ + "
invalid config syntax" \ + "
invalid config syntax
", + ) expect(FakeGithub.comments).to match_array [ { body: new_violation1[:message], @@ -50,7 +59,7 @@ end end - def stub_review_job(klass, violations:) + def stub_review_job(klass, violations:, error:) allow(klass).to receive(:perform) do |attributes| CompleteFileReview.call( "commit_sha" => attributes.fetch("commit_sha"), @@ -59,6 +68,7 @@ def stub_review_job(klass, violations:) "patch" => attributes.fetch("patch"), "pull_request_number" => attributes.fetch("pull_request_number"), "violations" => violations.map(&:stringify_keys), + "error" => error, ) end end diff --git a/spec/services/complete_build_spec.rb b/spec/services/complete_build_spec.rb index dfc0f42b3..2346b30d9 100644 --- a/spec/services/complete_build_spec.rb +++ b/spec/services/complete_build_spec.rb @@ -17,7 +17,7 @@ token: "abc123", ) - expect(pull_request).to have_received(:comment_on_violations) do |arg| + expect(pull_request).to have_received(:make_comments) do |arg| expect(arg.flat_map(&:messages)).to match_array ["bar"] end end @@ -67,7 +67,7 @@ token: "abc123", ) - expect(pull_request).not_to have_received(:comment_on_violations) + expect(pull_request).not_to have_received(:make_comments) expect(github_api).not_to have_received(:create_success_status) end end @@ -100,7 +100,24 @@ token: "abc123", ) - expect(pull_request).not_to have_received(:comment_on_violations) + expect(pull_request).not_to have_received(:make_comments) + end + end + + context "when build has file review errors" do + it "adds a comment to pull request review" do + build = stub_build([], review_errors: ["cannot parse config"]) + pull_request = stub_pull_request + stubbed_github_api + + CompleteBuild.call( + pull_request: pull_request, + build: build, + token: "abc123", + ) + + expect(pull_request).to have_received(:make_comments). + with([], ["cannot parse config"]) end end @@ -148,6 +165,7 @@ def stub_build(violation_messages, attributes = {}) end default_attributes = { completed?: true, + review_errors: [], repo: instance_double("Repo", subscription: false), repo_name: "foo/bar", commit_sha: "abc123", @@ -163,7 +181,7 @@ def stub_pull_request(comments = []) "PullRequest", head_commit: head_commit, comments: comments, - comment_on_violations: nil, + make_comments: nil, ) end diff --git a/spec/services/complete_file_review_spec.rb b/spec/services/complete_file_review_spec.rb index afa7b858e..b9ee20c54 100644 --- a/spec/services/complete_file_review_spec.rb +++ b/spec/services/complete_file_review_spec.rb @@ -14,9 +14,10 @@ expect(file_review).to be_completed expect(file_review.violations.size).to eq 1 expect(file_review.build.violations_count).to eq 1 + expect(file_review.error).to eq attributes["error"] end - it "runs Build Report" do + it "runs completes the build" do file_review = create_file_review build = file_review.build stub_build_report_run @@ -63,13 +64,14 @@ end end - let(:attributes) do + def attributes { "filename" => "test.scss", "commit_sha" => "abc123", "pull_request_number" => 123, "patch" => File.read("spec/support/fixtures/patch.diff"), "violations" => ["line" => 14, "message" => "woohoo"], + "error" => "Your linter config is invalid", } end diff --git a/spec/support/fake_github.rb b/spec/support/fake_github.rb index 892cb56bd..ab6dc71f4 100644 --- a/spec/support/fake_github.rb +++ b/spec/support/fake_github.rb @@ -1,7 +1,7 @@ require "sinatra" class FakeGithub < Sinatra::Base - cattr_accessor :comments + cattr_accessor :comments, :review_body self.comments = [] put "/repos/:owner/:repo/collaborators/:username" do @@ -41,18 +41,9 @@ class FakeGithub < Sinatra::Base ].to_json end - post "/repos/:owner/:repo/pulls/:number/comments" do - request.body.rewind - request_payload = JSON.parse(request.body.read) - - comments << build_comment(request_payload, params) - - content_type :json - status 201 - end - post "/repos/:owner/:repo/pulls/:number/reviews" do request_payload = JSON.parse(request.body.read) + self.review_body = request_payload["body"] request_payload["comments"].each do |comment| comments << build_comment(comment, params) diff --git a/spec/support/helpers/github_api_helper.rb b/spec/support/helpers/github_api_helper.rb index 6338c0747..97bf798e5 100644 --- a/spec/support/helpers/github_api_helper.rb +++ b/spec/support/helpers/github_api_helper.rb @@ -128,11 +128,11 @@ def stub_repos_requests(token) ) end - def stub_review_request(repo_name, pr_number, comments) + def stub_review_request(repo_name, pr_number, comments, body) url = "https://api.github.com/repos/#{repo_name}/pulls/#{pr_number}/reviews" stub_request(:post, url). - with(body: { event: "COMMENT", comments: comments }.to_json). + with(body: { event: "COMMENT", body: body, comments: comments }.to_json). to_return(status: 200) end