Allow testing of commands that return multiple messages with respond_with_slack_message #157

gcraig99 · 2017-09-14T17:40:36Z

Allows testing of a single response out of multiple responses, and also allows testing of all responses from a single command call.

Resolves #135

dblock · 2017-09-14T19:44:03Z

I like the idea. This needs tests and a passing build, please. I would use "once" instead of "at least once" to have a stronger expectation. Maybe something more elaborate can afford for multiple repeated messages. Right now that's not the behavior for a single message.

dblock · 2017-09-14T19:44:53Z

I also wonder whether we should instead introduce to_respond_with_slack_messages (plural)? Avoids the if and is clean and easily extensible.

…th_slack_message matcher was failing even if one of the messages received was the correct message. Refactored respond_with_slack_message to allow a check for a single message from multiple responses. Also added respond_with_slack_messages matcher that can take an array of responses to test commands that return multiple messages. Included are rspec tests for both the matchers. README.md has been edited as well to add notes on the new matcher.

gcraig99 · 2017-09-15T02:57:51Z

Refactored to update support for a single message from a group of messages using respond_with_slack_message and added new respond_with_slack_messages to allow checking of multiple responses at once. Agree that it's cleaner that way. Added tests for both matchers and updated README.md with notes on new matcher. A single test case is still failing for me, but it appears to be unrelated to any of my work. The VCR is not matching the response for migration_in_progress and is failing with an error that no response is available in the VCR.

gcraig99 · 2017-09-15T03:01:14Z

Build is still failing due to the above mentioned test case. If I'm missing something just let me know, but it doesn't appear that I'm the cause of that failure, copied below.

SlackRubyBot::Server retries on rtm.start errors migration_in_progress
Failure/Error:
expect do
subject.run
end.to raise_error Slack::Web::Api::Error, 'unknown'
```
expected Slack::Web::Api::Errors::SlackError with "unknown", got #<VCR::Errors::UnhandledHTTPRequestError: 
```

dblock

This is good work. Lets deal with the spec failure later. Made some comments. Thanks for hanging in here!

dblock · 2017-09-15T14:09:47Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ### 0.10.5 (Next)

 * Refactored `SlackRubyBot::MVC::Controller::Base`, consolidated ivar handling, centralized object allocations and DRYed up the code - [@chuckremes](https://github.com/chuckremes).
+* [#157](https://github.com/slack-ruby/slack-ruby-bot/pull/157): Added support for checking multiple responses at once via new `respond_with_slack_messages` matcher, and single response out of multiple responses to `response_with_slack_message` matcher - [@gcraig99](https://github.com/gcraig99) 


We already had respond_with_slack_message (typo btw), so I would just say "Added respond_with_slack_messages RSpec expectation".

You need a period at the end of this for Danger to be happy ;)

dblock · 2017-09-15T14:10:09Z

README.md

@@ -547,6 +547,7 @@ Slack-ruby-bot ships with a number of shared RSpec behaviors that can be used in

 * [behaves like a slack bot](lib/slack-ruby-bot/rspec/support/slack-ruby-bot/it_behaves_like_a_slack_bot.rb): A bot quacks like a Slack Ruby bot.
 * [respond with slack message](lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_message.rb): The bot responds with a message.
+* [respond with slack messages](lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_messages.rb): The bot responds with a set of message.


set of messageS or just "multiple messages"

dblock · 2017-09-15T14:11:21Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_message.rb

    message_command.call(client, Hashie::Mash.new(text: message, channel: channel, user: user))
+    expect(client).to have_received(:message).with(channel: channel, text: expected).once


I believe this changes behavior, ie. it allows the client to receive multiple messages that aren't the one expected plus the one expected. I think we don't want that, we want expectations to be "precise".

dblock · 2017-09-15T14:12:32Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_messages.rb

+require 'rspec/expectations'
+RSpec::Matchers.define :respond_with_slack_messages do |expected|
+  match do |actual|
+    raise ArgumentError, 'respond_with_slack_messages expects an array of ordered responses' unless expected.is_a? Array


I would check for Enumerable instead or maybe just that it responds to each? This way I can get the messages from some collection, database, whatever, if needed.

dblock · 2017-09-15T14:13:13Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_messages.rb

+    expected.each do |exp|
+      expect(client).to have_received(:message).with(channel: channel, text: exp).once
+    end
+    true


Same as above, I think we want exact expectations, making sure only the messages expected have been received. This behavior should have tests in both cases.

dblock · 2017-09-15T14:13:35Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_messages.rb

+  def parse(actual)
+    actual = { message: actual } unless actual.is_a?(Hash)
+    [actual[:channel] || 'channel', actual[:user] || 'user', actual[:message]]
+  end


There's some duplication here with the other expectation. Maybe put the common code in a module and include it?

dblock · 2017-09-15T14:16:23Z

Without debugging, the VCR error means that something is making an HTTP request that wasn't expecting. You can record that and look at what's being made by changing vcr.rb to record new episodes, but most likely there's a behavior change in the code if it's failing an old spec.

…changed from start to connect in the slack-ruby-client. Changed vcr file to reflect rtm.connect. Added better logging to respond_with_slack_message and respond_with_slack_messages so it's easier to see why the test is failing. Added a few more tests for the message expectations. Fixed CHANGELOG.md message as requested.

moving common code in expectations to a module and including it. Checking that expected responds to each, instead of testing specifically for an array.

gcraig99 · 2017-09-15T20:03:38Z

Ok, I think I've addressed all of the concerns and we have a passing build now. If there's anything else just let me know.

dblock

Much better. Still seems a bit complicated wrt the implementation, see my comments.

dblock · 2017-09-16T01:11:35Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_message.rb

+
+    def client.say(options = {})
+      super
+      @test_received_messages = @test_received_messages.nil? ? '' : @test_received_messages


Sending messages with \n is going to confuse things, so make this an array:

test_received_messages ||= [] test_received_messages << options

dblock · 2017-09-16T01:11:48Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_message.rb

-               SlackRubyBot::Client.new
-             end
+    client = respond_to?(:client) ? send(:client) : SlackRubyBot::Client.new
+    def client.test_messages


Try client.attr_accessor :test_messages instead?

No go, that causes an error since it's an instance of a class, rather than a class definition.

dblock · 2017-09-16T01:13:51Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_message.rb

    message_command.call(client, Hashie::Mash.new(text: message, channel: channel, user: user))
+    @messages = client.test_messages


Why not leave the old code with expect, and then you don't need the failure_message at all?

dblock · 2017-09-16T01:14:56Z

lib/slack-ruby-bot/rspec/support/slack-ruby-bot/respond_with_slack_messages.rb

+    allow(client).to receive(:message)
+    message_command.call(client, Hashie::Mash.new(text: message, channel: channel, user: user))
+    @messages = client.test_messages
+    expected.each do |exp|


If you make this expect(...) above you don't need to do this whole saving of @messages and failure_message, no? This seems unnecessarily complicated.

The expect statements are still there, the issue is that when using the expect statement without the custom failure message and the storage of the received messages, you don't get any useful output.

This is the output without the custom failure message:

RSpec it_fails_on_purpose_to_show_output
Failure/Error:
expect(message: "#{SlackRubyBot.config.user} single message")
.to respond_with_slack_message(/failure message/i)

expected {:message=>"rubybot single message"} to respond with slack message /failure message/i

RSpec respond_with_failure_to_show_output
Failure/Error:
expect(message: "#{SlackRubyBot.config.user} respond 5 times")
.to respond_with_slack_messages(expected_responses)

expected {:message=>"rubybot respond 5 times"} to respond with slack messages /response 0/i, /response 1/i, /response 2/i, /response 3/i, /response 4/i, and /response 7/i

This is the output with the custom failure message, much more useful in figuring out why it failed.

RSpec it_fails_on_purpose_to_show_output
Failure/Error:
expect(message: "#{SlackRubyBot.config.user} single message")
.to respond_with_slack_message(/failure message/i)

`expected to receive message with text: (?i-mx:failure message) once, received:[{:text=>"single response", :channel=>"channel"}]`

RSpec respond_with_failure_to_show_output
Failure/Error:
expect(message: "#{SlackRubyBot.config.user} respond 5 times")
.to respond_with_slack_messages(expected_responses)

expected to receive messages in order with text: [/response 0/i, /response 1/i, /response 2/i, /response 3/i, /response 4/i, /response 7/i] received:[{:text=>"response 0", :channel=>"channel"}, {:text=>"response 1", :channel=>"channel"}, {:text=>"response 2", :channel=>"channel"}, {:text=>"response 3", :channel=>"channel"}, {:text=>"response 4", :channel=>"channel"}]

…pectation failure along side the received message.

dblock · 2017-09-18T16:13:12Z

Merging, thanks for the good work.

gcraig99 force-pushed the multiple_expect_slack_messages branch from a086f41 to 86d3188 Compare September 14, 2017 17:51

gcraig99 force-pushed the multiple_expect_slack_messages branch from 86d3188 to ce559fb Compare September 15, 2017 02:54

dblock requested changes Sep 15, 2017

View reviewed changes

dblock mentioned this pull request Sep 15, 2017

Add RSpec regex expectations #158

Closed

lothos and others added 4 commits September 15, 2017 16:36

Fixed typo in README.md

a39cdfc

Incorporated additional feedback including:

2d8c3fb

moving common code in expectations to a module and including it. Checking that expected responds to each, instead of testing specifically for an array.

Trailing space in CHANGELOG.md flagged by Danger.

ab2e384

dblock requested changes Sep 16, 2017

View reviewed changes

lothos added 2 commits September 17, 2017 19:25

Formatted he output of the failure message a bit better.

b351432

Formatted he output of the failure message a bit better.

887980d

gcraig99 force-pushed the multiple_expect_slack_messages branch from df5b7d3 to 887980d Compare September 18, 2017 00:09

lothos added 2 commits September 17, 2017 21:11

Formatted he output of the failure message a bit better.

7d4bba9

Added logic so that the custom failure message only shows the true ex…

5f47a25

…pectation failure along side the received message.

dblock merged commit cffb894 into slack-ruby:master Sep 18, 2017

gcraig99 mentioned this pull request Sep 21, 2017

Fixed expectation failures when client.say is passed extra arguments #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow testing of commands that return multiple messages with respond_with_slack_message #157

Allow testing of commands that return multiple messages with respond_with_slack_message #157

gcraig99 commented Sep 14, 2017

dblock commented Sep 14, 2017 •

edited

Loading

dblock commented Sep 14, 2017 •

edited

Loading

gcraig99 commented Sep 15, 2017

gcraig99 commented Sep 15, 2017

dblock left a comment

dblock Sep 15, 2017

dblock Sep 15, 2017

dblock Sep 15, 2017

dblock Sep 15, 2017

dblock Sep 15, 2017

dblock Sep 15, 2017

dblock commented Sep 15, 2017

gcraig99 commented Sep 15, 2017

dblock left a comment

dblock Sep 16, 2017

dblock Sep 16, 2017

gcraig99 Sep 17, 2017

dblock Sep 16, 2017

dblock Sep 16, 2017

gcraig99 Sep 17, 2017

dblock commented Sep 18, 2017

		message_command.call(client, Hashie::Mash.new(text: message, channel: channel, user: user))
		expect(client).to have_received(:message).with(channel: channel, text: expected).once

		message_command.call(client, Hashie::Mash.new(text: message, channel: channel, user: user))
		@messages = client.test_messages

Allow testing of commands that return multiple messages with respond_with_slack_message #157

Allow testing of commands that return multiple messages with respond_with_slack_message #157

Conversation

gcraig99 commented Sep 14, 2017

dblock commented Sep 14, 2017 • edited Loading

dblock commented Sep 14, 2017 • edited Loading

gcraig99 commented Sep 15, 2017

gcraig99 commented Sep 15, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Sep 15, 2017

gcraig99 commented Sep 15, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This is the output without the custom failure message:

This is the output with the custom failure message, much more useful in figuring out why it failed.

dblock commented Sep 18, 2017

dblock commented Sep 14, 2017 •

edited

Loading

dblock commented Sep 14, 2017 •

edited

Loading