Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #87 Add some SlackNotificationService tests and gral Rubocop warnings cleanup #89

Merged
merged 4 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/services/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class PullRequest
:url, :body, :language, :repo_name

def initialize(params)
@info = params.dig(:pull_request)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying to avoid nil errors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Params never gonna be nil so why this can raise an error? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know anything about the Slack connection/gem/client/etc usages (I still don't).
I just started to test the class in isolation and noticed that it was raising a lot errors because it assumes a complex parameter tree structure. I usually try to validate initialization/params just a bit, to avoid ugly bugs later in the code (most of the time, undefined xxxx for nil errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes a complex input structure is a smell that could be improve/cleaned, sometimes with separated params or a Struct/PORO

@info = params.fetch(:pull_request, {})
@username = @info.dig(:user, :login)
@avatar_url = @info.dig(:user, :avatar_url)
@webhook_label = params.dig(:github_webhook, :label, :name)
Expand Down
18 changes: 10 additions & 8 deletions app/services/slack_notification_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ class SlackNotificationService
attr_reader :action, :extra_params, :slack_bot, :pr

def initialize(params)
@action = params[:github_webhook][:action]
@extra_params = params
@action = params.dig(:github_webhook, :action)
raise(ArgumentError, 'No action param set') unless @action
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to avoid wrong initializations


@extra_params = params || {}
@slack_bot = SlackBot.new(channel: channel(params))
@pr = PullRequest.new(params)
end
Expand Down Expand Up @@ -56,7 +58,7 @@ def filter_on_hold_labeled_action

def channel(params)
lang = LANGUAGES[params.dig(:repository, :language)&.to_sym]
repository_info = params.dig(:repository)
repository_info = params[:repository]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop fix

channel = "##{build_channel(lang, repository_info)}"

if search_channel(channel)
Expand All @@ -68,7 +70,7 @@ def channel(params)

def search_channel(channel)
Slack::Web::Client.new.conversations_info(channel: channel)
rescue Exception => e
rescue StandardError => e
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop fix

Rails.logger.error("Error #{e.inspect} for channel #{channel}")
nil
end
Expand Down Expand Up @@ -110,11 +112,11 @@ def filter_merged_action
slack_bot.add_merge_emoji matches
end

def js_channels(pr, lang)
return unless pr[:name]
def js_channels(pull_request, _lang)
return unless pull_request[:name]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop fixes


repo_name = pr[:name].downcase
topics = pr[:topics].present? ? pr[:topics] : []
repo_name = pull_request[:name].downcase
topics = pull_request[:topics].present? ? pull_request[:topics] : []

js_repo_name(repo_name, topics)
end
Expand Down
6 changes: 3 additions & 3 deletions spec/requests/api/v1/github_webhook/filter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ def expect_not_notification
end

def mock_channel_response(return_value)
allow_any_instance_of(SlackNotificationService).to receive(:search_channel)
.and_return(return_value)
allow_any_instance_of(SlackNotificationService)
.to receive(:search_channel)
.and_return(return_value)
end

74 changes: 74 additions & 0 deletions spec/services/slack_notification_service_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
require 'rails_helper'

describe SlackNotificationService do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe SlackNotificationService do
describe SlackNotificationService do
describe '#initialize' do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. My idea was to explore the class code by creating scenarios. First, started with initialization scenarios, quickly finding issues with nil errors when Hash structure is not the expected one. Added a raise on #initialize and 🔁 but only one scenario did into the PR. I could add more scenarios or change it to #initialize as you proposed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I like this way

describe 'initialization' do
subject { described_class.new({}) }

it 'raise an error if no action is set in params' do
expect { subject }.to raise_error(ArgumentError)
end
end

describe '#send_notification' do
subject { described_class.new(params).send_notification }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this. Isn't it better if we only create the object in the "subject", and after call the method? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what style is better. The subject under test in this scenario is the method


let(:username) { Faker::Internet.username }
let!(:user) do
User.create(github_name: username, blacklisted: false)
end

context 'with valid params' do
let(:random_action) { described_class::ACTION_METHODS.keys.sample }
let(:channel_name) { described_class::DEFAULT_CHANNEL }
let(:some_language) { 'Node' }
let(:params) do
{
github_webhook: {
action: random_action,
label: { name: Faker::Internet.slug(words: '_') }
},
channel: channel_name,
pull_request: {
user: { login: username },
body: Faker::Lorem.sentence
},
repository: {
name: Faker::Internet.slug,
language: some_language
}
}
end

before do
stub_request(:post, 'https://slack.com/api/conversations.list')
.to_return(status: 200, body: '', headers: {})

stub_request(:post, 'https://slack.com/api/chat.postMessage')
.with(
body: {
'channel': channel_name,
'text': ' :nodejs:',
'username': username
}
).to_return(status: 200, body: '', headers: {})
end

it 'sends the message' do
expect_any_instance_of(described_class)
.to receive(:send)
.with(described_class::ACTION_METHODS[random_action])
.and_call_original
subject
end

context 'with a PR from a blacklisted user' do
let!(:user) { User.create(github_name: username, blacklisted: true) }

it 'does NOT send the message' do
expect_any_instance_of(described_class).not_to receive(:send)
subject
end
end
end
end
end