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