Req.Test: stub anything

[wojtekmach]

In https://github.com/wojtekmach/req/pull/349/files#diff-455e75050e3f9495a2db20296942753a0f4b6e1a65b96d48127d119a60e38cc4R361 we added a restriction that we can only stub and and expectations for plugs.

@whatyouhide so on Livebook we just had a PR where it would be really convenient if we didn't have this restriction. To give some context, we're integrating with Fly API. Most of the time we want to be hitting a stub. But in integration tests we want to hit the real thing. Our unit and integration tests are both in Mix.env() == :test.

My idea was the following: https://github.com/livebook-dev/livebook/pull/2708/files#diff-489b337badb7f480b22ff4c701659f5aef800fb470112d4ddf51f39256c2bbbeR249-R273

and then in unit test we'd do:

Livebook.FlyAPI.stub(fn conn -> ... end)

and in integration tests:

Livebook.FlyAPI.passthrough()

(Of course these test-only helpers are totally optional, the more verbose Req.Test.stub(Livebook.FlyAPI, fn conn -> ... end) and Req.Test.stub(Livebook.FlyAPI, :passthrough) is fine too.)

if we forget to set a stub whenever we call FlyAPI in Mix.env :test, we'd get a crash.

When we discussed making Req.Test.stub/2 stricter, our consensus was let's keep it focused on plugs so people don't abuse this feature to stub random unrelated things willy nilly. People can use Mox and/or NimbleOwnership too. That being said, I think this is a legitimate use case that I'd like to support directly in Req.

I think the easiest thing to do is lift the restriction and add Req.Test.fetch_stub!(name) so we could implement the above mentioned code using public API. Dunno if we add Req.Test.fetch_expectations!(name) and what are the semantics.

Another idea is to make stubs still stricter but in a different way. Instead of stubbing plugs we stub Req options, usually plug: .... And so:

# fly_api.ex
def new do
  Req.new(test_options())
end

if Mix.env() == :test do
  defp test_options do
    Req.Test.fetch_stub!(__MODULE__)
  end
else
  defp test_options do
    []
  end
end

# unit test
Req.Test.stub(FlyAPI,
  plug: fn conn ->
    conn
  end
)

# integration test
Req.Test.stub(FlyAPI, [])

Maybe this is weird because vast majority of time people would be in fact stubbing plugs.

Anyway, regarding stubbing options, we could even do something like:

# config/test.exs
config :livebook, fly_api_req_options: [test_options: FlyAPI]

# fly_api.ex
def new do
  Req.new(Application.fetch_env!(:livebook, :fly_api_req_options))
end

# unit test
Req.Test.stub(FlyAPI,
  plug: fn conn ->
    conn
  end
)

That is, there is a new fetch_test_options step (which only makes sense as the very first step and we'd put it as such) and a corresponding test_options: name option which fetches options from stub/mock with that name. And so there is no longer need for plug: {Req.Test, name} which always felt tiny bit hacky. I don't know if I really like this approach but thought there's maybe something to it?

Btw, since day 1 we have a Req.default_options() and Req.default_options(options) which simply store stuff in app env. I used it in my .iex.exs and occasionally in test/test_helper.exs. This is completely global so at the moment doesn't play well with multiple Req clients in a single project but maybe using/changing this feature can be helpful in what we want to achieve.

cc @jonatanklosko

[jonatanklosko]

I actually like :test_options a lot, it gives more flexibility, while still keeping stubbing within Req boundaries.

[whatyouhide]

@wojtekmach stubbing options feels to me pretty complex. You have a lot of moving parts and possibly a complex matrix of behaviors. Instead, have you considered just adding Req.Test.passthrough(conn)? That way, you still pass a Plug but it just does the actual passthrough.

[wojtekmach]

Req.Test.passthrough unfortunately won’t work because users would already have set:

plug: {Req.Test, name}

and so it is too late to opt-out of using plug.

[wojtekmach]

I suppose we could make it work but it would be really really hacky. (We’d store in private the adapter people might have set and in run_plug/1 adapter we check if we are passing through and if so, bail, call the saved off adapter. I’d rather avoid it because it is adding a lot of coupling that wasn’t there before.)

[whatyouhide]

It would be hacky in that the plug would essentially run as a proxy and make the original request. Yah maybe a bit hacky but with the value that you don't change the user-facing API and are free to keep making improvements to all this.

Another option would be to just support :passthrough | plug, which while restrictive, I still think covers really most use cases.

[wojtekmach]

Another option would be to just support :passthrough | plug, which while restrictive, I still think covers really most use cases.

not sure what you mean, can you elaborate?

Instead of test_options: name and Req.Test.stub(name, plug: plug), we could have test: name and Req.Test.stub(name, plug | :passthrough). This would be similar hack to what I mentioned prior, just implemented elsewhere. I think the way it would work is we'd have a run_test step (its order doesn't matter) which checks for test: name and if so sets adapter to plug: fetched_stub_or_mock_by(name). I don't like to run a step that is only meant for tests in production but it's cheap if all it would mostly do is check for an option.

That being said, if passthrough feature can be considered pretty niche, I'm OK making it not completely composable, i.e. it sets the adapter back to the default adapter, Finch.

Another completely different option which I think keeps internals cleaner is moving from name-based to URL-based lookups. That is:

# config/test.exs
config :myapp, foo_req_options: [adapter: :test]

# lib/foo.ex
def new do
  Req.new(
    base_url: "http://example.com"
  )
  |> Req.merge(Application.fetch_env!(:myapp, :foo_req_options))
end

# test/foo_test.exs
Req.Test.stub("example.com", &Req.Test.json(&1, %{foo: 1}))

# test/integration_test.exs
Req.Test.stub("example.com", :passthrough)

In adapter: :test, we always find a stub/mock and use it as plug: plug (or passthrough). What I like about this is removing a thing, name, which always felt tiny bit awkward. The exact semantics of this URL are TBD but I imagine it'd be implicit or explicit wildcard on the request hostname. We'll catch typos. Hell, we could allow Req.Test.stub("*", plug | :passthrough) (or even skip the first argument in those cases) since I think vast majority of time people are using one Req client in system under test at a time anyway.

[wojtekmach]

Of course something closer to what we have right now would totally work too, I'm totally fine special casing adapter: {:test, name}:

# config/test.exs
config :myapp, foo_req_options: [adapter: {:test, Foo}]

# test/foo_test.exs
Req.Test.stub(Foo, &Req.Test.json(&1, %{foo: 1}))

# test/integration_test.exs
Req.Test.stub(Foo, :passthrough)

That being said I think there's something to the baseurl-based approach.