Skip to content

Loading…

Fix visiting urls with non-http(s) schemes #1339

Merged
merged 1 commit into from

2 participants

@abotalov
Collaborator

Fixes #1336

@twalpole twalpole commented on the diff
lib/capybara/session.rb
((8 lines not shown))
url = Capybara.app_host + url.to_s
end
if @server
- url = "http://#{@server.host}:#{@server.port}" + url.to_s unless url =~ /^http/
+ url = "http://#{@server.host}:#{@server.port}" + url.to_s if url_relative
@twalpole Collaborator

this will create an invalid url if both Capybara.app_host and @server are set

@abotalov Collaborator

@server is set in initialize method when Capybara.run_server and @app and driver.needs_server? is true. It seems to me Capybara doesn't support a case when both app and app_host are set, does it?

@twalpole Collaborator

app_host would override @server because the code prepended app_host to the url and then in the if @server block the url would already begin with ^http. A way to fix is just to set url_relative=false after prepending app_host

@abotalov Collaborator

It seems this bug existed before this PR too. And I'm not sure what Capybara should append if both server and app_host are set. Also it should be documented what takes precedence if both of them are set.

@twalpole Collaborator

it didnt exist before because once Capybara.app_host was appened (http://www.google.com for instance) then it matched /^http/ so "http://#{@server.host}:#{@server.port}" was not prepended in the if @server section

@twalpole Collaborator

Also - Capybara.app_host needs to take precendence, otherwise its useless -- Its handy for testing multi-tenant by subdomain systems.

@twalpole Collaborator

It is documented in the Calling remote servers section of the readme - "Normally Capybara expects to be testing an in-process Rack application, but you can also use it to talk to a web server running anywhere on the internet, by setting app_host:" .... "By default Capybara will try to boot a rack application automatically. You might want to switch off Capybara's rack server if you are running against a remote application: Capybara.run_server = false"

ie -- app_host has precendence -- and you may want to turn off server if you're using it (or may not - either way app_host has precendence)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twalpole twalpole merged commit 5db902a into jnicklas:master

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@twalpole
Collaborator

thanks

@abotalov abotalov deleted the abotalov:fix_schemes_handling_in_visit branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 19, 2014
  1. @abotalov
Showing with 14 additions and 4 deletions.
  1. +5 −2 lib/capybara/session.rb
  2. +5 −0 lib/capybara/spec/session/visit_spec.rb
  3. +2 −1 spec/dsl_spec.rb
  4. +2 −1 spec/rack_test_spec.rb
View
7 lib/capybara/session.rb
@@ -206,12 +206,15 @@ def visit(url)
@touched = true
- if url !~ /^http/ and Capybara.app_host
+ url_relative = URI.parse(url.to_s).scheme.nil?
+
+ if url_relative && Capybara.app_host
url = Capybara.app_host + url.to_s
+ url_relative = false
end
if @server
- url = "http://#{@server.host}:#{@server.port}" + url.to_s unless url =~ /^http/
+ url = "http://#{@server.host}:#{@server.port}" + url.to_s if url_relative
@twalpole Collaborator

this will create an invalid url if both Capybara.app_host and @server are set

@abotalov Collaborator

@server is set in initialize method when Capybara.run_server and @app and driver.needs_server? is true. It seems to me Capybara doesn't support a case when both app and app_host are set, does it?

@twalpole Collaborator

app_host would override @server because the code prepended app_host to the url and then in the if @server block the url would already begin with ^http. A way to fix is just to set url_relative=false after prepending app_host

@abotalov Collaborator

It seems this bug existed before this PR too. And I'm not sure what Capybara should append if both server and app_host are set. Also it should be documented what takes precedence if both of them are set.

@twalpole Collaborator

it didnt exist before because once Capybara.app_host was appened (http://www.google.com for instance) then it matched /^http/ so "http://#{@server.host}:#{@server.port}" was not prepended in the if @server section

@twalpole Collaborator

Also - Capybara.app_host needs to take precendence, otherwise its useless -- Its handy for testing multi-tenant by subdomain systems.

@twalpole Collaborator

It is documented in the Calling remote servers section of the readme - "Normally Capybara expects to be testing an in-process Rack application, but you can also use it to talk to a web server running anywhere on the internet, by setting app_host:" .... "By default Capybara will try to boot a rack application automatically. You might want to switch off Capybara's rack server if you are running against a remote application: Capybara.run_server = false"

ie -- app_host has precendence -- and you may want to turn off server if you're using it (or may not - either way app_host has precendence)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if Capybara.always_include_port
uri = URI.parse(url)
View
5 lib/capybara/spec/session/visit_spec.rb
@@ -33,6 +33,11 @@
end.to raise_error(TestApp::TestAppError)
end
+ it "should be able to open non-http url", requires: [:about_scheme] do
+ @session.visit("about:blank")
+ @session.assert_no_selector :xpath, "/html/body/*"
+ end
+
context "when Capybara.always_include_port is true" do
let(:root_uri) do
View
3 spec/dsl_spec.rb
@@ -11,7 +11,8 @@ class TestClass
:frames,
:windows,
:server,
- :hover
+ :hover,
+ :about_scheme,
]
RSpec.describe Capybara::DSL do
View
3 spec/rack_test_spec.rb
@@ -10,7 +10,8 @@ module TestSessions
:frames,
:windows,
:server,
- :hover
+ :hover,
+ :about_scheme,
]
RSpec.describe Capybara::Session do
Something went wrong with that request. Please try again.