Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tesla.Mock not compatible with Tesla.Middleware.Timeout #157

Closed
stefanchrobot opened this issue Jan 26, 2018 · 9 comments
Closed

Tesla.Mock not compatible with Tesla.Middleware.Timeout #157

stefanchrobot opened this issue Jan 26, 2018 · 9 comments

Comments

@stefanchrobot
Copy link

Tesla.Mock is not compatible with clients that use the Tesla.Middleware.Timeout middleware.

iex(1)> Application.put_env :tesla, :adapter, :mock
iex(2)> require Tesla
iex(3)> Tesla.Mock.mock(fn env -> %{env | status: 200} end)
iex(4)> client = Tesla.build_client([{Tesla.Middleware.Timeout, timeout: 2_000}])
iex(5)> Tesla.get(client, "/")
** (Tesla.Mock.Error) There is no mock set for process #PID<0.450.0>.
Use Tesla.Mock.mock/1 to mock HTTP requests.

See https://github.com/teamon/tesla#testing

    (tesla) lib/tesla/middleware/timeout.ex:60: Tesla.Middleware.Timeout.repass_error/1
    (tesla) lib/tesla/middleware/timeout.ex:35: Tesla.Middleware.Timeout.call/3

The reason is that Tesla.Mock looks for the mock in the current process' dictionary and the Timeout middleware runs the Tesla pipeline in a separate Task to be able to terminate it on timeout.

As a workaround, I'm going to disable the Timeout middleware in the test env, but this is kludgy. I'd rather have the mock support the Timeout middleware.

I see three ways to do this:

  1. very dirty: change the implementation of the Timeout middleware to copy the mock into the Task's process dictionary
  2. dirty: change the implementation of the Timeout middleware to keep the parent process pid in the process dictionary; the Tesla.Mock.pdict_get() will use that to look for the mock function in the parent's process dictionary
  3. clean: change the API of the mock spec so that it doesn't land in the process dictionary:
client = Tesla.Mock.mock(client, fn -> ... end)
module = Tesla.Mock.mock(MyApi, fn -> ... end)

The mock function would put the mock spec somewhere in the middlewares. Ideally, this would replace the current adapter with specced mock adapter - this has the added benefit that one no longer needs to set the :mock adapter in tests.

The last point would actually be great thing to do, since I have the following use case: I have a set of unit tests for my API module where I mock the adapter and verify it's behavior. But for all other tests I've built a fake implementation of the API which is started alongside the main app - this way the tests exercise the same code path as the production code. For that reason, I would strongly prefer solution 3.

Let me know which one works for you and I'll happily prepare a PR.

@teamon
Copy link
Member

teamon commented Jan 26, 2018

This is definitely a bug, thanks for reporting!

About the client/module approach:

Nevertheless, the issue with timeout & mock is valid and needs to be somehow fixed.

@unthought
Copy link

unthought commented Feb 14, 2018

@stefanchrobot I would actually prefer option 1, personally.

Option 2. makes following the behavior quite murky for no reason, and you actually really only need to fix the mock situation, after that you can capture other values from the test setup in the mock closure.

Option 3. you can do already now, so there's no change needed, though it has the limitations that @teamon mentioned.

@vitorleal
Copy link

Any news on this? Having the same problem with the timeout middleware when using the Mock.

@leineu2016
Copy link

Having same problem with the timeout middleware when using Mock

@teamon
Copy link
Member

teamon commented Nov 9, 2018

As you can read in #255, this issue is more complex than it seems.
There are two workarounds:

  • Use Tesla.Mock.mock_global/1 (preferred)
  • Or skip including Tesla.Middleware.Timeout if Mix.env == :test (not the best idea)

@teamon teamon pinned this issue Dec 14, 2018
@teamon teamon unpinned this issue Dec 14, 2018
dunyakirkali added a commit to dunyakirkali/notion.ex that referenced this issue Dec 25, 2020
@teamon teamon added this to Roadmap May 8, 2022
@teamon teamon moved this to Todo in Roadmap May 8, 2022
@teamon teamon removed the blocked label May 8, 2022
@yordis
Copy link
Member

yordis commented Sep 10, 2023

Use task_module of Tesla.Middleware.Timeout to control the Task module

- `:task_module` - the `Task` module used to spawn tasks. Useful when you want
use alternatives such as `OpentelemetryProcessPropagator.Task` from OTEL
project.

Swap the implementation with one that doesn't use Task

@yordis yordis closed this as completed Sep 10, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Roadmap Sep 10, 2023
@carrascoacd
Copy link
Contributor

@yordis even if the solution you propose will work but, I was thinking that an alternative solution could be using ancestors to find the process with the mocked fn, so we are able to solve this problem. Creating an alternative Task implementation is not very good as it requires storing the state in the process to await later for it synchronously forcing clients to do so.

Something like this should be enough to find the process that contained the mocked data:

    pid = Enum.find(Process.get(:"$ancestors", []), nil, fn ancestor ->
      !is_nil(Process.get(ancestor, :testa_mock))
    end)

Let me know if it makes sense and I could PR on this.

@yordis
Copy link
Member

yordis commented Apr 10, 2024

Let me know if it makes sense and I could PR on this.

PRs are always welcomed!

@carrascoacd
Copy link
Contributor

Here it goes, dude! #668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants