Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Let's reduce WebMock's monkey patching #200

Open
myronmarston opened this Issue · 6 comments

3 participants

Myron Marston Mislav Marohnić Bartosz Blimke
Myron Marston
Collaborator

I agree with Jon Leighton who is nervous about the amount of monkey patching WebMock relies on. Monkey patching is a useful technique and I'm glad it's possible in ruby but I think it should be a tool of last resort.

WebMock will probably always have to have at least some level of monkey patching (e.g. for Net/HTTP), but it has more than it needs.

Both Excon and Typhoeus provide public APIs to do stubbing, but WebMock has still chosen to monkey patch them. What's the reason? If WebMock needs more than those public APIs provide, then please work with the authors of those gems to improve the public APIs to meet WebMock's needs. It will make those public APIs better for everyone. It will make things better for you or any future WebMock maintainer because monkey patches are more likely to get broken with new releases of the client gems than public APIs (if the APIs are public, then the gem author has stated an intention for end users to use it and to not change it with a random refactoring).

Let me know if I can help with this.

Mislav Marohnić

Example where monkey patching bites back: Webmock can break net-http-persistent even for requests it is supposed to let through: #188

Myron Marston myronmarston referenced this issue in typhoeus/typhoeus
Closed

Webmock compatibility is broken #196

Bartosz Blimke
Owner

@myronmarston thanks for opening this topic.

The most obvious reason for why adapters are not being improved is lack of time :)

For most http clients there were no hook or mock functionalities available so monkey patching
was the only way I could find.

I have looked at the available stubbing apis, before implementing an adapter, but they were
(at least at the time of implementation) not flexible enough to use them in WebMock.

I don't remember all problems now, but WebMock is designed to use own stub registry,
and most of these stubbing api's were based on their own registries.
I also remember that at least in one case, the stubbing api were too high level and didn't provide enough
low level data for WebMock.

I think WebMock just requires more low level hooks, sth. like around_request , rather than stubbing apis.

I'm very much looking forward to get rid of monkey patching.

Currently the two most problematic adapters are em-http-request and net::http.
As long as I can see some opportunities for em-http-request,
I don't think much can be done about net::http other than smarter monkeypatches.

Bartosz Blimke
Owner

@myronmarston both Typhoeus and Excon adapters use public api now. Shall this issue be now closed or is it supposed to be forever lasting issue? :)

Myron Marston
Collaborator

@bblimke -- that's fantastic news. There are still many other adapters that use monkey patching, though. Ideally, over time WebMock will work with those gem authors to get proper public APIs and start using those. This is a long term improvement that won't be made overnight, of course. If you agree, and find it useful to keep this ticket open to not forget about it, feel free to keep it open. If you'd rather close it, that's fine too. It's up to you.

Bartosz Blimke
Owner

Maybe the best way would be to split it into an issue per adapter. Otherwise it's going to be "never-going-to-be-done-with-unclear-acceptance-criteria-epic-story" ;)

Myron Marston
Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.