Excon 0.17.0 makes the entire world explode #256

Closed
nate opened this Issue Feb 21, 2013 · 20 comments

Comments

Projects
None yet
6 participants
@nate

nate commented Feb 21, 2013

Excon 0.17.0 includes some refactoring of its internals and thus, of course, breaks WebMock. When I run WebMock's spec I get 186 failures.

Maybe we can pull @geemus in on this? or @dim?

@nate

This comment has been minimized.

Show comment Hide comment
@nate

nate Feb 21, 2013

I confess I'm at a loss since I'm not familiar with how WebMock works internally. If it's not a simple fix then maybe I can dig in.

nate commented Feb 21, 2013

I confess I'm at a loss since I'm not familiar with how WebMock works internally. If it's not a simple fix then maybe I can dig in.

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Feb 21, 2013

Owner

I wish Excon had a stable API which webmock could just hook into (same as it was done with Typhoeus recently), and not worry about Excon internals anymore.
I haven't had time to look what has changed in Excon yet.

Owner

bblimke commented Feb 21, 2013

I wish Excon had a stable API which webmock could just hook into (same as it was done with Typhoeus recently), and not worry about Excon internals anymore.
I haven't had time to look what has changed in Excon yet.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Feb 21, 2013

Ouch indeed.

@bblimke - Excon does have a stable API actually, in particular the mock/stub functionality should (I hope) provide a public non-changing interface that webmock could hook into. VCR for instance does all its excon stuff through this interface (and to the best of my knowledge did not break against 0.17.0).

I'm not that familiar with the WebMock side of this but I'm definitely happy to help out fixing the integration. In particular if we can aim it at excons stubs I think it should be easier to work with and more stable (the stub interface hasn't changed much, if at all, but I still have some more refactoring in mind moving forward).

geemus commented Feb 21, 2013

Ouch indeed.

@bblimke - Excon does have a stable API actually, in particular the mock/stub functionality should (I hope) provide a public non-changing interface that webmock could hook into. VCR for instance does all its excon stuff through this interface (and to the best of my knowledge did not break against 0.17.0).

I'm not that familiar with the WebMock side of this but I'm definitely happy to help out fixing the integration. In particular if we can aim it at excons stubs I think it should be easier to work with and more stable (the stub interface hasn't changed much, if at all, but I still have some more refactoring in mind moving forward).

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Feb 21, 2013

Owner

@geemus thanks, I'll have a look at stubbing interface and see if it's now enough for WebMock. At the time when Excon adapter was written, stubbing interface was not enough.

Owner

bblimke commented Feb 21, 2013

@geemus thanks, I'll have a look at stubbing interface and see if it's now enough for WebMock. At the time when Excon adapter was written, stubbing interface was not enough.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Feb 21, 2013

@bblimke - got it. Do let me know if you find things missing, it was originally built out to accomodate some of VCR use cases and it definitely helped to have a lot of discussions with it's maintainer at the time. I definitely want to support you in having this working, but I think using that interface is going to be the cleaner and less brittle way if we can work it out for you. Again, sorry for the breakage and just let me know if you have any questions or concerns so that we can get you back in business asap.

geemus commented Feb 21, 2013

@bblimke - got it. Do let me know if you find things missing, it was originally built out to accomodate some of VCR use cases and it definitely helped to have a lot of discussions with it's maintainer at the time. I definitely want to support you in having this working, but I think using that interface is going to be the cleaner and less brittle way if we can work it out for you. Again, sorry for the breakage and just let me know if you have any questions or concerns so that we can get you back in business asap.

@dim

This comment has been minimized.

Show comment Hide comment
@dim

dim Feb 22, 2013

Contributor

I could take a look, as I did the original integration, but I won't be able to do it until later next week.

Contributor

dim commented Feb 22, 2013

I could take a look, as I did the original integration, but I won't be able to do it until later next week.

@atombender

This comment has been minimized.

Show comment Hide comment
@atombender

atombender Mar 1, 2013

The problem is that WebMock overrides request_kernel, which no longer exists in 0.17. Looks like the internals have changed a lot; probably not a trivial fix.

The problem is that WebMock overrides request_kernel, which no longer exists in 0.17. Looks like the internals have changed a lot; probably not a trivial fix.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Mar 1, 2013

@alexstaubo - yeah, there were some large-ish changes. I'm hopeful that the existing stub infrastructure within excon can accomodate webmock's needs (or that we can adapt it to do so). Ideally we wouldn't get back into overriding internals (as I am reticent to promise they will remain the same indefinitely).

geemus commented Mar 1, 2013

@alexstaubo - yeah, there were some large-ish changes. I'm hopeful that the existing stub infrastructure within excon can accomodate webmock's needs (or that we can adapt it to do so). Ideally we wouldn't get back into overriding internals (as I am reticent to promise they will remain the same indefinitely).

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Mar 2, 2013

Owner

@geemus I had a look at the current Excon implementation. I could use the same technique as @myronmarston used in Excon adapter in VCR. The only potential issue is conflict with other stubs. I.e. if WebMock and VCR are both loaded only one stub should be active.

I also thought about writing a new WebMock middleware for Excon and inject it into Excon dynamically, which would be fairly easy with current middleware infrastructure. The only issue is that all middleware calls are executed before Connection call. Since WebMock also needs to invoke after request callbacks, I would need a way to intercept request and response after the a real http request is executed.

Owner

bblimke commented Mar 2, 2013

@geemus I had a look at the current Excon implementation. I could use the same technique as @myronmarston used in Excon adapter in VCR. The only potential issue is conflict with other stubs. I.e. if WebMock and VCR are both loaded only one stub should be active.

I also thought about writing a new WebMock middleware for Excon and inject it into Excon dynamically, which would be fairly easy with current middleware infrastructure. The only issue is that all middleware calls are executed before Connection call. Since WebMock also needs to invoke after request callbacks, I would need a way to intercept request and response after the a real http request is executed.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Mar 2, 2013

@bblimke - I hadn't considered the VCR + WebMock case. What happens now for other adapters in this case?

I think the middleware solution might work if stubs don't (see questions above), you can do separate things to execute actions after the response also (see for instance the instrumentor middleware). I don't know if just injecting it is the right thing or not, as it could cause some issues in terms of load order/setup. Maybe better to just tell users to add the middleware themselves to avoid this.

geemus commented Mar 2, 2013

@bblimke - I hadn't considered the VCR + WebMock case. What happens now for other adapters in this case?

I think the middleware solution might work if stubs don't (see questions above), you can do separate things to execute actions after the response also (see for instance the instrumentor middleware). I don't know if just injecting it is the right thing or not, as it could cause some issues in terms of load order/setup. Maybe better to just tell users to add the middleware themselves to avoid this.

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Mar 4, 2013

Owner

@geemus It depends on adapter. VCR uses WebMock as an adapter for some libs and has own adapters for others (i.e. excon). VCR would have to load excon adapter if WebMock is not present or remove an Excon stub created by WebMock, before adding own stub.

Owner

bblimke commented Mar 4, 2013

@geemus It depends on adapter. VCR uses WebMock as an adapter for some libs and has own adapters for others (i.e. excon). VCR would have to load excon adapter if WebMock is not present or remove an Excon stub created by WebMock, before adding own stub.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Mar 4, 2013

Ah. @myronmarston - thoughts on how to do this reasonably?

geemus commented Mar 4, 2013

Ah. @myronmarston - thoughts on how to do this reasonably?

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Mar 4, 2013

Owner

I started rewrite of Excon adapter to use Excon stubs, same as VCR. VCR can just do WebMock::HttpLibAdapters::ExconAdapter.disable! if needed.

Owner

bblimke commented Mar 4, 2013

I started rewrite of Excon adapter to use Excon stubs, same as VCR. VCR can just do WebMock::HttpLibAdapters::ExconAdapter.disable! if needed.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Mar 4, 2013

@bblimke - great, thanks for working through this. Just let me know if you get caught on anything else I can help with.

geemus commented Mar 4, 2013

@bblimke - great, thanks for working through this. Just let me know if you get caught on anything else I can help with.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Mar 4, 2013

Owner

@myronmarston excellent :)

btw. it looks like VCR's Excon adapter is not compatible with new Excon too. i.e connection hash is now called data.

Owner

bblimke commented Mar 4, 2013

@myronmarston excellent :)

btw. it looks like VCR's Excon adapter is not compatible with new Excon too. i.e connection hash is now called data.

@bblimke

This comment has been minimized.

Show comment Hide comment
@bblimke

bblimke Mar 4, 2013

Owner

ok. The new Excon adapter is now released in WebMock 1.11.0 and uses same mechanism as VCR (thanks @myronmarston :) utilising Excon's mock middleware.

@myronmarston VCR Excon adapter will have to set ::Excon.defaults[:mock] to true, after disabling WebMock's ExconAdapter. WebMock sets this value back to false when disabled because it sets it to true when enabled.

Owner

bblimke commented Mar 4, 2013

ok. The new Excon adapter is now released in WebMock 1.11.0 and uses same mechanism as VCR (thanks @myronmarston :) utilising Excon's mock middleware.

@myronmarston VCR Excon adapter will have to set ::Excon.defaults[:mock] to true, after disabling WebMock's ExconAdapter. WebMock sets this value back to false when disabled because it sets it to true when enabled.

@bblimke bblimke closed this Mar 4, 2013

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Mar 4, 2013

@bblimke - great, thanks!

geemus commented Mar 4, 2013

@bblimke - great, thanks!

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Mar 4, 2013

Collaborator

Glad to see we've averted the explosion of the whole world :).

My open source time these days has mostly been focused on rspec, but I'll try to get to updating VCR soon.

Collaborator

myronmarston commented Mar 4, 2013

Glad to see we've averted the explosion of the whole world :).

My open source time these days has mostly been focused on rspec, but I'll try to get to updating VCR soon.

@atombender

This comment has been minimized.

Show comment Hide comment
@atombender

atombender Mar 4, 2013

@bblimke: Great work fixing this so fast.

@bblimke: Great work fixing this so fast.

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