Repository: edgurgel/mimic
Pull request #20 (2020-09-23 05:21:30)

add `verify!()` and `verify_on_exit!()` to readme on `expect` section
Adding `verify!()`. and `verify_on_exit!()` to readme on `expect` section,
both are useful in actually verifying that expected function is called, but it is hard to know you need to call them because it's not mentioned in readme.
![image](https://user-images.githubusercontent.com/50098236/93969831-012f7d80-fd97-11ea-82ea-7ba76f05b09c.png)


edgurgel: If you `use Mimic` then there's no need to call verify on exit:

https://github.com/edgurgel/mimic/blob/master/lib/mimic.ex#L92
calvin-kargo: oh, wow, i didn't notice that Mimic actually auto verify expectation, i have been using `use Mimic` as suggested though.

After some researching, i found out that Mimic auto verify seems to has some issue with `DataCase` in my setup (https://hexdocs.pm/phoenix/testing.html#content)

for example code:
```elixir
defmodule TestDummy do
  use LoadService.DataCase
  use Mimic
  alias LoadService.V2.Oban

  describe "test Oban.Notification" do
    test "publish_message" do
      expect(Oban.Notification, :publish_message, fn attr ->
        assert attr.payload.payment_amount == "Rp 300.000"
        assert attr.event_type == "guaranteed_job_second_payment_reminder_s"

        {:ok, "some job"}
      end)
    end
  end
end
```

would return success (should fail)
```
Finished in 0.07 seconds
1 test, 0 failures
```

but changing `use LoadService.DataCase` to `use ExUnit.Case` make the test fail as expected
```
  1) test test Oban.Notification publish_message (TestDummy)
     test/load_service/v2/sqs/processor_guaranteed_delivery_order_status_update_copy.exs:7
     ** (Mimic.VerificationError) error while verifying mocks for #PID<0.976.0>:
     
       * expected LoadService.V2.Oban.Notification.publish_message/1 to be invoked 1 time(s) but it has been called 0 time(s)
``` 

`LoadService.DataCase` is auto-generated phoenix data case
```elixir
defmodule LoadService.DataCase do
  @moduledoc false

  # This module defines the setup for tests requiring
  # access to the application's data layer.

  # You may define functions here to be used as helpers in
  # your tests.

  # Finally, if the test case interacts with the database,
  # it cannot be async. For this reason, every test runs
  # inside a transaction which is reset at the beginning
  # of the test unless the test case is marked as async.

  use ExUnit.CaseTemplate

  using do
    quote do
      alias LoadService.Repo

      import Ecto
      import Ecto.Changeset
      import Ecto.Query
      import LoadService.DataCase
    end
  end

  setup tags do
    :ok = Ecto.Adapters.SQL.Sandbox.checkout(LoadService.Repo)

    unless tags[:async] do
      Ecto.Adapters.SQL.Sandbox.mode(LoadService.Repo, {:shared, self()})
    end

    Mimic.stub(LoadService.AWSImage, :upload_image_multipart, fn _ -> {:ok, "path/to/some/file.png"} end)
    Mimic.stub(LoadService.AWSImage, :get_image_url, fn _ -> "https://path/to/some/file.png" end)
    Mimic.stub(LoadService.Utils.SnsUtil, :publish_sns_message, fn _, _ -> {:ok, %{}} end)
    Mimic.stub(LoadService.Utils.SnsUtil, :publish_sns_message, fn _, _, _ -> {:ok, %{}} end)
    Mimic.stub(LoadService.SearchService.Sqs, :update_search_service, fn data -> data end)

    :ok
  end

  @doc """
  A helper that transforms changeset errors into a map of messages.

      assert {:error, changeset} = Accounts.create_user(%{password: "short"})
      assert "password is too short" in errors_on(changeset).password
      assert %{password: ["password is too short"]} = errors_on(changeset)

  """
  def errors_on(changeset) do
    Ecto.Changeset.traverse_errors(changeset, fn {message, opts} ->
      Enum.reduce(opts, message, fn {key, value}, acc ->
        String.replace(acc, "%{#{key}}", to_string(value))
      end)
    end)
  end
end
```
some more investigation and it seems this is caused by calling `Mimic.stub` in setup block `DataCase`, what is the correct in thing in this case?
edgurgel: Interesting! It might be a bug then 🤔. Do you think you can turn this into a smaller reproducible case that we can try out?
calvin-kargo: @edgurgel  i have just update the PR with the anomaly not auto verified test case, you could try it by
`mix test test/test_expect.exs` which should be failing (but not)
calvin-kargo: i investigate a bit of what's probably causing this issue, and i think i found out the root cause of the issue.
1. During `stub` or `expect`, `Mimic.Server` is going to setup `Process.Monitor` if it's not `verify_on_exit` mode
```elixir
  def handle_call({:stub, module, fn_name, func, arity, owner}, _from, state) do
    if valid_mode?(state, owner) do
      monitor_if_not_verify_on_exit(owner, state.verify_on_exit)
```
2. During message :DOWN from `Process.Monitor`, `Mimic.Server` is going to delete all expectation data for downed process PID
```elixir
  def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do
    {:noreply, clear_data_from_pid(pid, state)}
  end
```
3. :DOWN message come first before `Callbacks.on_exit` which call `verify!`, so when `verify!` is called, expectation data for process become empty

i also have updated the PR with what i think is the correct fixes, which is checking on :DOWN to see if it's a verify_on_exit process or not before deleting expectation data.
```elixir
  def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do
    new_state =
      if MapSet.member?(state.verify_on_exit, pid) do
        state
      else
        clear_data_from_pid(pid, state)
      end

    {:noreply, new_state}
  end
```
edgurgel: Nice! That's really good finding. Now I think we need to find a way to test this out and get a green test that covers this 🤔 

I'm not sure we can write a nice test for this given that we can't call `ExUnit.Callbacks.on_exit` outside of the testing process 🤔 

Maybe we should just test simulate this? New test file without calling `use Mimic`

* Spawn a process
* Call  `Mimic.stub`
* Call `Server.verify_on_exit`
* Stop the process
* Call `Mimic.verify!` and assert that an error was raised? 

What do you think?

BTW we need to rename the `test/test_expect.exs` file otherwise `mix test` wont run it. I think it must end with `_test.exs`



calvin-kargo: Hmm, i agree that we can't easily test `verify_on_exit!` as it is because it depend on `ExUnit.Callbacks.on_exit`.

Let me try making simulation for it
calvin-kargo: @edgurgel , i have just added simulation test for the bug and update readme regarding `verify!`.

However, during writing test and PR, CI build was failing intermittently for the new test, especially during `mix test --cover`.
```12:15:04.619 [error] Process #PID<0.374.0> raised an exception
** (ArgumentError) Stub cannot be called by the current process. Only the global owner is allowed.
    (mimic 1.3.0) lib/mimic.ex:449: Mimic.validate_server_response/2
    test/edge_case_test.exs:11: anonymous fn/1 in Mimic.EdgeCase."test should verify_on_exit! correctly even when stub is called before (simulation test)"/1
```

After some investigation, it seems somehow, sometime, `Mimic.Server` start by global mode during test start. I am guessing that it because previous test case runned is in global mode? wdyt, is this expected behaviour?
```
%Mimic.Server.State{
  expectations: %{},
  global_pid: #PID<0.370.0>,
  mode: :global,
  modules_beam: %{
    Calculator => {'/home/calvin/open_source/mimic/_build/test/lib/mimic/ebin/Elixir.Calculator.beam',
     "/home/calvin/open_source/mimic/Elixir.Calculator-29940.coverdata"},
    Counter => {'/home/calvin/open_source/mimic/_build/test/lib/mimic/ebin/Elixir.Counter.beam',
     "/home/calvin/open_source/mimic/Elixir.Counter-29940.coverdata"}
  },
  stubs: %{},
  verify_on_exit: #MapSet<[#PID<0.272.0>, #PID<0.304.0>, #PID<0.329.0>,
   #PID<0.337.0>, #PID<0.341.0>, #PID<0.345.0>, #PID<0.349.0>]>
}
```
edgurgel: Yeah good point. It's the expected behaviour. Each test case should set their expected mode. We could think about adding:

```elixir
setup :set_mimic_from_context
```

when people call `use Mimic` ?


`set_mimic_from_context` will set private or global based on `async`


Thanks for the PR! I'm going to check it once again later today and release a new version! Really happy with the final result 🎉 
calvin-kargo: >when people call use Mimic ?

hmm, i actually never use Mimic in global mode, so can't really say about it. 
But if we call `set_mimic_from_context` on `use Mimic`, how would this effect calling `stub` in `ExUnit.CaseTemplate`? i think from setting mimic mode should be done before calling `stub` in `ExUnit.CaseTemplate` so that there is no error due to test starting in global context?

Patch:
diff --git a/README.md b/README.md
index a149e6f..f8e36e3 100644
--- a/README.md
+++ b/README.md
@@ -96,6 +96,19 @@ assert Calculator.add(1, 3) == {:add, 1, 3}
 assert Calculator.add(4, 5) == {:add, 4, 5}
 ```
 
+With `use Mimic`, verification `expect/4` function call of is done automatically on test case end. `verify!/1` can be used in case custom verification timing required:
+
+```elixir
+Calculator
+|> expect(:add, 2, fn x, y -> {:add, x, y} end)
+
+# Will raise error because Calculator.add is not called
+# ** (Mimic.VerificationError) error while verifying mocks for #PID<0.3182.0>:
+#   * expected Calculator.add/2 to be invoked 1 time(s) but it has been called 0 time(s)
+verify!()
+```
+
+
 ### Reject
 
 One may want to reject calls to a specific function. `reject/1` can be used to achieved this behaviour.
diff --git a/lib/mimic/server.ex b/lib/mimic/server.ex
index f4270e9..20927c2 100644
--- a/lib/mimic/server.ex
+++ b/lib/mimic/server.ex
@@ -144,7 +144,14 @@ defmodule Mimic.Server do
   end
 
   def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do
-    {:noreply, clear_data_from_pid(pid, state)}
+    new_state =
+      if MapSet.member?(state.verify_on_exit, pid) do
+        state
+      else
+        clear_data_from_pid(pid, state)
+      end
+
+    {:noreply, new_state}
   end
 
   def handle_info(msg, state) do


Test patch:
diff --git a/test/edge_case_test.exs b/test/edge_case_test.exs
new file mode 100644
index 0000000..7648a5c
--- /dev/null
+++ b/test/edge_case_test.exs
@@ -0,0 +1,24 @@
+defmodule Mimic.EdgeCase do
+  use ExUnit.Case
+  import Mimic
+
+  describe "auto verification" do
+    test "should verify_on_exit! correctly even when stub is called before (simulation test)" do
+      set_mimic_private()
+      parent_pid = self()
+
+      spawn_link(fn ->
+        stub(Calculator, :add, fn _, _ -> 3 end)
+        Mimic.Server.verify_on_exit(self())
+        expect(Calculator, :add, fn _, _ -> 3 end)
+        send(parent_pid, {:ok, self()})
+      end)
+
+      assert_receive({:ok, child_pid})
+
+      assert_raise Mimic.VerificationError, fn ->
+        verify!(child_pid)
+      end
+    end
+  end
+end