Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Faraday::Adapter::Test stubs now support entire urls (with host) #741

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Faraday::Adapter::Test stubs now support entire urls (with host) #741

merged 1 commit into from
Nov 13, 2017

Conversation

escoberik
Copy link
Contributor

Addresses issue #358

@escoberik escoberik force-pushed the test_stub_support_for_host branch from 4190ea6 to cdddb8d Compare November 2, 2017 22:22
iMacTia
iMacTia requested changes Nov 4, 2017
View reviewed changes
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job so far.
I just think we lost backwards compatibility this way and we should have tests for both use cases (path only stub and full URL stub).

Please see my comments

lib/faraday/adapter/test.rb Outdated
request_path, request_query = request_uri.split('?')
request_params = request_query ?
Faraday::Utils.parse_nested_query(request_query) :
{}
# meta is a hash use as carrier
# that will be yielded to consumer block
meta = {}
return path_match?(request_path, meta) &&
return host == request_host &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, this line should break backwards compatibility.
That's because we're now passing the request_host to be checked, but host could be nil (in case the stub is created with just a path).
That should cause this condition to fail

test/adapters/test_middleware_test.rb Outdated
@@ -81,7 +94,7 @@ def test_middleware_allow_different_outcomes_for_the_same_request
end

def test_yields_env_to_stubs
@stubs.get '/hello' do |env|
@stubs.get 'http://foo.com/hello' do |env|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above regarding the host.
We still want to allow people to stub paths (hence URLs with no host).
We should then keep this tests unchanged and duplicate all of them to test that the stub works WITH and WITHOUT the host specified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm actually kind of confused about this. The question is, what does it mean when user doesn't specify any host in the stub? Does it mean the stub should be available for all hosts? I mean, should connection.get('http://domain.com/path') look for stubs declared like stubs.get('/path') as well as stubs.get('http://domain.com/path').

I think the use case mentioned in the issue (#358 (comment)) is trying to make sure Faraday is hitting the right domain. With my changes is still possible to make stubs with no host, but they'll only match requests with no host as well. Is this wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for not being more clear from the beginning.
The wanted behaviour is the following:

  1. If a hostname is specified on the stub, then both the hostname AND the path should match.
  2. If no hostname is specified on the stub (hence only the path), then any hostname should match.

Point 1 will "fix" #358, while point 2 will maintain backward compatibility, as the current behaviour is "stub this path on any host" and we should keep that working as well 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for clarifying! I'll take care of it 😃

test/authentication_middleware_test.rb Outdated
@@ -2,7 +2,7 @@

class AuthenticationMiddlewareTest < Faraday::TestCase
def conn
Faraday::Connection.new('http://example.net/') do |builder|
Faraday::Connection.new do |builder|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying a domain in the connection forces the stubs to only be available for that domain. Therefore, making connection.get('/path') (without the host) won't match. I removed this to make the connection domainless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my new comment above on the expected behaviour.
I hope this is clear now, this test should definitely work with the domain in place (it will test point 2 of my comment).

Rather than changing existing tests, your approach should be to duplicate them in order to test both points (old and new behaviour).

@iMacTia iMacTia added this to the v0.14.0 milestone Nov 9, 2017
@iMacTia
Copy link
Member

iMacTia commented Nov 12, 2017

Hey @erik-escobedo, sorry I'm just touching base to know if there's any progress on this.
I have included it into v0.14.0 which will I'd like to ship before the end of the month.
Do you think you can find some time to work on this before then?

@escoberik
Copy link
Contributor Author

Totally. I'm working on it right now

@escoberik escoberik force-pushed the test_stub_support_for_host branch from cdddb8d to 2aabd19 Compare November 12, 2017 21:06
@escoberik escoberik force-pushed the test_stub_support_for_host branch from 2aabd19 to 7588f72 Compare November 12, 2017 21:19
@iMacTia
Copy link
Member

iMacTia commented Nov 13, 2017

Amazing job @erik-escobedo 👍
Nice extension of the existing test adapter stubs.
Looks ready for merging to me, were you planning any additional work on it?

@escoberik
Copy link
Contributor Author

Thanks! No, I'm not planning to make additional work on this, until you think it's necessary. I've been reading about this issue and I'm ready to help if you give the green light:
#718

@iMacTia iMacTia merged commit 53b5e08 into lostisland:master Nov 13, 2017
@iMacTia iMacTia mentioned this pull request Nov 13, 2017
Closed
@iMacTia
Copy link
Member

iMacTia commented Nov 13, 2017

Merged 👍!
Regarding #718, that's a bit tricky as I'm still deciding what to do.
I'll add a comment there with the latest details so you can leave your thoughts.
You can also contribute to v1.0 having a look at the list on #620 if you want 😄

@escoberik
Copy link
Contributor Author

Of course! I'd like to work on this one: #305

@iMacTia
Copy link
Member

iMacTia commented Nov 13, 2017

Sounds great! There's still some discussion around what the current "options" method should be called so please join the discussion and provide a suggestion before working on the PR!
If you're out of ideas then let me know and I'll have a look and tell you my opinion 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iMacTia iMacTia iMacTia approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
v0.14.0
Development

Successfully merging this pull request may close these issues.
2 participants
@escoberik @iMacTia