Respect the params_encoder in Faraday::Adapter::Test
#1316
Conversation
Faraday provides a feature to let its callers configure `params_encoder`, but `Faraday::Adapter::Test` had always created test stubs with `#parse_nested_query` (`NestedParamsEncoder`). So, we had not able to test such a connection with `Faraday::Adapter::Test`. This might be inconvenient for users who want to use non-default `params_encoder`, such as `Faraday::FlatParamsEncoder`. This patch changed the adapter to use the same `params_encoder` as its connection in order to solve the problem.
| # uncomment to raise Stubs::NotFound | ||
| # assert_equal 'shrimp', cli.sushi('ebi', params: { abc: 123, foo: 'Kappa' }) | ||
| stubs.verify_stubbed_calls | ||
| end |
There was a problem hiding this comment.
I forgot to add this test in #1298 😅
There was a problem hiding this comment.
Nice work @yykamei, I left a comment with a suggestion on how to keep the code cleaner and more maintainable. I'm scared that the duplication introduced might cause issues in the future when only one side is updated and not the other.
I'm also open to different solutions that solve the same issue in different ways, mine is only an idea
lib/faraday/adapter/test.rb
Outdated
| params_encoder = env.request.params_encoder || Faraday::Utils.default_params_encoder | ||
| env[:params] = params_encoder.decode(env[:url].query) || {} |
There was a problem hiding this comment.
Mmmmh I really like how much simpler the matches? methods signature are now, thanks to the env being passed, but I notice there's a lot a of repeated code between here and there. These two lines for example, and line 242 as well.
I wonder if instead of reusing env it might make sense to introduce a new Struct, internal and only used in the Test adapter, that holds this information so we only need to calculate it once. What do you think?
There was a problem hiding this comment.
Thank you for your quick review! It makes sense 👍 I'll try it.
There was a problem hiding this comment.
I tried to avoid similar codes with 91689f0. How about the change?
Co-authored-by: Matt <iMacTia@users.noreply.github.com>
In order to avoid dups of `params_encoder.decode`, set `env[:params]` with the vaule generated by `params_encoder.decode` before calling `stubs.match`.
Close #1312
Faraday provides a feature to let its callers configure
params_encoder, butFaraday::Adapter::Testhad always created teststubs with
#parse_nested_query(NestedParamsEncoder). So, we had been notable to test such a connection with
Faraday::Adapter::Test. This mightbe inconvenient for users who want to use non-default
params_encoder,such as
Faraday::FlatParamsEncoder.This patch changed the adapter to use the same
params_encoderas itsconnection in order to solve the problem.
Todos