ihumanable/patch

Ergonomic Mocking for Elixir

patch's Issues

assert_called matching is incompatible with meck and assert_receive

Meck provided some degree of pattern matching when using called/3.

assert_receive/1 allows for normal pattern matching and performs non-hygienic binding.

Patch.assert_called (and associated functions) should use pattern matching instead of equality checking to provide greater utility for the test author.

Performance: Investigate persistent patching as a performance boost.

When a test patches multiple modules with many test cases it can lead to poor performance.

The current lifecycle of patching looks like this.

1. On setup start a Patch.Supervisor with start_supervised which in turn starts the Patch.Listener.Supervisor and Patch.Mock.Supervisor
2. For each module that gets patched, start a Patch.Mock.Server GenServer supervised by the Patch.Mock.Supervisor and then generate the Delegate, Facade and Original modules.
3. On exit the Patch.Supervisor exits and shuts down the Patch.Listener.Supervisor and the Patch.Mock.Supervisor.
4. The Patch.Mock.Supervisor in turn shuts down all the Patch.Mock.Server GenServers.
5. Each Patch.Mock.Server GenServer has a terminate callback which restores the original module by purging the Delegate, Facade, and Original modules and recompiling the true original module.

The above can be quite time intensive, step 2 rewrite the abstract form for the module multiple times, for each patched module there are 4 modules compiled.

Possible higher performance strategy.

1. Either globally or on setup_all start the Patch.Supervisor
2. Allow patching to happen normally
3. In setup register an on_exit which resets the internal state of the mock servers (unregister all mocks and reset the history)

This would mean that modules are only recompiled once either during the entire test run or at least a single test case.

Give authors more control over imports

Patch wants to own various symbols in a test but these can conflict with symbols the author might be importing either directly or indirectly.

A good example is that Phoenix would like to import patch from Phoenix.Router, this directly conflicts with Patch.patch

Create some mechanisms that allows the author to have greater control over the imports.

Patching GenServer breaks Patch

Since Patch uses GenServer internally, patching GenServer actually causes a deadlock inside of Patch.Mock.

This limitation should either be documented or removed.

Patch GenServer :handle/2 - Idea / question?

Hello I have some GenServer that subscribe to Phoenix.PubSub channel to reduce code coupling

I feel like something is missing Patch (that is really nice btw).

My workflow is everytime a character is created, I broadcast a message that will be catch by some GenServers to do their shit (e.g. send welcome email, push discord msg, or log the event in a db or w.e)

I want to test this but because it's a PhoenixPubSub event, it's not easily testable. I end with the following test (that works great).

defmodule ForHonor.Feed.AdventureEventSubscriberTest do
  use Patch
  #...

  test "on character created", %{character: character} do
    patch_handle_info(AdventureEventSubscriber)

    msg = {:character_created, character}
    Phoenix.PubSub.broadcast(ForHonor.PubSub, "game:events", msg)

    assert_handle_info(AdventureEventSubscriber)

    # test if side effect was apply or w.e
  end


  defp patch_handle_info(module) do
    pid = self()

    patch(module, :handle_info, fn msg, state ->
      real(module).handle_info(msg, state)
      |> tap(fn _ -> send(pid, module) end)
    end)
  end

  defp assert_handle_info(module, timeout \\ :timer.seconds(5)) do
    receive do
      ^module -> assert true
    after
      timeout -> assert false
    end
  end
end

I don't have a global overview of patch, maybe I miss something. I would like to have your opinion on the previous code. If their is no other way to accomplish that, are you interested by a pull request ?

Thanks a lot ;)

Passthrough behavior for callables

Right now when you patch a function with a callable there are two cases where the callable can fail.

BadArity

This only occurs with apply dispatched callables. It happens when the caller calls the function with a different number of arguments than the callable accepts

FunctionClauseError

This can occur in both apply and list dispatched callables. It happens when the callable performs some matching or guarding and the caller's call fails to match.

Solutions

There are a number of solutions.

Do Nothing

This behavior could be considered correct. If the test author wants the function to behave as though it has a match or guard, then that's how the function should behave.

The major downside is that this does not allow the author to call the original function. Since the original function will have it's local calls rewritten to be remote Delegate calls, this introduces the high likelihood of GenServer deadlock if the callable directly calls the original function.

Provide a Signaling Sentinel

Another option would be to allow the test author to opt-in to this behavior by returning a sentinel value that indicates the original function should be used.

Example

patch(SomeModule, :some_function, fn 
  %WhenCalledWithThisStructReturnPatchedValue{} ->
    :patched

  _any_other_value ->
    use_original_value()
end)

The major drawback to this is that it is hard to discover. You have to get bitten by the non-passthrough behavior and then discover that you can create a new clause in your callable and return the magic sentinel for it to behave correctly.

Change the Server's behavior.

The last option is to make it so that the Server / Callable behavior is updated in such a way that a rescued BadArityError or FunctionClauseError itself acts as the signal to use the original value.

Example

defmodule SomeModule do
  def some_function(a) do
    {:original, a}
  end
end

patch(SomeModule, :some_function, fn 
  a when is_integer(a) ->
    :patched
end)

assert SomeModule.some_function(1) == :patched
assert SomeModule.some_function(:example) == {:original, :example}

This is very nice from an ergonomics point of view, but has the downside of being a bit magical and it makes it easy for a test author to think they've patched a function but the real function is called.

Decision

Based on the solutions above, solution 3 (Change the Server's behavior) is most desirable. It provides a high degree of ergonomics and aligns with the other implicit pasthrough design decisions of the library.

Using `Ecto.Changeset.cast/4` within a patch causes an error

If you try to call Ecto.Changeset.cast inside of a patched function, it raises an ** (EXIT) process attempted to call itself

Minimum reproduction:

defmodule ToBePatched do
 use Ecto.Schema

 embedded_schema do
   field :foo, :string
 end
 
 def changeset(arg1, arg2) do
   Ecto.Changeset.cast(arg1, arg2, [:foo])
 end
end

defmodule ToBePatchedTest do
  use ExUnit.Case
  use Patch
  
  test "this shouldn't fail" do
    patch(ToBePatched, :changeset, fn arg1, arg2 ->
      Ecto.Changeset.cast(arg1, arg2, [:foo})
    end)
    
    ToBePatched.changeset(%ToBePatched{}, %{})
  end
end

Combine :sys.get_state + listen + inject

Fairly common scenario to want to listen to a pid that some other process is holding. This results in code like this following

state = :sys.get_state(target)
{:ok, listener} = listen(:subject, state.subject_pid)
inject(target, :subject_pid, listener)

It would be nicer to be able to just do

{:ok, listener} = listen(:subject, target, [:subject_pid])

This conflicts though with the existing listen/3 which accepts options that can control whether or not replies should be captured and how long to wait for timeout.

Option 1: Use different types to change behavior on listen/2,3

Since Keyword.t() can be distinguished from [atom] via Keyword.keyword?/1 the following could be implemented.

# Spawn a listener that forwards to the pid
listen(:tag, pid)

# Spawn a listener that forwards to the pid with some options
listen(:tag, pid, capture_replies: false)

# Spawn a listener to the pid in the state of the pid given and inject it into said state
listen(:tag, pid, [:target])

# Spawn a listener to the pid in the state of the pid given and inject it into said state with some options
listen(:tag, pid, [:target], capture_replies: false)

Option 2: Rename the current listen/2,3

Once the current one is renamed then the listen/3,4 function would encode this common pattern.

listen was originally called intercept during development, but the definition of intercept could imply that the message won't be delivered. forward seems like an ok alternative, clearly indicates that it will forward the messages but doesn't as clearly indicate that it will also send it to the test process.

Option 3: Rename the current inject/3

Once the current one is renamed then the inject/3,4 function would encode this common pattern.

replace is an option since it is a helper around :sys.replace_state.

Option 4: Choose another name for this operation

listen_and_inject/3,4 is an option but unwieldy.

Some other random options in no particular order:

• bug (not great, overloaded term)
• tap (will be a new Kernel symbol in 1.13+)
• snoop (likely too cute)
• track (possibility)
• watch (possibility)

Decision

Option 3 provides the best improvements even though it's a breaking change.

inject/3,4 better describes this common pattern while replace/3 still adequately describes the behavior of the current inject/3 while connecting more closely with its mechanism, :sys.replace_state/2 which may make it more obvious for some authors.

Given that both of these are improvements over the current incarnation, this breaking change should likely be adopted.

Call will happen assertions / refutations.

ExUnit has assert_receive/3 and assert_received/2.

assert_received/2 asserts that a message is in the message queue.

assert_receive/3 asserts that a message is in the message queue already or will be delivered to the message queue within a given deadline.

Patch has assert_called/1,2 which like assert_received/2 asserts that a call has been observed.

This enhancement would add assert_call/2,3 which would assert that a call has been observed or will be observed within the given deadline.

These present-tense versions could exist for all the call assertions and could perhaps be built on a general use utility function.

modules using patch are excluded from coverage report

Something I noticed is that when adding use patch with a test file, that file and corresponding implementation file are removed from the coverage report when invoking with mix test --cover.

Patch.Macro map key support

Does Patch.Macro not support map keys properly?

key = "key"
value = "value"

assert_called Example.example(%{^key => ^value})

Unverified report that this might not work, check it out.

Stacked Callables

This enhancement targets the somewhat clumsy way multiple arities are currently implemented and builds on the improvement introduced in #30

Problem

Patching a function with multiple arities is possible but quite clunky. Consider the following module.

defmodule Example do
  def example(a) do
    {:original, a}
  end

  def example(a, b) do
    {:original, a, b}
  end
end

This module has two functions example/1 and example/2. This is the current best practice for patching these functions.

patch(Example, :example, callable(fn 
  [a] ->
    {:patched, a}

  [a, b] ->
    {:patched, a, b}
end, :list)

This is workable but it's not very ergonomic or discoverable. Test authors must be aware of 2 features of Patch to be able to make the leap from "I want to patch multiple arities" to "Oh, I should make a list dispatched callable!"

1. Test authors must realize that functions are being promoted up to callables in the background
2. Test authors must realize that callables have dispatch modes and that the list dispatch mode can be used for this use case.

Solution

"Stacked Callables" is a new idea that might solve this issue. Here's the ideal version of the test code

patch(Example, :example, fn a -> {:patched, a} end)
patch(Example, :example, fn a, b -> {:patched, a, b} end)

This code would not work correctly today because :example has a single patch value, the last one it was called with.

To make this work, Patch would introduce stacking to the callables.

Patching :: Functional Specification

How should StackedCallables work?

The necessary precondition for StackedCallables is when a test author patches a function with a callable. There are 4 states the patched function can be in.

1. Unpatched - In this case a simple callable is used, behavior is identical to today.
2. Patched with a non-callable - In this case a simple callable replaces the previous value, behavior is identical to today.
3. Patched with a callable - The new callable and the existing callable are combined into a StackedCallable
4. Patched with a StackedCallable - The new callable is placed on top of the StackedCallable.

Value Resolution :: Functional Specification

How do StackedCallables resolve?

StackedCallables maintain a stack of callables, with the oldest callable at the bottom and the newest callable at the top.

When the StackedCallable needs to resolve to a value it begins at the top of the stack and calls each callable in turn with Patch.Apply.safe/2 if the safe application returns :error the next callable is tried until a callable returns a value. If no functions return a value then passthrough behavior should be executed.

This resolution order means that the following code evaluates sensibly

patch(Example, :example, fn a -> {:first, a} end)
assert Example.example(1) == {:first, 1}

patch(Example, :example, fn a -> {:second, a} end)
assert Example.example(1) == {:second, 1}

It also has some less obvious behaviors, which are semi-reasonable

patch(Example, :example, fn 1, b -> {:first, b} end)
patch(Example, :example, fn a, 2 -> {:second, a} end)

# Only matches the first patch
assert Example.example(1, :b) == {:first, :b}

# Only matches the second patch
assert Example.example(:a, 2) == {:second, :a}

# Matches both the first and second patch (newest patch wins)
assert Example.example(1, 2) == {:second, 1}

# Matches neither patch (passthrough behavior)
assert Example.example(:a, :b) == {:original, :a, :b}

Cross Reference `expose/2` and `private/1`

Common issue when using expose/2 is that the compiler starts producing nuisance warnings. This is because as far as the compiler is concerned you are calling a function that doesn't exists, since the exposure happens at runtime.

There's already a solution in the private/1 macro but these two functions don't formally cross-reference each other.

• Add cross-references so test authors using expose/2 know about private/1 and vice versa.
• The super-powers doc should also be updated to show correct usage
• This is a good candidate for adding to the guide book.