Merge pull request #1922 from teamcapybara/issue_1921

Don't overwrite port when no server or always include port is false

History.md

@@ -1,3 +1,9 @@
+# Version 2.15.4
+Release date: unreleased
+
+### Fixed
+*  Visiting an absolute URL shouldn't overwrite the port when no server and/or always_include_port=false - Issue #1921
+
 # Version 2.15.3
 Release date: 2017-10-03
 

lib/capybara.rb

@@ -69,7 +69,7 @@ module Capybara
     # === Configurable options
     #
     # [app_host = String/nil]             The default host to use when giving a relative URL to visit, must be a valid URL e.g. http://www.example.com
-    # [always_include_port = Boolean]     Whether the Rack server's port should automatically be inserted into every visited URL (Default: false)
+    # [always_include_port = Boolean]     Whether the Rack server's port should automatically be inserted into every visited URL unless another port is explicitly specified (Default: false)
     # [asset_host = String]               Where dynamic assets are hosted - will be prepended to relative asset locations if present (Default: nil)
     # [run_server = Boolean]              Whether to start a Rack server for the given Rack app (Default: true)
     # [raise_server_errors = Boolean]     Should errors raised in the server be raised in the tests? (Default: true)

lib/capybara/session.rb

@@ -255,21 +255,24 @@ module Capybara
         config.app_host && ::Addressable::URI.parse(config.app_host)
       end
 
-      uri_base.port ||= @server.port if @server && config.always_include_port
-
       if uri_base && [nil, 'http', 'https'].include?(visit_uri.scheme)
-        visit_uri_parts = visit_uri.to_hash.delete_if { |k,v| v.nil? }
+        if visit_uri.relative?
+          uri_base.port ||= @server.port if @server && config.always_include_port
+
+          visit_uri_parts = visit_uri.to_hash.delete_if { |k,v| v.nil? }
 
-        if visit_uri.scheme.nil?
           # TODO - this is only for compatability with previous 2.x behavior that concatenated
           # Capybara.app_host and a "relative" path - Consider removing in 3.0
           # @abotalov brought up a good point about this behavior potentially being useful to people
           # deploying to a subdirectory and/or single page apps where only the url fragment changes
           visit_uri_parts[:path] = uri_base.path + visit_uri.path
-        end
 
-        visit_uri = uri_base.merge(visit_uri_parts)
+          visit_uri = uri_base.merge(visit_uri_parts)
+        else
+          visit_uri.port ||= @server.port if @server && config.always_include_port
+        end
       end
+
       driver.visit(visit_uri.to_s)
     end
 

lib/capybara/spec/session/visit_spec.rb

@@ -80,8 +80,41 @@ Capybara::SpecHelper.spec '#visit' do
       @session.visit('/random')
     end
 
+    it "shouldn't override port if no server", requires: [:server] do
+      session = Capybara::Session.new(@session.mode, nil)
+      expect(session.driver).to receive(:visit).with("http://www.google.com")
+      session.visit("http://www.google.com")
+    end
+
+    it "shouldn't override port if no server but app_host is set", requires: [:server] do
+      session = Capybara::Session.new(@session.mode, nil)
+      Capybara.app_host = "http://www.example.com:6666"
+      expect(session.driver).to receive(:visit).with("http://www.google.com")
+      session.visit("http://www.google.com")
+    end
   end
 
+  context "when Capybara.always_include_port is false" do
+    before(:each) do
+      Capybara.always_include_port = false
+    end
+
+    it "shouldn't overwrite port if app_host is set", requires: [:server] do
+      session = Capybara::Session.new(@session.mode, nil)
+      Capybara.app_host = "http://www.example.com:6666"
+      expect(session.driver).to receive(:visit).with("http://www.google.com")
+      session.visit("http://www.google.com")
+    end
+
+    it "shouldn't overwrite port if port specfified", requires: [:server] do
+      session = Capybara::Session.new(@session.mode, nil)
+      Capybara.app_host = "http://www.example.com:6666"
+      expect(session.driver).to receive(:visit).with("http://www.google.com:99")
+      session.visit("http://www.google.com:99")
+    end
+  end
+
+
   context "without a server", requires: [:server] do
     it "should respect `app_host`" do
       serverless_session = Capybara::Session.new(@session.mode, nil)