Add Req.Test.expect/3

[whatyouhide]

@wojtekmach I think this makes sense in the optics of tests and stubs. I have found myself doing a lot of send(test_pid, ...) to ensure that everything gets called and in the right order. This helps a lot with that, since it introduces ordering. We'd need the usual verify suspects, but I'll only spend time on those if you think this change is good 😄

[wojtekmach]

Could you share a real world-ish example of using this API?

It's amazing it's so little code thanks to NimbleOwnership.

I'm not sure about expanding the scope, I'd really prefer to keep it focused on just stubs. However, maybe the way it started, this is the only logical next step. I.e. because it's possible to set stubs per test it's only natural you want to set some expectations. Maybe these are not stubs something that has predefined behaviour (i.e. completely static) but these were mocks all along. Sorry thinking out loud here and getting a bit philosophical. Anyhow, an example would be great!

[whatyouhide]

@wojtekmach

Could you share a real world-ish example of using this API?

My tests are now riddled with stubs like this:

Example code

Req.Test.stub(Klaviyo.API, fn conn ->
  cond do
    conn.method == "PATCH" and conn.request_path == "/api/profiles/#{user.klaviyo_id}" ->
      send(test_pid, :updated)

      conn
      |> Plug.Conn.put_status(200)
      |> Req.Test.json(%{})

    conn.method == "POST" and conn.request_path =~ "/api/lists/" ->
      assert ["api", "lists", list_id, "relationships", "profiles"] =
                String.split(conn.request_path, "/", trim: true)

      send(test_pid, {:added_to_list, list_id})

      conn
      |> Plug.Conn.put_status(204)
      |> Req.Test.json(%{})

    true ->
      flunk("No stub defined for request: #{conn.method} #{conn.request_path}")
  end
end)

Essentially, I have to do the heavy lifting of figuring out which request I’m stubbing, how many requests have been made, in which order, and so on. Expectations let me define an order of requests so I can rewrite the example above with something like this:

Req.Test.expect(Klaviyo.API, fn conn ->
  assert conn.mathod == "PATCH"
  assert conn.request_path = ...
  
  conn
  |> Plug.Conn.put_status(200)
  |> Req.Test.json(%{})
end)

Req.Test.expect(...)

Now it takes care of the ordering, of making sure these don't get called too many times (for too few times we need verify and friends), and of expectations.

these were mocks all along

Yep 🙃

[wojtekmach]

Thanks for the example, that's super useful. I think for your particular use case something custom would be way more appealing. Something like the following perhaps?

Klaviyo.Test.expect("PATCH", "/api/profiles/#{user.klaviyo_id}", fn conn, _params ->
  Req.Test.json(conn, %{})
end)

Klaviyo.Test.expect("POST", "/api/lists/:id/relationships/profiles", fn conn, %{"id" => id} ->
  Req.Test.json(%{conn | status: 204}, %{})
end)

And I don't think it would be super difficult to implement. Ideally you'd be able to build this custom solution on top of Req.Test but I gotta a feeling you'd not be able to reuse any of Req.Test.{stub/1, stub/2, init/1, call/2} functions though because you'd need NimbleOnwership expectations. But all these functions are pretty tiny so I don't think it's a big deal.

Such expect(method, url, (conn, params -> conn)), if that's even a good idea, seems like something I'd maybe occasionally use myself so I'll keep an eye for such cases. But it's plug specific and Req.Test.stub/1-2 are for arbitrary stubs so there'd be a bit of a conflict.

All that being said, I'm not sure on expanding the scope just yet so I'm thinking about holding off on this. Important question is if we can ship Req.Test.{stub/1, stub/2, allow/3} in Req v1.0 and iterate on it afterwards or we're paining ourselves into a corner naming, semantics, and/or implementation-wise. I think we're good.

WDYT?

[wojtekmach]

I guess part of my hesitations is I tend not to write too many expectations which, yeah, make my tests catch less bugs but I don't think I've been burned too badly. (It really helps that I always write perfect code.) Maybe I should read a book on testing or something!

[whatyouhide]

I guess part of my hesitations is I tend not to write too many expectations which, yeah, make my tests catch less bugs but I don't think I've been burned too badly.

Mmm sure but I think if anything having only stubs makes it easier to write bugs in the code, no?

I think for your particular use case something custom would be way more appealing.

The API you suggested would be quite a bit of custom code that I'd end up writing for all HTTP APIs I used Req with, really. Every call to the proposed Klaviyo.Test.expect/3 would have to store the expectation somewhere, only for the function passed to Req.Test.stub/2 to retrieve, pop off there, and so on. It's quite a hassle IMO.

Such expect(method, url, (conn, params -> conn)), if that's even a good idea, seems like something I'd maybe occasionally use myself so I'll keep an eye for such cases. But it's plug specific and Req.Test.stub/1-2 are for arbitrary stubs so there'd be a bit of a conflict.

Yeah this expect/3 is very specific to a specific HTTP request (of which you know the path, not always true since often those are dynamic). The Req.Test.expect/2 I propose in the PR makes that trivial.

I absolutely don't think we're painting ourselves into a corner. If you want to ship Req.Test, my 2c is that expectations are non negotiable. After having worked with the Req.Test API for a bit, the ergonomics are not good without expectations IMO. Otherwise, you can always hold off on Req.Test altogether and remove it before 1.0, and someone can release a lib with it or something. Honestly, the API that Im proposing is so small and so familiar (Mox) that I thought it'd be a no brainer 😬

[wojtekmach]

OK, let's forget about expect(method, url, fun).

Every call to the proposed Klaviyo.Test.expect/3 would have to store the expectation somewhere, only for the function passed to Req.Test.stub/2 to retrieve, pop off there, and so on. It's quite a hassle IMO.

I guess my idea was to use NimbleOwnership process started by Req and write your custom expect/2 function just like this in PR which wouldn't be a lot of code but I don't think it's gonna work after all as the ownership will hold different state. So you'd need to reimplement everything, here's a quick very lightly tested draft: https://gist.github.com/wojtekmach/bb4a77037386c170d103d8beb455de37. Fair enough it's a bit more stuff than I expected.

If you want to ship Req.Test, my 2c is that expectations are non negotiable.

That's very fair. I'll think about it.

Btw to take a big step back, the whole deal with Mox is of course we use explicit contracts. Req.Test a bit mimics Mox. But With Req.Test we can stub (and with this PR also expect) willy nilly anything. Maybe that's gonna be misused?

[whatyouhide]

So you'd need to reimplement everything

That's a 100 lines of pretty complex code if you ask me. Sure I can do that, but I’m not sure most folks are familiar with nimble_ownership, which itself is a pretty niche lib targeted at a pretty specific use case.

But with Req.Test we can stub (and with this PR also expect) willy nilly anything. Maybe that's gonna be misused?

I don't think you can stub anything, I think you can only stub/expect requests. Req is a HTTP client that only makes HTTP requests (with a bunch of stuff to embellish the requests, sure, but at the end of the day) and we're stubbing/expecting only that, so I think it fits like a glove? 🙃

Req.Test in the first place?

I think Req.Test truly does belong in Req. Most of the HTTP testing today happens in one of these ways:

1. Bypass. This is nice, but it's most importantly not very concurrent-test-friendly. It's rare that libs let you configure endpoints to hit in a non-global way, after all.
2. Mocks at the contract level (like, MyApp.WeatherAPIMock). Great for testing higher layers of the abstraction, and fast and whatnot, but does not exercise the HTTP code, so useless if you want to actually test that.
3. Recorded requests (ExVCR) — just another form of mocks a la bypass, just a different flavour.

Req.Test allows you to do 2 (fast, concurrent tests) while also exercising the HTTP code. If you ship it with Req, my hope (and gut feeling) is that it'll steer people into writing this kind of tests more and more, which can only be a good thing for me.

Hope this all makes sense 💌

[wojtekmach]

But with Req.Test we can stub (and with this PR also expect) willy nilly anything. Maybe that's gonna be misused?

I don't think you can stub anything, I think you can only stub/expect requests.

OK I partially take it back, we can't stub willy nilly anything, the stubs are explicitly named. The point still stands that we can put anything into a stub. The primary use case has always been plug stubs. But another totally viable use case is Bypass. I don't talk about it in docs but this is possible today:

# config/test.exs
config :myapp, req_options: [base_url: fn -> Req.Test.stub(:foo) end]

# test/myapp_test.exs
Req.Test.stub(:foo, "http://localhost:#{bypass.port}")

But yeah, nevermind, I freaked out a little bit but because stubs are explicitly named I think we're good.

Hope this all makes sense 💌

Yeah it all makes sense. I think given all this I'm leaning towards adding expect.

My question is, does this look ok conceptually?

Req.Test.verify(:foo, 1)
Req.Test.stub(:foo)

I.e. we define an expectation so it's a mock but then refer to it as stub? Should we rename:

1. def stub(stub_name, value) -> def stub(mock_name, value)
2. def stub(stub_name) -> def mock(mock_name)

(Let's not do any changes just yet.)

I guess it's all a bit blurry to me. The way I always understood mocks was we expect function fun to be called with arg1..argN and return result1. Here there are no functions involved per se, I mean, again, the primary use case is stubbing plug functions but these functions are treated as data if that makes sense. But maybe it doesn't matter in practice.

[whatyouhide]

My question is, does this look ok conceptually?

Not really. stub/1 never really made sense to me, I think what I'd want there is Req.Test.get/1 instead? Like, we have a key-value store here essentially, and I just want a way to fetch the value associated with a given key, right?

Here there are no functions involved per se, I mean, again, the primary use case is stubbing plug functions but these functions are treated as data if that makes sense. But maybe it doesn't matter in practice.

I think you hit the nail on the head: it doesn't matter in practice.

The way I always understood mocks was we expect function fun to be called with arg1..argN and return result1.

That's a subset of what mocks can be. Terminology is always hard to agree on, but the concept of a "mock" in this case is more like a value that you can use in place of the real thing. In Mox those are functions and behaviors (as the value), but here it can be a plug or a value that you return (Bypass URL) or whatever. Even with that, you still might want to assert that a value is fetched n times—even if that value is not a function but just a URL or whatever; enter expect/3 🙃

[whatyouhide]

One more note: I don't think the "stub can be any value" vs "stub is behavior in the context of mocks, so we need expect/3" approaches are exclusive.

[wojtekmach]

Ok to recap, today we have:

Req.Test.stub(stub_name)
Req.Test.stub(stub_name, value)
Req.Test.allow(stub_name, owner_pid, pid_to_allow)

I'd be OK with renaming to Req.Test.get/1. Naming wise, it returns a stub or mock or something more generic? Going with mock_name we have this:

Req.Test.stub(mock_name, value)
Req.Test.expect(mock_name, count \\ 1, value)
Req.Test.allow(mock_name, owner_pid, pid_to_allow)
Req.Test.get(mock_name)

# and eventually also:
Req.Test.verify!()
Req.Test.verify!(mock_name)
Req.Test.verify_on_exit!()

Strong Mox vibes can make it really easy for folks to pick up. It's gonna suck when someone in the same test is gonna import both Req.Test and Mox though, which is totally gonna happen, but I guess it is what it is. Speaking of Mox, worth pointing out that it's trivial to replicate Req.Test with it:

Mix.install([:req, :plug, :mox])

Mox.defmock(MyApp.Mock, for: Plug)

Mox.stub(MyApp.Mock, :call, fn conn, [] ->
  Plug.Conn.send_resp(conn, 200, "hi")
end)

"hi" = Req.get!(plug: MyApp.Mock).body

(which is how Req.Test started btw) but it's important to me to have something nice built-in and I guess it grows on me that having expectations is non negotiable.

I'll think some more about it.

[josevalim]

I would call it context or group:

Req.Test.stub(context, value)
Req.Test.expect(context, count \\ 1, value)
Req.Test.allow(context, owner_pid, pid_to_allow)
Req.Test.url_for(context)

# and eventually also:
Req.Test.verify!(context)
Req.Test.verify!(context)
Req.Test.verify_on_exit!(context)

---

If Req.Test only uses Req public APIs, I would consider having it as a separate package, req_test, which we can promote by default. The benefit is that we can add nimble_ownership and other dependencies without affecting dev/prod.

[whatyouhide]

If Req.Test only uses Req public APIs, I would consider having it as a separate package, req_test, which we can promote by default. The benefit is that we can add nimble_ownership and other dependencies without affecting dev/prod.

Yeah I've been thinking more and more about this... I like this. I'd do this for now.

It's gonna suck when someone in the same test is gonna import both Req.Test and Mox though, which is totally gonna happen

It is. We can rename these to Req.Test.{expect|allow|stub}_req, I do something similar in Sentry to avoid the naming collision. Thoughts?

I would call it context or group:

Love group. context is already used in ExUnit so I'd avoid that.

[josevalim]

Maybe we don't suggest users to import. Req.Test.stub|expect is fine. Use setup {Req.Test, :verify_on_exit} for verifying?

[wojtekmach]

group as in process group?

A separate req_test package is definitely cleaner dependencies-wise. Req :plug feature already requires the :plug optional dependency so a req_test would have a required dependency on that. There are no other relevant dependencies at the moment. (Req itself has a required dependency on Jason.)

So the benefit of not directly depending on NimbleOwnership is on principle we should bring as least deps as possible, less package downloads for folks, less possible version conflicts. That being said, NimbleOwnership is pretty nimble, 300 LOC without docs & specs... I might just vendor it in and be on the hook for manually updating it. Would that be ridiculous?

In any case, I'll think about req_test some more for sure.

Yeah, not suggesting to import Req.Test sounds good to me.

Regarding verify! and friends, could expect automatically verify or it's not that simple? I.e. expect/3 calls something like ExUnit.Callbacks.on_exit(fn -> verify!(group) end) and verify!/1 is private?

[whatyouhide]

setup {Req.Test, :verify_on_exit}... I swear you never stop learning. I didn't know you could do setup {mod, fun}. Can you all please kick me out of the core team? 😄

So the benefit of not directly depending on NimbleOwnership is on principle we should bring as least deps as possible, less package downloads for folks, less possible version conflicts. That being said, NimbleOwnership is pretty nimble, 300 LOC without docs & specs... I might just vendor it in and be on the hook for manually updating it. Would that be ridiculous?

You could vendor in but the req_test package is a better idea IMO, it allows you to do {:req_test, only: :test} as a dep as well.

[whatyouhide]

group as in process group?

No as in group of requests, but good point. scope could also work, or something like that.

[wojtekmach]

Thank you!

I'm still not sure about naming, stub/mock/group/scope/etc, so will leave it as is for now.