jjh42 / mock

Mocking library for Elixir language
MIT License
639 stars 81 forks source link

Proposal: clearer calls assertions with pattern matching #116

Open eteeselink opened 4 years ago

eteeselink commented 4 years ago

Hi there,

First off, thanks for Mock, it's super nice. Now, I read #109 but I think I have a better solution. I wanted to discuss the API a bit before I turn it into a PR.

Basically, besides the lack of pattern matching there's something more that bothers me about assert_called: it asserts that the function has been called at least once. That means you don't catch bugs where a function is called too often. I'd prefer to be able to verify that a function is called exactly as many times as I expected, with exactly the parameters I care about.

The good news is, I think I wrote a macro that solves this:

  defmacro calls({{:., _, [mod, fun]}, _, args}) do
    quote do
      for {_pid, {_mod, unquote(fun), unquote(args) = params}, _return} <-
            :meck.history(unquote(mod)) do
        params
      end
    end
  end

It's my first Elixir macro so let me know if I did something stupid :-)

Call it like this:

# similar to `called`, except the parameters can be a pattern
calls = calls(MyModule.somefunc(:insert, %{type: "user"}))

This returns a list of all calls that were made to MyModule.somefunc with parameters that match the parameters shown. Eg if the software under test calls this:

MyModule.somefunc(:insert, %{type: "user", name: "George", age: 7})
MyModule.somefunc(:insert, %{type: "group", topic: "pimples"})
MyModule.somefunc(:update, %{type: "user", name: "Hans", age: 5})
MyModule.somefunc(:insert, %{type: "user", name: "Pete", age: 8})

then the calls call above returns this list, the parameters for each call:

[
   [:insert, %{type: "user", name: "George", age: 7}],
   [:insert, %{type: "user", name: "Pete", age: 8}]
]

Now, that's a lovely list for a bog standard assert, which allows pattern-matched assignments. Eg:

# this verifies that exactly 2 calls were made
assert [george, pete] = calls(MyModule.somefunc(:insert, %{type: "user"}))

# this verifies that the calls happened in the expected order and with the right data
assert [_, %{name: "George"}] = george
assert [_, %{name: "Pete"}] = pete

What do you think about this? I'll be happy to prepare a PR.

Note: personally I think I'd want to ship a second overload for the macro that accepts each part separately:

calls(MyModule, :somefunc, [:insert, %{type: "user"}])

This lets you specify a pattern for the function name too, eg calls(MyModule, _, _) gets all calls of any arity to (mocked) functions in MyModule.

If you like this approach I'll code that overload up as well and put them in the same PR. I'm also not 100% sure about the name calls. I considered get_calls too, and call_history, but they feel needlessly long, plus you got call_history in master already (even though I feel like this macro would essentially supersede it).

The final thing I'm not sure about is that this macro returns the parameter list, but not the return values. I don't think return values are often useful when asserting mock behavior, but maybe sometimes they are (esp when using :passthrough). Maybe it should return a defstruct [:params, :return_value] instead of just the params.

Curious about your thoughts!

eteeselink commented 4 years ago

@Olshansk any thoughts? I'd be happy to prepare a PR (incl proper documentation etc) but I prefer not to do the work if you don't like the API.

Olshansk commented 4 years ago

Hey, sorry for the delay on this...

Overall, I like the proposal and think we should move forward with it.

That means you don't catch bugs where a function is called too often

Just noteworthy: Erlang's meck (what this uses) has a num_calls function. In a separate PR, we could build a wrapper around it to expose it in an elixir native way.

If you like this approach I'll code that overload up as well and put them in the same PR. I'm also not 100% sure about the name calls. I considered get_calls too, and call_history, but they feel needlessly long, plus you got call_history in master already (even though I feel like this macro would essentially supersede it).

Let's go with matching_calls. What do you think?

The final thing I'm not sure about is that this macro returns the parameter list, but not the return values

Let's avoid looking at the return values for the purpose of this feature.