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

@@ -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

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)
+    "<details><summary>#{summary}</summary><pre>#{details}</pre></details>"
+  end
+
   def user_github
     @user_github ||= GithubApi.new(token)
   end

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

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

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

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

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

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

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

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:" \
+        "<details><summary>invalid config</summary>" \
+        "<pre>invalid config</pre></details>" \
+        "<details><summary>foo</summary>" \
+        "<pre>foo<br>  bar</pre></details>",
       )
     end
   end

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:" \
+        "<details><summary>invalid config syntax</summary>" \
+        "<pre>invalid config syntax</pre></details>",
+      )
       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

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
 

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
 

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)

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