Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make absolute uri test more general #1227

Closed
wants to merge 1 commit into from

3 participants

@strzalek

This test had hardcoded values which may vary for different apps and while this specs are integrated in custom driver's test suite like capybara-mechanize, it fails becouse of that (https://travis-ci.org/jeroenvandijk/capybara-mechanize/jobs/17002693#L153)

Problem occurred when I was trying to bump up capybara version for capybara-mechanize gem where app have different host (www.local.com not localhost) and don't have /foo/bar endpoint.

@strzalek strzalek Make absolute uri test more general
Don't wrongly assume that app have /foo/bar path and it's host is localhost
e923eda
@strzalek strzalek referenced this pull request in jeroenvandijk/capybara-mechanize
Closed

Change capybara dep #55

@twalpole twalpole commented on the diff
lib/capybara/spec/session/visit_spec.rb
@@ -19,10 +19,10 @@
it "should fetch a response when absolute URI doesn't have a trailing slash" do
# Preparation
- @session.visit('/foo/bar')
+ @session.visit('/')
@twalpole Collaborator

This change negates the value of the whole test, since the page will now already have the content 'Hello World!'

I can understand that, however that's very unlike I think. We're making request for / which will in fact return Hello World in it's body but then we're doing another request by using previous request's host and port. Now, it'll either complete new request successfully returning new content (which will be the same as previous as we're requesting same resource but differently) or will it'll throw an error and we won't reach following assertion. It think that this makes sense and there's nothing to be afraid of, also test above does the same assumption (for the opposite case with trailing slash): https://github.com/jnicklas/capybara/blob/master/lib/capybara/spec/session/visit_spec.rb#L11-L15

What do you think?

@jnicklas Owner

I agree with @twalpole. IIRC the point of this test was navigating from a page which has a path to one which does not.

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

Ping.

@twalpole
Collaborator

Since this change negates the original purpose of the test I'm closing this PR

@twalpole twalpole closed this
@phillbaker phillbaker referenced this pull request from a commit in phillbaker/capybara
@phillbaker phillbaker Replace hardcoded `localhost` in spec helper
This test had a hardcoded values for the URI to visit which varies for different test cases. When these specs are integrated in custom driver's test suite like capybara-mechanize, it leads to failures.

This replaces #1227, which was rejected for changing the original purpose of the test. However, this patch only changes the hardcoding. It will allow jeroenvandijk/capybara-mechanize#55 to proceed and bump it's compatibility to beyond capybara 2.1.
dfa268b
@phillbaker phillbaker referenced this pull request from a commit in phillbaker/capybara
@phillbaker phillbaker Replace hardcoded `localhost` in spec helper.
This test had a hardcoded values for the URI to visit which varies for different test cases. When these specs are integrated in custom driver's test suite like capybara-mechanize, it leads to failures.

This replaces #1227, which was rejected for changing the original purpose of the test. However, this patch only changes the hardcoding. It will allow jeroenvandijk/capybara-mechanize#55 to proceed and bump it's compatibility to beyond capybara 2.1.
d9bb8ce
@twalpole twalpole referenced this pull request from a commit
@phillbaker phillbaker Replace hardcoded `localhost` in spec helper.
This test had a hardcoded values for the URI to visit which varies for different test cases. When these specs are integrated in custom driver's test suite like capybara-mechanize, it leads to failures.

This replaces #1227, which was rejected for changing the original purpose of the test. However, this patch only changes the hardcoding. It will allow jeroenvandijk/capybara-mechanize#55 to proceed and bump it's compatibility to beyond capybara 2.1.
f9077ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 16, 2014
  1. @strzalek

    Make absolute uri test more general

    strzalek authored
    Don't wrongly assume that app have /foo/bar path and it's host is localhost
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +2 −2 lib/capybara/spec/session/visit_spec.rb
View
4 lib/capybara/spec/session/visit_spec.rb
@@ -19,10 +19,10 @@
it "should fetch a response when absolute URI doesn't have a trailing slash" do
# Preparation
- @session.visit('/foo/bar')
+ @session.visit('/')
@twalpole Collaborator

This change negates the value of the whole test, since the page will now already have the content 'Hello World!'

I can understand that, however that's very unlike I think. We're making request for / which will in fact return Hello World in it's body but then we're doing another request by using previous request's host and port. Now, it'll either complete new request successfully returning new content (which will be the same as previous as we're requesting same resource but differently) or will it'll throw an error and we won't reach following assertion. It think that this makes sense and there's nothing to be afraid of, also test above does the same assumption (for the opposite case with trailing slash): https://github.com/jnicklas/capybara/blob/master/lib/capybara/spec/session/visit_spec.rb#L11-L15

What do you think?

@jnicklas Owner

I agree with @twalpole. IIRC the point of this test was navigating from a page which has a path to one which does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
root_uri = URI.parse(@session.current_url)
- @session.visit("http://localhost:#{root_uri.port}")
+ @session.visit("http://#{root_uri.host}:#{root_uri.port}")
@session.should have_content('Hello world!')
end
Something went wrong with that request. Please try again.