Implement mox #10
Implement mox #10
Conversation
| @@ -40,7 +7,8 @@ defmodule Spacesuit.SessionService do | |||
| require Logger | |||
| use Elixometer | |||
|
|
|||
| @behaviour SessionService | |||
| @callback validate_api_token(String.t()) :: Tuple.t() | |||
There was a problem hiding this comment.
When the only reason for a behaviour is for testing, it's pretty common (and acceptable) to move the callbacks/behaviour definition into the module that implements it in prod.
There was a problem hiding this comment.
This is super useful because I didn't know about Mox and so I'll definitely check it out. I'm a little lost about how it works ATM moment but I'm sure some reading will clear it up.
In this case we only had one implementation and a mock, but this module is not that generic and the idea was that people could provide their own implementation of a SessionService if they needed to handle it differently. Given that I'd like to support that, but that also nobody else seems to be interested, I'm on the fence about merging.
There was a problem hiding this comment.
Ah, not a problem. I will update the PR to leave the callback in the SessionService behaviour. That is technically a cleaner implementation and definitely the way to go if you want the interface behaviour for more than just testing. Mox will work just fine that way, too.
| @@ -63,15 +31,13 @@ defmodule Spacesuit.SessionService do | |||
| {:ok, _, _} -> | |||
| result | |||
|
|
|||
| {:error, type, code, error} -> | |||
| {:error, type, code, error} when is_binary(error) -> | |||
There was a problem hiding this comment.
Adding the guard clause allows us to treat this as a separate case instead of nesting flow control.
| @@ -139,7 +105,8 @@ defmodule Spacesuit.SessionService do | |||
| {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> | |||
| {:ok, body} | |||
|
|
|||
| {:ok, %HTTPoison.Response{status_code: code, body: body}} when code >= 400 and code <= 499 -> | |||
| {:ok, %HTTPoison.Response{status_code: code, body: body}} | |||
| when code >= 400 and code <= 499 -> | |||
There was a problem hiding this comment.
sorry. This is from the 1.6.4 formatter. I can remove it if you want, but I recommend updating and starting to use the formatter.
| :elixometer | ||
| ], | ||
| mod: {Spacesuit, []} | ||
| mod: {Spacesuit, []}, |
There was a problem hiding this comment.
As of 1.4(?) you no longer want to explicitly pass applications that need to be started. While this kind of sucks since it takes away the explicitness, it means that you can't mess up by forgetting to add a new application to the list when you start using it. In my case, I needed to clean this up or mox wasn't getting started.
Make tests more explicit.
| @@ -1,91 +1,167 @@ | |||
| defmodule SpacesuitAuthMiddlewareTest do | |||
| use ExUnit.Case | |||
| import Mox | |||
There was a problem hiding this comment.
importing gives us mock and stub as local functions. If you prefer it to be more explicit, you can require Mox and the calls will be Mox.mock and Mox.stub.
Mox is a mocking library created by Jose Valim. It's coolest feature is that it breaks if you try to mock something that doesn't have a behavior or if you try to mock a function that doesn't exist. It helps things be a little more readable by allowing you to define the functions in the test themselves, so everything is a little less mysterious.
You could move the mock definition (with multiple clauses for the anonymous functions) up into a setup block. I prefer to be more explicit. Additionally, you can mock (set an expectation of a call received) the calls with the library, but, in this case, they are queries, not commands, so I left them as stubs.
I've switched the MockSessionService to use that library.