Issue #87 Add some SlackNotificationService tests and gral Rubocop warnings cleanup #89
Conversation
| @@ -4,7 +4,7 @@ class PullRequest | |||
| :url, :body, :language, :repo_name | |||
|
|
|||
| def initialize(params) | |||
| @info = params.dig(:pull_request) | |||
There was a problem hiding this comment.
trying to avoid nil errors
There was a problem hiding this comment.
Params never gonna be nil so why this can raise an error? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sometimes a complex input structure is a smell that could be improve/cleaned, sometimes with separated params or a Struct/PORO
| @action = params[:github_webhook][:action] | ||
| @extra_params = params | ||
| @action = params.dig(:github_webhook, :action) | ||
| raise(ArgumentError, 'No action param set') unless @action |
There was a problem hiding this comment.
Trying to avoid wrong initializations
| @@ -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] | |||
| @@ -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 | |||
| def js_channels(pr, lang) | ||
| return unless pr[:name] | ||
| def js_channels(pull_request, _lang) | ||
| return unless pull_request[:name] |
| end | ||
|
|
||
| context 'with a PR from a blacklisted user' do | ||
| let!(:user) { User.create(github_name: 'pepe', blacklisted: true) } |
There was a problem hiding this comment.
I guess that with an existing dependabot user should work, too.
| @@ -0,0 +1,48 @@ | |||
| require 'rails_helper' | |||
|
|
|||
| describe SlackNotificationService do | |||
There was a problem hiding this comment.
| describe SlackNotificationService do | |
| describe SlackNotificationService do | |
| describe '#initialize' do |
There was a problem hiding this comment.
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.
Co-authored-by: Alan Lamas <[email protected]>
|
Added another commit with some better params setup requiring extra web stubbing. |
| @@ -0,0 +1,48 @@ | |||
| require 'rails_helper' | |||
|
|
|||
| describe SlackNotificationService do | |||
| stub_request(:post, 'https://slack.com/api/conversations.list') | ||
| .to_return(status: 200, body: '', headers: {}) | ||
|
|
||
| # stub_request(:post, 'https://slack.com/api/conversations.info') |
There was a problem hiding this comment.
during test construction some execution paths required the stubing of this endpoint, too. Then I simplified the scenarios and was no longer required. I kept it just in case, but maybe it's better to cleanup the code as much as possible. I can remove it.
| end | ||
|
|
||
| describe '#send_notification' do | ||
| subject { described_class.new(params).send_notification } |
There was a problem hiding this comment.
I am not sure about this. Isn't it better if we only create the object in the "subject", and after call the method? 🤔
There was a problem hiding this comment.
not sure what style is better. The subject under test in this scenario is the method
Summary
Trying to tackle #87 by proving that with a PR from a blacklisted user it does not sends the message.
Other Information
none