Tests with the combination of `strict_mode` and `FlatParamsEncoder` may fail
yykamei opened this issue · 6 comments
With strict_mode, the Testing adapter tries to check the equality of parameters here.
faraday/lib/faraday/adapter/test.rb
Line 202 in 24b3fd3
This check sometimes return false with FlatParamsEncoder because an array value may contain elements by different order. For example,
stubs = Faraday::Adapter::Test::Stubs.new do |stub|
stub.get("/foo?key=123&key=234&key=abc") do |_env|
[200, { content_type: "application/json" }, JSON.dump(status: "OK")]
end
end
conn = Faraday.new { |b| b.adapter(:test, stubs) }
conn.get("/foo", { key: [123, 234, "abc"] })
stubs.verify_stubbed_calls # => Error with this comparison: `Set.new(key: [123, 234, "abc]) == Set.new(key: ["abc", 123, 234])`In general, the order of query is not important, so I want to check keys without order.
What do you think?
This makes sense, but I can see someone coming in future and asking how to write a test that also checks the specific order of things...
Ideally, it would be great to support both scenarios and let the developer choose which one they want to apply in any given case. I'd also feel reluctant in changing the current behaviour as this would cause existing tests relying on the adapter to fail.
A possible solution to all these points could be the following:
- We keep the current behaviour as-is
- We extend the test adapter capabilities so that query parameters in the stub can be passed as a separate argument, rather than as part of the URI (e.g.
stub.get('/foo', query: { key: [123, 234, "abc"] })) - We allow to pass RSpec matchers as values for
query, similarly to how Webmock allows you to do
I like this because it would be flexible enough to support all cases, while also keeping the existing behaviour, but I'm not sure about the complexity of such a change, so I'm open to alternative suggestions as far as they respect the points I highlighted above!
Oh, I understood FlatParamsEncoder has a feature to sort parameters.
As you said, it would be necessary for someone who wants to check the specific order of parameter values.
Introducing query looks great 👍 Of course, the Test adapter must consider parameters included in path as well as query, but it would be useful to construct parameters with the Hash value. So, I think it's worth implementing 😄
By the way, is query good for the argument name? Faraday's methods seem to use params for the URL query parameters. For example, Connection#get has params for its method argument.
faraday/lib/faraday/connection.rb
Line 135 in 24b3fd3
The idea behind using query and body instead of just params is that you'll be able to distinguish them on HTTP methods that support both (e.g. POST).
Of course, the Test adapter must consider parameters included in path as well as query
Good point, I think I see where this is coming from.
Could you show an example of this just to be sure we're on the same page?
OK, using query makes sense 👍
I think changing here might be the first step:
faraday/lib/faraday/adapter/test.rb
Line 151 in 24b3fd3
Stub seems to have query as a string, but we will have to merge this query with the new parameter query, which comes from the argument of #new_stub. So, I'm considering changing Stub#query from string to Hash.
The example code in #new_stub here.
path, query = normalized_path.respond_to?(:split) ? normalized_path.split('?') : normalized_path
query = convert_to_hash(query) # NOTE: convert_to_hash is something like URI.decode_www_form
query.merge!(query_from_args)
stub = Stub.new(host, path, query, headers, body, @strict_mode, block)That makes sense to me, Stub can use any internal representation for query as far as it works in the same way 😄!
Sorry I was sure I got back to you already on this 🙈
Oh, I misunderstood the implementation of FlatParamesEncoder. I reconsidered this issue and keeping as-is might be the best way. Thanks for thinking about this issue.
By the way, adding query for testing adapters might be a nice to have. This is out of this issue 😄