elixir-tesla / tesla

The flexible HTTP client library for Elixir, with support for middleware and multiple adapters.
MIT License
2k stars 342 forks source link

Deprecate Tesla.Mock #241

Open teamon opened 6 years ago

teamon commented 6 years ago

Currently Tesla.Mock is implemented as adapter with special interface to intercept requests and provide mocked responses. It has a few drawbacks, like for example being a single instance - if you want to mock multiple clients you still have to use single Tesla.Mock.mock fn -> ... block (#238). It does not verify if the requests were actually called or not. Under the hood it uses very similar tricks as mox. Mox requires a behaviour to be able to provide expectations. And actually I can be used instead of Tesla.Mock right now.

# define as many mock adapters as you want
Mox.defmock(MyApp.NiceApi.Mock, for: Tesla.Adapter)
Mox.defmock(MyApp.EvilApi.Mock, for: Tesla.Adapter)

# config/test.exs
config :tesla, MyApp.NiceApi, adapter: MyApp.NiceApi.Mock
config :tesla, MyApp.EvilApi, adapter: MyApp.EvilApi.Mock

# test!
MyApp.NiceApi.Mock
|> expect(:call, fn
  %{url: "http://github.com"}, _opts ->
    {:ok, %Tesla.Env{status: 200, body: "ok"}}

  %{url: "http://example.com"}, _opts ->
    {:ok, %Tesla.Env{status: 500, body: "oups"}}
end)

MyApp.EvailApi.Mock
|> expect(:call, fn %{url: "http://noop"}, _opts ->
  {:error, :econnrefused}
end)

I am currently using this method in one project, so far with success.

We could keep helper functions like Tesla.Mock.json/* but we could probably remove the underlying mock machinery and promote using something like mox.

amatalai commented 6 years ago

We could keep helper functions like Tesla.Mock.json/* but

Is there any value in it? One can simply call encode/* from json lib of choice.

teamon commented 6 years ago

The value is

json(%{"some" => "data"})

instead of


%Tesla.Env{status: 200, headers: [{"content-type", "application/json"}], body: Jason.encode!(%{"some" => "data"})}
amatalai commented 6 years ago

Ah, ok. I have never used it before πŸ˜…

victorolinasc commented 6 years ago

I'll test this approach thanks for posting it here.

I have intermittent errors with Tesla.Mock like this:

 ** (exit) exited in: GenServer.call(#PID<0.562.0>, {:update, #Function<2.12978969/1 in Tesla.Mock.agent_set/1>}, 5000)
         ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
     stacktrace:
       (elixir) lib/gen_server.ex:924: GenServer.call/3
teamon commented 6 years ago

@victorolinasc Thanks! I can’t wait to hear some feedback.

Please post your issues with current Tesla.Mock as a separate issue, together with code sample (let’s keep this issue focused on deprecation)

ivan-kolmychek commented 5 years ago

I do find the Tesla.Mock.json() helpful, and I actually came here looking is there an issue to document json_engine option on on it - I had to look into source code for it, since we use Poison and don't have Jason installed at all.

As for Mox vs Tesla.Mock - we already use Mox in those few places where we decided that we actually need mocks, except that we used Tesla.Mock to mock HTTP requests so far, since no one noticed that there was a Tesla.Adapter behavior. :)

So hopefully I can try it out tomorrow or on Friday and provide some feedback.

ivan-kolmychek commented 5 years ago

I have tried out approach with Mox briefly and it 0) successfully replaces Tesla.Mock (with small differences that @teamon has in original post) and 1) behaves pretty much as you would expect a Mox mock to behave. :)

Unless we will find some problem that I have not stumbled upon while playing around with it, we would probably prefer using Mox instead of Tesla.Mock from now on, since (as I have already said) we already use Mox in other places.

@teamon thank you for this nice suggestion. :)

UPD: almost forgot, I still find the Tesla.Mock.json() useful in this situation, so I personally would suggest to keep it.

teamon commented 5 years ago

@ivan-kolmychek Thank you for trying this out, your feedback is very valuable πŸ‘

My biggest concern is the verbosity of expect(:call, ) - you need to remembers that the call/2 take two params (env & usually unused adapter opts), the return value must be full %Tesla.Env{..} and the need to match on full URL (instead of e.g. path).

Could you share some examples how do you use it?

ivan-kolmychek commented 5 years ago

@teamon I have tried it pretty much how you described.

Our use-case is relatively simple, so for us it's not that much more verbose than Tesla.Mock is - right now we have isolated requests in their own modules and test them in isolation, and then mock the components. So we don't really need to pattern-match request, but thinking about it now - yes, matching by the path sounds useful, especially since we randomize base URL in tests. Maybe we can have path as separate field on request, alongside full URL?

I can't post the actual code, so here is a short and "sanitized" version:

In test helper:

...
alias ProjectName.TestHelpers
...

Mox.defmock(ProjectName.TeslaMock, for: Tesla.Adapter)

# we set up random strings for most config params as a quick and
# dirty way to check that requests do look at configuration. 
Application.put_env(:project_name, ProjectName.BlahRequests,
  adapter: ProjectName.TeslaMock,
  base_url: "http://localhost/#{TestHelpers.random_string()}",
  token: TestHelpers.random_string(),
  user: TestHelpers.random_string(),
  password: TestHelpers.random_string()
)

Application.put_env(:tesla, Tesla.Mock, json_engine: Poison)
...

Test:

defmodule ProjectName.BlahRequests.UpdateSomeResourceTest do
  use ProjectName.DataCase

  ... # import internal helpers and such,

 test "perform/2 issues request to api" do
    config = Application.get_env(...)

    ProjectName.TeslaMock
    |> Mox.expect(:call, fn
     request, _opts ->
       ... # assertions on request here, including url, headers, body and such, using values from config.

       {:ok, Tesla.Mock.json(%{...}, status: 200)}
    end)

    assert {:ok, result} = ProjectName.BlahRequests.UpdateSomeResource.perform(...)

    ... # assertions on result
  end

  ... # other cases and test-specific helpers
end

I am pretty sure we can improve quite a few things, but it does the job for now. :)

polmiro commented 5 years ago

I'll test this approach thanks for posting it here.

I have intermittent errors with Tesla.Mock like this:

 ** (exit) exited in: GenServer.call(#PID<0.562.0>, {:update, #Function<2.12978969/1 in Tesla.Mock.agent_set/1>}, 5000)
         ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
     stacktrace:
       (elixir) lib/gen_server.ex:924: GenServer.call/3

@victorolinasc did you ever fix or open a new thread in regards to this error?

teamon commented 5 years ago

@polmiro Check #157 or use mock_global/2

smaximov commented 5 years ago

I found this issue when I had been looking for a way to test my app's behavior when Tesla.request/2 returns {:error, any()} which seems to be impossible with Tesla.Mock. I tried Mox as suggested by @teamon and it worked like a charm! Another reason to use Mox is that you can verify whether (or not) the mock has been called during a test.

ivan-kolmychek commented 5 years ago

Using Mox also proves to be quite good for us because it's so universal.

For example, we use it now to test our emails that we send using Swoosh.

sobolevn commented 5 years ago

Here's the full example:

defmodule KiraTest.Usecases.QueueIssueFromNoteTest do
  use Kira.DataCase

  import Mox
  import KiraTest.Factory

  alias Kira.Projects.Services.Reactions.Providers.GitlabReaction
  alias Kira.Usecases.QueueIssueFromNote
  alias KiraTest.Projects.Services.Reactions.Providers.GitlabReaction.Mock

  describe "create issue command" do
    setup :verify_on_exit!

    setup do
      issue = insert(:issue)
      note_iid = 123

      Application.put_env(:tesla, :adapter, Mock)

      {:ok, issue: issue, note_iid: note_iid}
    end

    test "valid queue command", %{issue: issue, note_iid: note_iid} do
      issue_url = GitlabReaction.issue_reaction_url(issue)
      issue_note_url = GitlabReaction.issue_note_reaction_url(issue, note_iid)

      Mock
      |> expect(:call, fn
        %{method: :post, url: "https://gitlab.com/api/v4" <> ^issue_url},
        _opts ->
          {:ok, %Tesla.Env{status: 200}}

        %{method: :post, url: "https://gitlab.com/api/v4" <> ^issue_note_url},
        _opts ->
          {:ok, %Tesla.Env{status: 200}}
      end)

      {:ok, context} =
        QueueIssueFromNote.run(
          project_uid: issue.project.uid,
          issue_uid: issue.uid,
          note_text: "@kira-bot queue",
          note_iid: note_iid
        )

      assert context.entity.state == "queued"
    end
  end
end

test/test_helper.ex:

ExUnit.start()
Ecto.Adapters.SQL.Sandbox.mode(Kira.Repo, :manual)

# Mocking different things:
# https://hexdocs.pm/mox/Mox.html
# https://github.com/teamon/tesla/issues/241
Mox.defmock(
  KiraTest.Projects.Services.Reactions.Providers.GitlabReaction.Mock,
  for: Tesla.Adapter
)

Actual code:

defmodule Kira.Projects.Services.Reactions.Providers.GitlabReaction do
  @moduledoc """
  HTTP client to award emojies to all possible kinds of Gitlab content.

  Uses smart retry and timeout policy. Works syncronously.
  For async version use:

    Task.start(fn -> GitlabReaction.issue(some_issue) end)

  This is something this module should not care about.
  """

  use Tesla

  @reaction "ok_hand"
  @domain Application.get_env(:kira, :gitlab)[:domain]
  @api_url "#{@domain}/api/v4"
  @private_token Application.get_env(:kira, :gitlab)[:personal_token]

  plug Tesla.Middleware.BaseUrl, @api_url
  plug Tesla.Middleware.Headers, [{"Private-Token", @private_token}]
  plug Tesla.Middleware.Query, name: @reaction
  plug Tesla.Middleware.Timeout, timeout: 2000
  plug Tesla.Middleware.Retry

  # URL API:

  def issue_reaction_url(issue) do
    "#{issue_path(issue)}/award_emoji"
  end

  def issue_note_reaction_url(issue, note_iid) do
    "#{issue_path(issue)}/notes/#{note_iid}/award_emoji"
  end

  # HTTP API:

  def issue(issue) do
    post!(issue_reaction_url(issue), [])
  end

  def issue_note(issue, note_iid) do
    post!(issue_note_reaction_url(issue, note_iid), [])
  end

  # Private:

  defp issue_path(issue) do
    "/projects/#{issue.project.uid}/issues/#{issue.iid}"
  end
end
fusillicode commented 5 years ago

Hi @teamon sorry for the bother but I can't figure out the exact structure of you first example:

# define as many mock adapters as you want
Mox.defmock(MyApp.NiceApi.Mock, for: Tesla.Adapter)
Mox.defmock(MyApp.EvilApi.Mock, for: Tesla.Adapter)

# config/test.exs
config :tesla, MyApp.NiceApi, adapter: MyApp.NiceApi.Mock
config :tesla, MyApp.EvilApi, adapter: MyApp.EvilApi.Mock

# test!
MyApp.NiceApi.Mock
|> expect(:call, fn
  %{url: "http://github.com"}, _opts ->
    {:ok, %Tesla.Env{status: 200, body: "ok"}}

  %{url: "http://example.com"}, _opts ->
    {:ok, %Tesla.Env{status: 500, body: "oups"}}
end)

MyApp.EvailApi.Mock
|> expect(:call, fn %{url: "http://noop"}, _opts ->
  {:error, :econnrefused}
end)

I mean, where does these 2 lines live? πŸ€”:

# define as many mock adapters as you want
Mox.defmock(MyApp.NiceApi.Mock, for: Tesla.Adapter)
Mox.defmock(MyApp.EvilApi.Mock, for: Tesla.Adapter)

I'm asking because from the all the other examples I clearly see that the most common approach is to modify the env inside the specs setup with Application.put_env but in your case you're also mentioning these 2:

# config/test.exs
config :tesla, MyApp.NiceApi, adapter: MyApp.NiceApi.Mock
config :tesla, MyApp.EvilApi, adapter: MyApp.EvilApi.Mock

Just to give you a little bit of context I'm trying to test a module that relies on some other ones that wraps Tesla for doing HTTP requests. These wrapping modules are configured via env like this:

config :rest_clients, RestClients.ExternalService,
  adapter: RestClients.ExternalService.Mock,
  base_url: "http://localhost:8080/rest"

All the modules, the wrapping ones, and the one under test are "separated" apps inside an umbrella.

Sorry for if I've been vague but I'm a little bit confused about the whole env configs πŸ˜…

P.S: thanks a lot for you wonderful work and this incredible library πŸ™‡

teamon commented 5 years ago

@fusillicode See mox docs for instruction on how to setup mocks

fusillicode commented 5 years ago

@teamon thanks for your quick reply πŸ™‡

I've already looked at it but unfortunately it didn't help much :(

Anyway the specific thing that I can't understand is why everything seems to be mocked correctly (yes I know, I'm using a verb rather that a nown here) if I set the mock adapter for all the clients but not if I set it for my specific module (client), i.e:

# config/test.exs
use Mix.Config

config :rest_clients, RestClients.ExternalService,
  base_url: "http://localhost:8080/external-service"

config :tesla, RestClients.ExternalService, adapter: RestClients.ExternalService.Mock # <- this does not work

config :tesla, adapter: RestClients.ExternalService.Mock # <- this do work

Btw this is the snippet of the test that is failing due to the "wrong" mock setup:

...
setup_all do
    Mox.defmock(RestClients.ExternalService.Mock, for: Tesla.Adapter)
    :ok
  end

  test "successful and correct API response with no process found" do

    RestClients.ExternalService.Mock
    |> expect(:call, fn %{url: "http://localhost:8080/external-service/process-definition/dummy-id/start", method: :post}, _opts -> {:ok, %Tesla.Env{status: 200}} end)

    # just a bunch of assertions
...

P.S: sorry once again for the bother πŸ™

teamon commented 5 years ago

This line

config :tesla, RestClients.ExternalService, adapter: RestClients.ExternalService.Mock

only works if you use module-based clients (with use Tesla), otherwise you need to handle adapter configuration yourself. I can't tell any more without RestClients.ExternalService source.

Please do NOT comment in this issue and open a new one if you need any more help.

garthk commented 5 years ago

Would switching to Mox avoid the Mock.mock_global required when the call to Tesla is a couple Task.async/1 calls away from the actual test?

ivan-kolmychek commented 5 years ago

@garthk I'm not an expert, but I don't see how it would - by default Mox doesn't intercept/count calls from other processes, you need to explicitly allow it by passing the mock, pid of the current process and pid of the process that makes the call to Mox.allow/3.

To me it sounds like you may have to use Mox'es "global mode" unless you have a way to obtain the pid of process that's created by that Task.async().

I'm not sure how much in advance you have to do that, but my guess would be at least before the actual call to the mock happens.

I hope I'm wrong and someone else knows an easier way to achieve what you're looking for without using global mode.

teamon commented 5 years ago

You're right @ivan-kolmychek, if you have anything async you need to use global mode.

garthk commented 5 years ago

Doc notes for people finding this through a search:

In Elixir 1.8, Mox can peek at Process.get(:"$callers") to figure out which expectations to satisfy. That'll work fine as long as Task.async/1 and Task.async/3 are the only async mechanisms in play. If that's not always true, call Mox.set_mox_from_context/1 to use global mocking for all tests with async: false.

yordis commented 5 years ago

My 2 cents, don't remove the module.

I just realized the module exists for mocking because I was scanning the issues but I use Mox for all mocking scenarios.

But I still use Tesla.Mock.json from it

Here is a full example

defmodule Okta.TestSupport.Helpers do
  @moduledoc false
  import ExUnit.Assertions

  def mock_request(opts) do
    mock_request(base_url(), opts)
  end

  def mock_request(base_url, opts, callback \\ nil) do
    status = Keyword.get(opts, :status)
    path = Keyword.get(opts, :path)
    method = Keyword.get(opts, :method)
    query = Keyword.get(opts, :query, [])
    response = Keyword.get(opts, :response, %{})
    body = Keyword.get(opts, :body)

    Mox.expect(Okta.Tesla.Mock, :call, fn
      request, _opts ->
        assert(request.body == with_default_body(request, body))
        assert(request.method == method)
        assert(request.query == query)
        assert(request.url == base_url <> path)

        if callback do
          callback.()
        end

        {:ok, Tesla.Mock.json(response, status: status)}
    end)
  end

  def base_url do
    "https://dev-000000.okta.com"
  end

  def with_default_body(%{method: method}, nil) when method in [:post, :put, :patch] do
    ""
  end

  def with_default_body(_, body) do
    body
  end
end

in the test files

    alias Okta.TestSupport.Helpers
    # ..........
    Helpers.mock_request(
      path: "/api/v1/users",
      method: :post,
      query: [activate: true],
      body: Jason.encode!(%{profile: profile}),
      status: 200
    )
PragTob commented 4 years ago

:wave:

My couple of cents - I like Tesla.Mock and I also use it alongside Mox.

Basically I have some business logic that is supposed to send API notifications.

I use mox to verify that the Notification module has been called with the right data.

I then have separate tests for the Notification module where I use Tesla.Mock to make sure my API requests receive with all the HTTP stuff I expect and that I deal appropriately with responses.

What I could see is deprecating this and moving it to a separate package that depends on mox to remove the duplication in tricks pulled, but to me it actually really does provide value :crossed_fingers:

chulkilee commented 4 years ago

@PragTob

I then have separate tests for the Notification module where I use Tesla.Mock to make sure my API requests receive with all the HTTP stuff I expect and that I deal appropriately with responses.

I think this part should be done with actual HTTP request/response processing - including hitting tesla adopters. Doing with only Tesla.Mock is not testing the whole thing. For example, we've seen strange behavior or bug from some library or adapters - such as httpc...

For that reason, I think it's better to use bypass or similar tools, which spins up real HTTP server during tests, not Tesla.Mock, to test the actual HTTP interaction. See [ex_force test file] as an example.

PragTob commented 4 years ago

@chulkilee thanks for the input! I had actually assumed that Tesla.Mock went through all the adapters.

I usually use bypass for this kind of thing but was happy that Tesla provided a mock interface which meant one less dependency for me (Tesla.Mock just seemed like a slightly more convenient bypass to me as I didn't have to make the URL injectable) but I might have to revisit that decision then.

Thanks! :green_heart:

tomekowal commented 3 years ago

I like the idea of Tesla using Mox.

I know Mox and its quirks. E.g. allowances system for using it in spawned processes. I know its limitations, e.g. I can't mock stuff for global processes that are part of my supervision tree. Mox is a great, well-known abstraction.

When I saw Tesla.Mock, I needed to start looking through code to understand how it works. I was pleasantly surprised that it uses mechanisms similar to Mox. Then I wanted to open an issue to ask why it doesn't use Mox in the first place, and I've found this issue πŸ˜„ BTW, sorry for reviving the issue. I hope that my two cents will be useful.

Functions like Tesla.Mock.json are valuable. The solution I'd like is using Mox for its mechanisms but leave Tesla.Mock for its abstractions. The tradeoff is that users unfamiliar with Mox will have to learn it to use Tesla.Mock but users who already know Mox will feel right at home. It also feels right at home with Tesla's purpose. It provides an abstraction over other HTTP libraries so for testing it would provide an abstraction over Mox. E.g. Tesla.Mock.json could call expect from Mox under the hood. (or even Hammox for type safety)

yukster commented 3 years ago

If Tesla.Mock isn't going away it seems like it's worth making it fail if a request is mocked and not called. Would a PR to do so be welcome? Is there some technical reason why this would be hard/impossible?