Make absolute uri test more general #1227

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@lukaszx0

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.

Make absolute uri test more general
Don't wrongly assume that app have /foo/bar path and it's host is localhost

@lukaszx0 lukaszx0 referenced this pull request in jeroenvandijk/capybara-mechanize Jan 16, 2014

Closed

Change capybara dep #55

@@ -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('/')

This comment has been minimized.

Show comment Hide comment
@twalpole

twalpole Jan 17, 2014

Collaborator

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

@twalpole

twalpole Jan 17, 2014

Collaborator

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

This comment has been minimized.

Show comment Hide comment
@lukaszx0

lukaszx0 Jan 17, 2014

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?

@lukaszx0

lukaszx0 Jan 17, 2014

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?

This comment has been minimized.

Show comment Hide comment
@jnicklas

jnicklas Jan 31, 2014

Collaborator

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

@jnicklas

jnicklas Jan 31, 2014

Collaborator

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

@lukaszx0

This comment has been minimized.

Show comment Hide comment
@lukaszx0

lukaszx0 Jan 31, 2014

Ping.

Ping.

@twalpole

This comment has been minimized.

Show comment Hide comment
@twalpole

twalpole Apr 2, 2014

Collaborator

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

Collaborator

twalpole commented Apr 2, 2014

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

@twalpole twalpole closed this Apr 2, 2014

phillbaker added a commit to phillbaker/capybara that referenced this pull request Oct 5, 2014

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.

phillbaker added a commit to phillbaker/capybara that referenced this pull request Oct 5, 2014

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.

twalpole added a commit that referenced this pull request Oct 13, 2014

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment