Add Finch support

[reisub]

Hi there @parroty, first off - thanks for the effort you put into building and maintaining this library, it's very much appreciated!

I used the excellent Finch library recently so I thought I'd add support for it in ExVCR. The initial idea was to add support for Mint (which Finch and some other libs use under the hood, shout out to #146), but Mint is on a lower level of abstraction than the rest of the clients supported by ExVCR so this seemed more appropriate.

I tried to keep the same style as the rest of the library, my starting point was the ibrowse code.

Two things are still bugging me about this code:

1. I think it's important for the mock response to look identical to the real one so there is a bunch of code to convert strings to atoms in the right places (see finch/converter.ex, lines 35-81). It's not perfect, but it should cover a large majority of the cases according to Finch/Mint docs. I'd love to get some input from @keathley or @ericmj on this if possible. -> fixed by using Elixir code (de)serialization methods for the error reason
2. The tests keep emitting [warn] Failed to lookup telemetry handlers. Ensure the telemetry application has been started. a bunch of times since I've added Finch to the deps. I haven't yet figured out why and what to do to make it stop spamming the log. -> fixed

Any comments and suggestions are very much welcome.

[parroty]

Thanks for the extensive implementations, and I'm sorry being late to respond.

I think it's important for the mock response to look identical to the real one so there is a bunch of code to convert strings.

Thanks for the note. Do you have insights whether it's good enough for the use? (or need to fix at first?). As long as it doesn't break existing behavior, it may be one option to add as experimental feature, etc (it's my basic assumption).

The tests keep emitting [warn] Failed to lookup telemetry handlers. Ensure the telemetry application has been started.

I could locally confirm the behavior when executing mix test. It's a little noisy and would like to silence them in some way before merging (I'm afraid I don't have resolution yet, though).

[reisub]

Thanks for the extensive implementations, and I'm sorry being late to respond.

I think it's important for the mock response to look identical to the real one so there is a bunch of code to convert strings.

Thanks for the note. Do you have insights whether it's good enough for the use? (or need to fix at first?). As long as it doesn't break existing behavior, it may be one option to add as experimental feature, etc (it's my basic assumption).

The current implementation should cover a vast majority of the error responses people might want to test with so I think you should consider shipping like this. The worst that can happen is that the response returned from a cassette has a string in place where Finch/Mint would return an atom. In that case, an issue should be filed and this can be corrected very quickly - just ping me and I can make add a test case and fix. For now, I've covered all the cases I've encountered and all the ones I could find documented in Finch and Mint docs.

I've also considered an alternative solution. We could add some more metadata into the serialized json, additional field(s) to use in deserialization to decide whether to make something a string or atom. Quick mockup (see atoms key):

  {
    "request": {
      "body": "",
      "headers": [],
      "method": "get",
      "options": [],
      "request_body": "",
      "url": "http://invalid_url/"
    },
    "response": {
      "binary": false,
      "body": "{\"__atoms__\":[\"reason\",\"nxdomain\"],\"__exception__\":true,\"__original_struct__\":\"Elixir.Mint.TransportError\",\"reason\":\"nxdomain\"}",
      "headers": [],
      "status_code": null,
      "type": "error"
    }
  }
]

I've thought about it a bit, but then decided that it would add more complexity for not that much of a gain. I'd be open to reconsider that decision, though.

The tests keep emitting [warn] Failed to lookup telemetry handlers. Ensure the telemetry application has been started.

I could locally confirm the behavior when executing mix test. It's a little noisy and would like to silence them in some way before merging (I'm afraid I don't have resolution yet, though).

I just pushed a new commit to start the :telemetry app in test_helper.exs and that did the trick, so this is resolved.

[coveralls]

Coverage increased (+0.4%) to 88.764% when pulling b64d475c207de31bf232b3f727872f1b35c6aed5 on reisub:feature/finch-support into a8e4a8579c970cf1e5659310f2bf3c80805d87d0 on parroty:master.

[reisub]

@parroty just one more thing, I had to change the OTP/Elixir version in the github action config and subsequently update the excoveralls dependency for it to work with OTP 23 - I hope it's not a problem.

[reisub]

I found one case which doesn't work correctly, Mint can return an error that contains a tuple, like this one: %Mint.TransportError{reason: {:bad_alpn_protocol, "h3"}} and JSX doesn't know what to do with the tuple.

I'll change this PR to draft and see what we can do about it. Maybe we can define a custom (de)serializer for tuple..

[reisub]

@parroty you can disregard the above comments, I found a much more elegant way of (de)serializing the error reason struct - now it will work correctly regardless of what kind of error format is encountered because we use the Elixir code (de)serialization functions.

Ready for review, thanks for your patience!

[parroty]

Sorry not being responding again. I could confirm that the warning messages are no longer shown. Also, thank you for the explanation about the functional coverage. If existing behavior (other than finch) is not affected, I would be ok to merge.

Could you let me reconfirm that it's ready to merge?

[reisub]

@parroty yes, it's ready to merge. Thanks for reviewing!

[parroty]

Thanks for the implementations!

[ruslandoga]

@parroty @reisub just a heads up, this update breaks apps that don't have :finch dependency since it uses %Finch.Response{} struct unconditionally on its presence during compilation.

== Compilation error in file lib/exvcr/adapter/finch.ex ==
** (CompileError) lib/exvcr/adapter/finch.ex:70: Finch.Response.__struct__/1 is undefined, cannot expand struct Finch.Response. Make sure the struct name is correct. If the struct name exists and is correct but it still cannot be found, you likely have cyclic module usage in your code
    (stdlib 3.15.2) lists.erl:1358: :lists.mapfoldl/3
    (elixir 1.12.2) expanding macro: Kernel.if/2

[reisub]

@ruslandoga thanks for the bug report and sorry for the inconvenience! I'll open a PR with a fix shortly.

[ruslandoga]

@reisub I think one way would be to use %{__struct__: Finch.Response} to make compiler skip struct checking, and the other -- and more correct -- way is to check for Finch or Finch.Response presence before defining the adapter module.

if Code.ensure_loaded?(Finch) do
  defmodule FinchAdapter do
  end
else
  defmodule FinchAdapter do
    def function_that_exvcr_calls_during_tests(_opts) do
       raise "missing dep: Finch"
    end
  end
end

Sorry for unsolicited advice ...

---

Also, thank you for the PR, it'd definitely come in useful once we switch to finch from hackney.

[reisub]

@ruslandoga thanks for the advice, I appreciate the guidance as I was unsure what the best way to solve it would be :)

[parroty]

@ruslandoga @reisub Thanks for the comment and fix. I've merged #178.