Adding force_web_connect #52

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@pyrat

pyrat commented Oct 18, 2010

Hi,

I have altered the implementation as discussed and added a test. It works pretty well I think, feel free to rip it apart and use it to your tastes.

Cheers,
Alastair

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Oct 25, 2010

Owner

Sorry for the late reply but I was away.
Thanks for that solution. I'm glad it works for you.
The issue is that it requires users to set force_net_connect in order to preserve,
original Net::HTTP behaviour and I wouldn't like webmock to change Net::HTTP behaviour by default.
On the other side I don't want the connect to happen, since the whole purpose of webmock,
is to run the tests in isolation :)
I will try to think more about it.

Owner

bblimke commented Oct 25, 2010

Sorry for the late reply but I was away.
Thanks for that solution. I'm glad it works for you.
The issue is that it requires users to set force_net_connect in order to preserve,
original Net::HTTP behaviour and I wouldn't like webmock to change Net::HTTP behaviour by default.
On the other side I don't want the connect to happen, since the whole purpose of webmock,
is to run the tests in isolation :)
I will try to think more about it.

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Nov 1, 2010

Owner

Alastair,

I was thinking about this feature and it's not an easy problem to solve.
HTTP is divided into 3 steps: connect, request, response
All ruby http clients except Net::HTTP only expose request and response steps so
this issue is only relevant to Net::HTTP with start method.
WebMock api also only exposes request and response steps, and that's why
it's not able to easily deal with connect.
The proper solution would probably be to redesign WebMock api to accomodate connect step, where stubbing connections would be separate from stubbing request, so maybe that's what should be done in WebMock 2.0 in the future.

This issue is also only relevant when allow_net_connect is enabled. I can't see a case, where people would like to make real connections with Net::HTTP.start and then stub requests.

I'm going to add an option net_http_connect_on_start to WebMock.allow_net_connect! which can be set to true or false. This option will be false by default for backwards compatibility.

I'm not really happy that webmock modifies default Net::HTTP.start behaviour when required,
but on the other side I don't want to break backwards compatibility, which guarantees that no http connections are made by default when WebMock is required.

Cheers,

Bartosz

Owner

bblimke commented Nov 1, 2010

Alastair,

I was thinking about this feature and it's not an easy problem to solve.
HTTP is divided into 3 steps: connect, request, response
All ruby http clients except Net::HTTP only expose request and response steps so
this issue is only relevant to Net::HTTP with start method.
WebMock api also only exposes request and response steps, and that's why
it's not able to easily deal with connect.
The proper solution would probably be to redesign WebMock api to accomodate connect step, where stubbing connections would be separate from stubbing request, so maybe that's what should be done in WebMock 2.0 in the future.

This issue is also only relevant when allow_net_connect is enabled. I can't see a case, where people would like to make real connections with Net::HTTP.start and then stub requests.

I'm going to add an option net_http_connect_on_start to WebMock.allow_net_connect! which can be set to true or false. This option will be false by default for backwards compatibility.

I'm not really happy that webmock modifies default Net::HTTP.start behaviour when required,
but on the other side I don't want to break backwards compatibility, which guarantees that no http connections are made by default when WebMock is required.

Cheers,

Bartosz

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Nov 2, 2010

Owner

The fix is now in master and will be part of the next release.

Owner

bblimke commented Nov 2, 2010

The fix is now in master and will be part of the next release.

@pyrat

This comment has been minimized.

Show comment Hide comment
@pyrat

pyrat Nov 2, 2010

This looks great.

I think as it is essentially an edge case the solution you have presented solves the issue.

Thanks for responding to this bug and coming up with an elegant solution.

Cheers,
Alastair

pyrat commented Nov 2, 2010

This looks great.

I think as it is essentially an edge case the solution you have presented solves the issue.

Thanks for responding to this bug and coming up with an elegant solution.

Cheers,
Alastair

This issue was closed.

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