From 7bfd195e0219c3b5f3ccb643d8f2198e5d3e1958 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 02:42:23 +0900 Subject: [PATCH 1/8] Modify with errors --- lib/lgtm/error_handleable.rb | 26 ++++++++++++++++++++++++++ lib/lgtm/errors.rb | 6 ++++++ lib/lgtm/plugin.rb | 14 +++++++++----- 3 files changed, 41 insertions(+), 5 deletions(-) create mode 100644 lib/lgtm/error_handleable.rb create mode 100644 lib/lgtm/errors.rb diff --git a/lib/lgtm/error_handleable.rb b/lib/lgtm/error_handleable.rb new file mode 100644 index 0000000..dfac39a --- /dev/null +++ b/lib/lgtm/error_handleable.rb @@ -0,0 +1,26 @@ +module Lgtm + module ErrorHandleable + # 2xx http success status. + SUCCESS_STATUSES = [Net::HTTPSuccess, Net::HTTPFound] + # 4xx http status. + CLIENT_ERRORS = [Net::HTTPBadRequest, Net::HTTPForbidden, Net::HTTPNotFound] + # 5xx http status. + SERVER_ERRORS = [ + Net::HTTPInternalServerError, + Net::HTTPBadGateway, + Net::HTTPServiceUnavailable, + Net::HTTPGatewayTimeOut, + ] + + def validate_response(response) + case response + when *SUCCESS_STATUSES + # Nothing to do. + when *SERVER_ERRORS + raise ::Lgtm::Errors::UnexpectedError + when *CLIENT_ERRORS + raise ::Lgtm::Errors::UnexpectedError + end + end + end +end diff --git a/lib/lgtm/errors.rb b/lib/lgtm/errors.rb new file mode 100644 index 0000000..81158ae --- /dev/null +++ b/lib/lgtm/errors.rb @@ -0,0 +1,6 @@ +module Lgtm + class Errors + class UnexpectedError < StandardError + end + end +end diff --git a/lib/lgtm/plugin.rb b/lib/lgtm/plugin.rb index a5f3bc4..577ede6 100644 --- a/lib/lgtm/plugin.rb +++ b/lib/lgtm/plugin.rb @@ -3,6 +3,8 @@ require 'uri' require 'json' require 'net/http' +require './lib/lgtm/errors' +require './lib/lgtm/error_handleable' module Danger # Lgtm let danger say lgtm when there is no violations. @@ -13,9 +15,10 @@ module Danger # lgtm.check_lgtm image_url: 'http://some.iamge' # # @see leonhartX/danger-lgtm - # @tags lgtm, github + # @tags lgtm, githubrequire'pry';binding.pry # class DangerLgtm < Plugin + include ::Lgtm::ErrorHandleable RANDOM_LGTM_POST_URL = 'https://lgtm.in/g'.freeze # Check status report, say lgtm if no violations @@ -41,7 +44,6 @@ def check_lgtm(image_url: nil, https_image_only: false) # returns "

LGTM

" when ServiceTemporarilyUnavailable. def fetch_image_url(https_image_only: false) - return unless lgtm_post_url lgtm_post_response = process_request(lgtm_post_url) do |req| req['Accept'] = 'application/json' end @@ -53,6 +55,8 @@ def fetch_image_url(https_image_only: false) return fetch_image_url(https_image_only: true) end url + rescue ::Lgtm::Errors::UnexpectedError + nil end def process_request(url) @@ -68,9 +72,9 @@ def process_request(url) end def lgtm_post_url - lgtm_post_req = process_request(RANDOM_LGTM_POST_URL) - return if lgtm_post_req.code == '503' - lgtm_post_req['location'] + res = process_request(RANDOM_LGTM_POST_URL) + validate_response(res) + res['location'] end def markdown_template(image_url) From 51c6d059c55aa803628f61eeab9f3553ca61d58e Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 02:42:35 +0900 Subject: [PATCH 2/8] Fix up specs --- spec/lgtm_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/lgtm_spec.rb b/spec/lgtm_spec.rb index 889beb2..f359c4c 100644 --- a/spec/lgtm_spec.rb +++ b/spec/lgtm_spec.rb @@ -25,8 +25,8 @@ module Danger expect(@dangerfile.status_report[:markdowns].length).to eq(1) end - it 'lgtm with default url is OverQuota' do - allow(Net::HTTP).to receive(:start).and_return(mock(code: '503')) + it 'lgtm with errors' do + allow(@lgtm).to receive(:validate_response).and_raise(::Lgtm::Errors::UnexpectedError) @lgtm.check_lgtm @@ -35,14 +35,12 @@ module Danger end def mock(request_url: 'https://lgtm.in/p/sSuI4hm0q', - actual_image_url: 'https://example.com/image.jpg', - code: '302') + actual_image_url: 'https://example.com/image.jpg') double( :[] => request_url, body: JSON.generate( actualImageUrl: actual_image_url - ), - code: code + ) ) end From 2b99fa533da7e4d59edc015cf8c637c94fa2d496 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 03:03:55 +0900 Subject: [PATCH 3/8] Improve tests --- spec/lgtm_spec.rb | 97 +++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 50 deletions(-) diff --git a/spec/lgtm_spec.rb b/spec/lgtm_spec.rb index f359c4c..239a0d9 100644 --- a/spec/lgtm_spec.rb +++ b/spec/lgtm_spec.rb @@ -2,70 +2,67 @@ require File.expand_path('spec_helper', __dir__) -module Danger - describe Danger::DangerLgtm do - it 'should be a plugin' do - expect(Danger::DangerLgtm.new(nil)).to be_a Danger::Plugin - end +describe Danger::DangerLgtm do + def mock(request_url: 'https://lgtm.in/p/sSuI4hm0q', + actual_image_url: 'https://example.com/image.jpg') + double( + :[] => request_url, + body: JSON.generate( + actualImageUrl: actual_image_url + ) + ) + end - # - # You should test your custom attributes and methods here - # - describe 'with Dangerfile' do - before do - @dangerfile = testing_dangerfile - @lgtm = @dangerfile.lgtm - end + it 'should be a plugin' do + expect(Danger::DangerLgtm.new(nil)).to be_a(Danger::Plugin) + end - # Some examples for writing tests - # You should replace these with your own. + describe '#check_lgtm' do + subject { lgtm.check_lgtm(https_image_only: https_image_only, image_url: image_url) } - it 'lgtm with no violation' do - @lgtm.check_lgtm - expect(@dangerfile.status_report[:markdowns].length).to eq(1) - end + let(:dangerfile) { testing_dangerfile } + let(:lgtm) { dangerfile.lgtm } + let(:https_image_only) { false } # default false + let(:image_url) {} # default nil - it 'lgtm with errors' do - allow(@lgtm).to receive(:validate_response).and_raise(::Lgtm::Errors::UnexpectedError) - - @lgtm.check_lgtm + shared_examples 'returns correctly message' do + it do + allow(Net::HTTP).to receive(:start).and_return(mock) + is_expected - expect(@dangerfile.status_report[:markdowns][0].message) - .to eq("

LGTM

") + expect(dangerfile.status_report[:markdowns][0].message) + .to match(expected_message) end + end - def mock(request_url: 'https://lgtm.in/p/sSuI4hm0q', - actual_image_url: 'https://example.com/image.jpg') - double( - :[] => request_url, - body: JSON.generate( - actualImageUrl: actual_image_url - ) - ) + context 'with Dangerfile' do + it 'when no violation' do + is_expected + expect(dangerfile.status_report[:markdowns].length).to eq(1) end - it 'pick random pic from lgtm.in' do - allow(Net::HTTP).to receive(:start).and_return(mock) - - @lgtm.check_lgtm - - expect(@dangerfile.status_report[:markdowns][0].message) - .to match(%r{https:\/\/example.com\/image.jpg}) + it 'lgtm with errors' do + allow(lgtm).to receive(:validate_response).and_raise(::Lgtm::Errors::UnexpectedError) + is_expected + expect(dangerfile.status_report[:markdowns][0].message) + .to eq("

LGTM

") end - it 'pick random pic from lgtm.in with https_image_only option' do - allow(Net::HTTP).to receive(:start).and_return(mock) - - @lgtm.check_lgtm https_image_only: true + context 'pick random pic from lgtm.in' do + let(:expected_message) { %r{https:\/\/example.com\/image.jpg} } + it_behaves_like 'returns correctly message' + end - expect(@dangerfile.status_report[:markdowns][0].message) - .to match(%r{https:\/\/example.com\/image.jpg}) + context 'pick random pic from lgtm.in with https_image_only option' do + let(:https_image_only) { true } + let(:expected_message) { %r{https:\/\/example.com\/image.jpg} } + it_behaves_like 'returns correctly message' end - it 'use given url' do - @lgtm.check_lgtm image_url: 'http://imgur.com/Irk2wyX.jpg' - expect(@dangerfile.status_report[:markdowns][0].message) - .to match(%r{http:\/\/imgur\.com\/Irk2wyX\.jpg}) + context 'use given url' do + let(:image_url) { 'http://imgur.com/Irk2wyX.jpg' } + let(:expected_message) { %r{http:\/\/imgur\.com\/Irk2wyX\.jpg} } + it_behaves_like 'returns correctly message' end end end From 75edbf6755a65312a99ed6b27c3c70354d4d33b3 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 03:10:27 +0900 Subject: [PATCH 4/8] Oops --- lib/lgtm/plugin.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/lgtm/plugin.rb b/lib/lgtm/plugin.rb index 577ede6..f425510 100644 --- a/lib/lgtm/plugin.rb +++ b/lib/lgtm/plugin.rb @@ -15,7 +15,7 @@ module Danger # lgtm.check_lgtm image_url: 'http://some.iamge' # # @see leonhartX/danger-lgtm - # @tags lgtm, githubrequire'pry';binding.pry + # @tags lgtm, github # class DangerLgtm < Plugin include ::Lgtm::ErrorHandleable From 89e1b7df00f35c00a10c7ed22d3d79f2e015b944 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 03:19:47 +0900 Subject: [PATCH 5/8] Fix with ci --- lib/lgtm/error_handleable.rb | 15 ++++++++------- lib/lgtm/plugin.rb | 3 +-- spec/lgtm_spec.rb | 10 ++++++++-- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/lgtm/error_handleable.rb b/lib/lgtm/error_handleable.rb index dfac39a..ab6810a 100644 --- a/lib/lgtm/error_handleable.rb +++ b/lib/lgtm/error_handleable.rb @@ -1,21 +1,22 @@ module Lgtm + # ErrorHandleable of module of error handling module ErrorHandleable - # 2xx http success status. - SUCCESS_STATUSES = [Net::HTTPSuccess, Net::HTTPFound] # 4xx http status. - CLIENT_ERRORS = [Net::HTTPBadRequest, Net::HTTPForbidden, Net::HTTPNotFound] + CLIENT_ERRORS = [ + Net::HTTPBadRequest, + Net::HTTPForbidden, + Net::HTTPNotFound + ].freeze # 5xx http status. SERVER_ERRORS = [ Net::HTTPInternalServerError, Net::HTTPBadGateway, Net::HTTPServiceUnavailable, - Net::HTTPGatewayTimeOut, - ] + Net::HTTPGatewayTimeOut + ].freeze def validate_response(response) case response - when *SUCCESS_STATUSES - # Nothing to do. when *SERVER_ERRORS raise ::Lgtm::Errors::UnexpectedError when *CLIENT_ERRORS diff --git a/lib/lgtm/plugin.rb b/lib/lgtm/plugin.rb index f425510..0c074bc 100644 --- a/lib/lgtm/plugin.rb +++ b/lib/lgtm/plugin.rb @@ -55,8 +55,7 @@ def fetch_image_url(https_image_only: false) return fetch_image_url(https_image_only: true) end url - rescue ::Lgtm::Errors::UnexpectedError - nil + rescue ::Lgtm::Errors::UnexpectedError; nil end def process_request(url) diff --git a/spec/lgtm_spec.rb b/spec/lgtm_spec.rb index 239a0d9..04f4369 100644 --- a/spec/lgtm_spec.rb +++ b/spec/lgtm_spec.rb @@ -18,7 +18,12 @@ def mock(request_url: 'https://lgtm.in/p/sSuI4hm0q', end describe '#check_lgtm' do - subject { lgtm.check_lgtm(https_image_only: https_image_only, image_url: image_url) } + subject do + lgtm.check_lgtm( + https_image_only: https_image_only, + image_url: image_url + ) + end let(:dangerfile) { testing_dangerfile } let(:lgtm) { dangerfile.lgtm } @@ -42,7 +47,8 @@ def mock(request_url: 'https://lgtm.in/p/sSuI4hm0q', end it 'lgtm with errors' do - allow(lgtm).to receive(:validate_response).and_raise(::Lgtm::Errors::UnexpectedError) + allow(lgtm).to receive(:validate_response) + .and_raise(::Lgtm::Errors::UnexpectedError) is_expected expect(dangerfile.status_report[:markdowns][0].message) .to eq("

LGTM

") From 16809c148c3d3d1d915c0380c03f789e9029174c Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 03:33:54 +0900 Subject: [PATCH 6/8] Fix with order by rake --- lib/lgtm/error_handleable.rb | 5 +++++ lib/lgtm/plugin.rb | 5 ++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/lgtm/error_handleable.rb b/lib/lgtm/error_handleable.rb index ab6810a..188d528 100644 --- a/lib/lgtm/error_handleable.rb +++ b/lib/lgtm/error_handleable.rb @@ -15,6 +15,11 @@ module ErrorHandleable Net::HTTPGatewayTimeOut ].freeze + # validate_response is response validationg + # @param [Net::HTTPxxx] response Net::HTTP responses + # @raise ::Lgtm::Errors::UnexpectedError + # @return [void] + # def validate_response(response) case response when *SERVER_ERRORS diff --git a/lib/lgtm/plugin.rb b/lib/lgtm/plugin.rb index 0c074bc..e1dbed7 100644 --- a/lib/lgtm/plugin.rb +++ b/lib/lgtm/plugin.rb @@ -24,11 +24,10 @@ class DangerLgtm < Plugin # Check status report, say lgtm if no violations # Generates a `markdown` of a lgtm image. # - # @param [image_url] lgtm image url - # @param [https_image_only] fetching https image only if true + # @param [String] image_url lgtm image url + # @param [Boolean] https_image_only https image only if true # # @return [void] - # def check_lgtm(image_url: nil, https_image_only: false) return unless status_report[:errors].length.zero? && status_report[:warnings].length.zero? From c649f817c63d07a36319faf246aadb88d8be4614 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Sun, 5 Aug 2018 03:41:20 +0900 Subject: [PATCH 7/8] bump up --- Gemfile.lock | 4 ++-- lib/lgtm/gem_version.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index ee8db6a..e49397a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - danger-lgtm (1.0.2) + danger-lgtm (1.0.3) danger-plugin-api (~> 1.0) GEM @@ -135,4 +135,4 @@ DEPENDENCIES yard (~> 0.9.11) BUNDLED WITH - 1.15.4 + 1.16.3 diff --git a/lib/lgtm/gem_version.rb b/lib/lgtm/gem_version.rb index 2c6250d..71ffd74 100644 --- a/lib/lgtm/gem_version.rb +++ b/lib/lgtm/gem_version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Lgtm - VERSION = '1.0.2'.freeze + VERSION = '1.0.3'.freeze end From 92c87c6ad28216f20ea221d6a9f5f297fb4a7bc9 Mon Sep 17 00:00:00 2001 From: Ryota Ikezawa Date: Mon, 6 Aug 2018 01:46:25 +0900 Subject: [PATCH 8/8] Fix up unneed points --- Gemfile.lock | 2 +- lib/lgtm/error_handleable.rb | 5 +++-- lib/lgtm/plugin.rb | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index e49397a..b0bbb22 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -135,4 +135,4 @@ DEPENDENCIES yard (~> 0.9.11) BUNDLED WITH - 1.16.3 + 1.15.4 diff --git a/lib/lgtm/error_handleable.rb b/lib/lgtm/error_handleable.rb index 188d528..1625e31 100644 --- a/lib/lgtm/error_handleable.rb +++ b/lib/lgtm/error_handleable.rb @@ -1,5 +1,5 @@ module Lgtm - # ErrorHandleable of module of error handling + # ErrorHandleable is module of error handling module ErrorHandleable # 4xx http status. CLIENT_ERRORS = [ @@ -15,7 +15,8 @@ module ErrorHandleable Net::HTTPGatewayTimeOut ].freeze - # validate_response is response validationg + # validate_response is response validating + # # @param [Net::HTTPxxx] response Net::HTTP responses # @raise ::Lgtm::Errors::UnexpectedError # @return [void] diff --git a/lib/lgtm/plugin.rb b/lib/lgtm/plugin.rb index e1dbed7..1b3fd29 100644 --- a/lib/lgtm/plugin.rb +++ b/lib/lgtm/plugin.rb @@ -28,6 +28,7 @@ class DangerLgtm < Plugin # @param [Boolean] https_image_only https image only if true # # @return [void] + # def check_lgtm(image_url: nil, https_image_only: false) return unless status_report[:errors].length.zero? && status_report[:warnings].length.zero?