Get Slack tests working again

It appears that Slack is now sending recently-sent messages to a client
when it connects. That is, it's possible to receive messages sent before
you actually connect. This was causing lots of random failures in the
tests, because we would end up making assertions on messages sent in
response to previous tests! The actual functionality of the Slack
provider was fine; this was purely a testing issue.

Many changes were made to clean and tighten up the tests (detailed
below), but the big take-home is that we now pay attention to message
timestamps to determine whether to process a message or not.

An overview of what was done:

* Run Slack tests in Travis CI

* Create a Slack client once per Slack suite, as opposed to once per
  Slack *test*.

  Though not strictly necessary, this reduces the chances of running
  afoul of Slack's throttling, which results in shorter overall test
  runs.

* Update Slack tests to send test-specific messages.

  Previously, we'd send a lot of "echo blah" or "echo $TIMESTAMP"
  messages. Once Slack started apparently sending recently sent messages
  upon connection, this made for some hard-to-track-down false postives
  and timeouts. Now that every test sends unique (and traceable)
  messages, it's much easier to figure out what messages are being
  matched.

* Clean up and refactor the testing Slack client

  In the course of debugging the tests, I discovered extra code in the
  testing Slack client (and its `SlackClientState`) that wasn't actually
  being used. For instance, we would store messages, but not actually do
  anything with them. We also never join a specific channel when
  connecting our test client (that is taken care of when setting up the
  Slack accounts, anyway), so that code is out. Finally, we don't
  currently test anything with edited messages, so that functionality is
  out, too.

  Additionally, we now explicitly pay attention to the timestamp of
  messages. When we send a message, we note its timestamp and only
  process responses with a greater timestamp.

Fixes #1377

.travis.yml

@@ -5,6 +5,7 @@ services:
 
 env:
   - TEST=unit
+  - TEST=slack
   - TEST=hipchat
 
 script:

test/integration/slack_registration_test.exs

@@ -14,7 +14,7 @@ defmodule Integration.SlackRegistrationTest do
 
   @ci_room "ci_bot_testing"
 
-  setup do
+  setup_all do
     {:ok, client} = ChatClient.new()
     {:ok, %{client: client}}
   end

test/integration/slack_test.exs

@@ -8,29 +8,36 @@ defmodule Integration.SlackTest do
   @redirect_channel "ci_bot_redirect_tests"
   @private_group "group_ci_bot_testing"
 
-  setup do
+  setup_all do
+    {:ok, client} = ChatClient.new()
+    {:ok, %{client: client}}
+  end
+
+  setup context do
     # The user always interacts with the bot via the `@user` account
     # (see above). Our helper functions set up a user with the same
     # Cog username and Slack handle
     user = user(@user)
     |> with_chat_handle_for("slack")
 
-    {:ok, client} = ChatClient.new()
-    {:ok, %{client: client, user: user}}
+    {:ok, Map.put(context, :user, user)}
   end
 
   test "running the st-echo command", %{user: user, client: client} do
     user |> with_permission("operable:st-echo")
-    {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: "@#{@bot} operable:st-echo test", reply_from: @bot])
-    assert reply.text == "test"
+    {:ok, reply} = ChatClient.chat_wait!(client,
+                                         [room: @ci_room,
+                                          message: "@#{@bot}: operable:st-echo 'running the st-echo command'",
+                                          reply_from: @bot])
+    assert reply.text == "running the st-echo command"
   end
 
   test "running the st-echo command without permission", %{client: client} do
-    message = "@#{@bot}: operable:st-echo test"
+    message = "@#{@bot}: operable:st-echo 'running the st-echo command without permission'"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message,
                                                   reply_from: @bot])
     expected = """
-    ```Sorry, you aren't allowed to execute 'operable:st-echo test'.
+    ```Sorry, you aren't allowed to execute 'operable:st-echo 'running the st-echo command without permission''.
     You will need at least one of the following permissions to run this command: 'operable:st-echo'.```
     """ |> String.strip
 
@@ -47,17 +54,17 @@ defmodule Integration.SlackTest do
     |> add_to_group(role)
     |> add_to_group(user)
 
-    message = "@#{@bot}: seed '[{\"test\": \"blah\"}]' | echo $test"
+    message = "@#{@bot}: seed '[{\"test\": \"running commands in a pipeline\"}]' | echo $test"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
-    assert reply.text == "blah"
+    assert reply.text == "running commands in a pipeline"
     assert reply.location.type == :channel
     assert reply.location.name == @ci_room
   end
 
   test "running commands in a pipeline without permission", %{user: user, client: client} do
     user |> with_permission("operable:st-echo")
 
-    message = "@#{@bot}: operable:st-echo \"this is a test\" | operable:st-thorn $body"
+    message = "@#{@bot}: operable:st-echo \"running commands in a pipeline without permission\" | operable:st-thorn $body"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
     expected = """
     ```Sorry, you aren't allowed to execute 'operable:st-thorn $body'.
@@ -68,76 +75,56 @@ defmodule Integration.SlackTest do
     assert reply.location.name == @ci_room
   end
 
-  test "sending a message to a group", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-
-    message = "@#{@bot}: operable:echo blah"
+  test "sending a message to a group", %{client: client} do
+    message = "@#{@bot}: operable:echo sending a message to a group"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @private_group, message: message, reply_from: @bot])
-    assert reply.text == "blah"
-    assert reply.location.type == :group
-    assert reply.location.name == @private_group
-  end
-
-  test "redirecting from a private channel", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    message = "@#{@bot}: operable:echo blah > ##{@private_group}"
-    {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
     assert reply.location.type == :group
     assert reply.location.name == @private_group
-    assert reply.text == "blah"
+    assert reply.text == "sending a message to a group"
   end
 
-  test "redirecting to a private channel", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    time = "#{System.os_time()}"
-    message = "@#{@bot}: operable:echo #{time} > ##{@private_group}"
+  test "redirecting to a private channel", %{client: client} do
+    message = "@#{@bot}: operable:echo redirecting to a private channel > ##{@private_group}"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
     assert reply.location.type == :group
     assert reply.location.name == @private_group
-    assert reply.text == time
+    assert reply.text == "redirecting to a private channel"
   end
 
- test "redirecting to 'here'", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    message = "@#{@bot}: operable:echo blah > here"
+ test "redirecting to 'here'", %{client: client} do
+    message = "@#{@bot}: operable:echo redirecting to here > here"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
-    assert reply.text == "blah"
+    assert reply.text == "redirecting to here"
     assert reply.location.type == :channel
     assert reply.location.name == @ci_room
   end
 
-  test "redirecting to 'me'", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-
-    message = "@#{@bot}: operable:echo blah > me"
+  test "redirecting to 'me'", %{client: client} do
+    message = "@#{@bot}: operable:echo redirecting to me > me"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
     assert reply.location.type == :im
     # Since Cog responds when direct messaging it we have to assert
     # that both our marker text and message test exist.
     # assert_response "here\nblah", [after: marker, count: 2], "@#{@user}"
   end
 
-  test "redirecting to a specific user", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    message = "@#{@bot}: operable:echo blah > @#{@user}"
+  test "redirecting to a specific user", %{client: client} do
+    message = "@#{@bot}: operable:echo redirecting to a specific user > @#{@user}"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
-    assert reply.text == "blah"
+    assert reply.text == "redirecting to a specific user"
     assert reply.location.type == :im
   end
 
-  test "redirecting to another channel", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    message = "@#{@bot}: operable:echo blah > ##{@redirect_channel}"
+  test "redirecting to another channel", %{client: client} do
+    message = "@#{@bot}: operable:echo redirecting to another channel > ##{@redirect_channel}"
     {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: message, reply_from: @bot])
     assert reply.location.type == :channel
     assert reply.location.name == @redirect_channel
-    assert reply.text == "blah"
+    assert reply.text == "redirecting to another channel"
   end
 
   @tag :skip
-  test "formatting with unicode present", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-
+  test "formatting with unicode present", %{client: client} do
     # We use a different URL each time because we otherwise end up
     # having to deal with messages from Slack saying "we didn't unfurl
     # this for you since we already did in the past hour" when we run

test/integration/slack_threads_test.exs

@@ -22,22 +22,24 @@ defmodule Integration.SlackThreadsTest do
   @ci_room "ci_bot_testing"
 
   setup do
-    user = user(@user)
-    |> with_chat_handle_for("slack")
-
+    @user |> user |> with_chat_handle_for("slack")
     {:ok, client} = ChatClient.new()
-    {:ok, %{client: client, user: user}}
+    {:ok, %{client: client}}
   end
 
-  test "messages are threaded based on the original message", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: "@#{@bot} operable:echo test", reply_from: @bot])
+  test "messages are threaded based on the original message", %{client: client} do
+    {:ok, reply} = ChatClient.chat_wait!(
+      client, [room: @ci_room,
+               message: "@#{@bot}: operable:echo messages are threaded based on the original message",
+               reply_from: @bot])
     refute reply.thread_ts == nil
   end
 
-  test "messages redirected to another room or dm are not threaded", %{user: user, client: client} do
-    user |> with_permission("operable:echo")
-    {:ok, reply} = ChatClient.chat_wait!(client, [room: @ci_room, message: "@#{@bot} operable:echo test > #ci_bot_redirect_tests", reply_from: @bot])
+  test "messages redirected to another room or dm are not threaded", %{client: client} do
+    {:ok, reply} = ChatClient.chat_wait!(
+      client, [room: @ci_room,
+               message: "@#{@bot}: operable:echo messages redirected to another room or dm are not threaded > #ci_bot_redirect_tests",
+               reply_from: @bot])
     assert reply.thread_ts == nil
   end
 end