From b23cfa85eff470558d2053ee5c1eee17d5b02f10 Mon Sep 17 00:00:00 2001 From: Jon Yurek Date: Thu, 12 Mar 2015 17:19:11 -0400 Subject: [PATCH 1/2] Give an OK of false if the PR comment exists --- lib/cc/services/github_pull_requests.rb | 20 ++++++++++---------- test/github_pull_requests_test.rb | 5 +++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/cc/services/github_pull_requests.rb b/lib/cc/services/github_pull_requests.rb index 9f9958a..b29d0f7 100644 --- a/lib/cc/services/github_pull_requests.rb +++ b/lib/cc/services/github_pull_requests.rb @@ -85,19 +85,17 @@ def update_status_pending end def update_status(state, description) - if config.update_status - params = { - state: state, - description: description, - target_url: @payload["details_url"], - context: "codeclimate" - } - service_post(status_url, params.to_json) - end + params = { + state: state, + description: description, + target_url: @payload["details_url"], + context: "codeclimate" + } + service_post(status_url, params.to_json) end def add_comment - if config.add_comment && !comment_present? + if !comment_present? body = { body: COMMENT_BODY % @payload["compare_url"] }.to_json @@ -106,6 +104,8 @@ def add_comment doc = JSON.parse(response.body) { id: doc["id"] } end + else + { ok: false, message: "Comment already present" } end end diff --git a/test/github_pull_requests_test.rb b/test/github_pull_requests_test.rb index 600392f..5f32cf2 100644 --- a/test/github_pull_requests_test.rb +++ b/test/github_pull_requests_test.rb @@ -38,7 +38,6 @@ def test_pull_request_status_error commit_sha: "abc123", state: "error", }) - end def test_pull_request_status_test_success @@ -115,11 +114,13 @@ def test_pull_request_comment_already_present # With no POST expectation, test will fail if request is made. - receive_pull_request({ add_comment: true }, { + rsp = receive_pull_request({ add_comment: true, update_status: false }, { github_slug: "pbrisbin/foo", number: 1, state: "success", }) + assert_not_nil rsp + assert_false rsp[:ok] end private From 5116688994dc465644b480e27b119b5e8bf12e1b Mon Sep 17 00:00:00 2001 From: Jon Yurek Date: Mon, 16 Mar 2015 17:03:34 -0400 Subject: [PATCH 2/2] Ensure any result that's nil result becomes useful Add a middleware that should be placed at the bottom of the Invocation stack. This middleware ensures that any time the invocation stack is called, there is a return value. If the stack comes back nil, then a message is put in its place implying an error. While this might not technically be the case that the service errored, a lack of return value should be considered an error and looked in to. --- lib/cc/service/invocation.rb | 2 ++ .../service/invocation/with_return_values.rb | 18 +++++++++++++++ test/invocation_return_values_test.rb | 21 ++++++++++++++++++ test/invocation_test.rb | 22 +++++++++++++++++++ 4 files changed, 63 insertions(+) create mode 100644 lib/cc/service/invocation/with_return_values.rb create mode 100644 test/invocation_return_values_test.rb diff --git a/lib/cc/service/invocation.rb b/lib/cc/service/invocation.rb index f643c0e..3248f19 100644 --- a/lib/cc/service/invocation.rb +++ b/lib/cc/service/invocation.rb @@ -2,12 +2,14 @@ require 'cc/service/invocation/with_retries' require 'cc/service/invocation/with_metrics' require 'cc/service/invocation/with_error_handling' +require 'cc/service/invocation/with_return_values' class CC::Service::Invocation MIDDLEWARE = { retries: WithRetries, metrics: WithMetrics, error_handling: WithErrorHandling, + return_values: WithReturnValues, } attr_reader :result diff --git a/lib/cc/service/invocation/with_return_values.rb b/lib/cc/service/invocation/with_return_values.rb new file mode 100644 index 0000000..67a1409 --- /dev/null +++ b/lib/cc/service/invocation/with_return_values.rb @@ -0,0 +1,18 @@ +class CC::Service::Invocation + class WithReturnValues + def initialize(invocation, message = nil) + @invocation = invocation + @message = message || "An internal error happened" + end + + def call + result = @invocation.call + if result.nil? + { ok: false, message: @message } + else + result + end + end + end +end + diff --git a/test/invocation_return_values_test.rb b/test/invocation_return_values_test.rb new file mode 100644 index 0000000..5db3842 --- /dev/null +++ b/test/invocation_return_values_test.rb @@ -0,0 +1,21 @@ +require File.expand_path('../helper', __FILE__) + +class InvocationReturnValuesTest < CC::Service::TestCase + def test_success_returns_upstream_result + handler = CC::Service::Invocation::WithReturnValues.new( + lambda { :return_value }, + "error message" + ) + + assert_equal :return_value, handler.call + end + + def test_empty_results_returns_hash + handler = CC::Service::Invocation::WithReturnValues.new( + lambda { nil }, + "error message" + ) + + assert_equal( {ok: false, message: "error message"}, handler.call ) + end +end diff --git a/test/invocation_test.rb b/test/invocation_test.rb index dc04c2c..e2fe483 100644 --- a/test/invocation_test.rb +++ b/test/invocation_test.rb @@ -10,6 +10,28 @@ def test_success assert_equal :some_result, result end + def test_success_with_return_values + service = FakeService.new(:some_result) + + result = CC::Service::Invocation.invoke(service) do |i| + i.with :return_values, "error" + end + + assert_equal 1, service.receive_count + assert_equal :some_result, result + end + + def test_failure_with_return_values + service = FakeService.new(nil) + + result = CC::Service::Invocation.invoke(service) do |i| + i.with :return_values, "error" + end + + assert_equal 1, service.receive_count + assert_equal( {ok: false, message: "error"}, result ) + end + def test_retries service = FakeService.new service.fake_error = RuntimeError.new