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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ruby 3.0] improve specs for Slack action #18512

Merged
merged 5 commits into from Apr 20, 2021

Conversation

ainame
Copy link
Contributor

@ainame ainame commented Apr 6, 2021

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

As per the plan #17931 for Ruby 3.0, I've been working on fixing Ruby 2.7's deprecation warnings which will crash on Ruby 3.0. There is an issue where one of the fastlane's dependencies slack-notifier, which is not maintained any more, keeps producing that warning.

There would be some options in solving that issue -

  1. Ping the maintainers and ask them to release a new version
  2. Fork the repo and publish it as a different gem, like fastlane-slack-notifier
  3. Replace the gem with our own one

In my opinion, 1 is too little hope and 2 would be too much. I would go with 3 as I've got this rewritten one tailored for fastlane's use-case within 60 LoC, which I think is fair to have its own implementation. #18537

So, in order to migrate the Slack action, I'd like to prepare for it by improving the testability of it. Prior to this PR, slack_spec.rb uses a hack that makes it return specific objects to verify instead of performing its process entirely. That pulls out Slack::Notifier object from the gem to unit tests. Meaning the unit tests will be completely broken when removing the dependency. This PR makes the test cases independent from the gem.

Description

There are two parts to this PR. The first one is to extract logic in Fastlane::SlackAction.run into a runner class that allows me to mock a network request to Slack by expect(..).to receive(...).with(...) in rspec-mocks. It is a simpler approach than using the Helper.test? hack. The second one is to use argument matcher to describe what parameters are given to the method handling network requests without having to depend on slack-notifier gem.

-        notifier, attachments = Fastlane::Actions::SlackAction.run(options)
-
-        expect(notifier.config.defaults[:username]).to eq('fastlane')
-        expect(notifier.config.defaults[:channel]).to eq(channel)
-
-        expect(attachments[:color]).to eq('danger')
-        expect(attachments[:text]).to eq(message)
-        expect(attachments[:pretext]).to eq(nil)
-
-        fields = attachments[:fields]
-        expect(fields[1][:title]).to eq('Built by')
-        expect(fields[1][:value]).to eq('Jenkins')
-
-        expect(fields[2][:title]).to eq('Lane')
-        expect(fields[2][:value]).to eq(lane_name)
-
-        expect(fields[3][:title]).to eq('Result')
-        expect(fields[3][:value]).to eq('Error')
+        expected_args = {
+          attachments: [
+            hash_including(
+              color: 'danger',
+              pretext: nil,
+              text: message,
+              fields: array_including(
+                { title: 'Built by', value: 'Jenkins', short: false },
+                { title: 'Lane', value: lane_name, short: true },
+                { title: 'Result', value: 'Error', short: true },
+              )
+            )
+          ],
+          link_names: false,
+          icon_url: 'https://fastlane.tools/assets/img/fastlane_icon.png',
+          fail_on_error: true,
+        }
+        expect(subject).to receive(:post_message).with(expected_args)
+        subject.run(options)

Argument Matcher is much more descriptive than using many assertions to verify a big Hash object馃檨

Testing Steps

Add a lane like below to fastlane/Fastfile in this repo's clone.

lane :slack_test do
  slack(
    slack_url: 'https://hooks.slack.com/services/xxxxxxxxxxx',
    pretext: 'pretext test <@xxxxxx> #general',
    username: 'ainame-bot',
    channel: '#general',
    message: 'test <@xxxxxx> #general',
    link_names: true,
  )
end

Screenshot 2021-04-07 at 02 41 16

@google-cla google-cla bot added the cla: yes label Apr 6, 2021
@ainame ainame force-pushed the refactor-slack-action-spec branch 2 times, most recently from b25addd to c82b36a Compare April 7, 2021 01:39
@ainame ainame force-pushed the refactor-slack-action-spec branch from c82b36a to 40ca684 Compare April 7, 2021 01:43
@ainame ainame marked this pull request as ready for review April 7, 2021 01:43
@ainame ainame changed the title [action] improve specs for Slack action [Ruby 3.0] improve specs for Slack action Apr 7, 2021
Comment on lines 299 to 301
def self.generate_slack_attachments(options)
UI.deprecation('`Fastlane::Actions::Slack.generate_slack_attachments` is subject to be removed as Slack recommends migrating `attachments` to Block Kit. fastlane will also the same direction.')
Runner.generate_slack_attachments(options)
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've got feedback to keep this method working here.
#18537 (comment)

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Just a UX comment, other than that this PR LGTM 馃挭

end
a.merge(b, &merger)
def self.generate_slack_attachments(options)
UI.deprecated('`Fastlane::Actions::Slack.generate_slack_attachments` is subject to be removed as Slack recommends migrating `attachments` to Block Kit. fastlane will also follow the same direction.')
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't support Block Kit yet (do we?), I'm concerned to be marking something as deprecated without offering an alternative to users yet 馃

Copy link
Contributor Author

@ainame ainame Apr 19, 2021

Choose a reason for hiding this comment

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

What do you think about this?
#18512 (comment)

I guess UI.deprecated is not necessarily meant to provide alternatives and can be used to announce future removal 馃 I mean most users won't see this message. Only plugin users whose plugins refer to this method directly, like this plugin, will see it.

https://github.com/crazymanish/fastlane-plugin-slack_bot/blob/0ca42f4d8f6f6d7cdd02f54615210a6ae254eed9/lib/fastlane/plugin/slack_bot/actions/update_slack_message.rb#L16

crazymanish added a commit to crazymanish/fastlane-plugin-slack_bot that referenced this pull request Apr 18, 2021
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is such a high quality code change! I鈥檓 learning so much about better code architecture from you 馃槏

@joshdholtz joshdholtz merged commit a7db339 into fastlane:master Apr 20, 2021
@ainame ainame deleted the refactor-slack-action-spec branch April 20, 2021 05:10
@fastlane-bot
Copy link

Congratulations! 馃帀 This was released as part of fastlane 2.181.0 馃殌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants