VCR 2.8 w/ Faraday 0.9.0 Failures After Upgrading #386
Comments
|
Thanks for reporting this. Not sure when I'll have the time to look into it given my current focus on shipping RSpec 3. Are you willing to look into it? I'd love to merge a PR fixing this... |
|
@myronmarston after everything you do for me and my team? Of course I'll help you out. 😄 Although if you could give me an idea of what you think is happening when it jumps from RSpec to VCR just in general terms I should be able to take it the rest of the way. It seems like there's a lot of meta going on and I'm not exactly sure how the request/response info is getting over to VCR from RSpec. I'd assume that the ultimate problem would be in the faraday adapter in VCR? But I don't see a line the backtrace for that. If you don't have time to explain, I'll try to do it the best I can. |
|
The failure is happening here: Line 497 in f227c0a
vcr/lib/vcr/middleware/faraday.rb Line 75 in f227c0a I think this failure will probably be exposed by updating VCR to run its tests against Faraday 0.9 -- the tests that use faraday should fail with this error and you should be able to go from there to fix it. |
|
Hey guys. I'm seeing the same with VCR 2.8.0 and Faraday 0.9.0. Have you made a start on this, @jfelchner? I might have some time today. |
|
@stuliston I hadn't had time. It's on my docket for this weekend. Totally up if you want to pair on it a bit. |
|
Pairing sounds like a great idea if we can swing it - I'm in Melbourne so time differences will come into it. I have my email add. published on my profile. Fancy dropping me an email to see if we can arrange something? |
|
With a somewhat cursory look, it looks like in Faraday 0.8.x the called {
:method=>:get,
:body=>nil, :url=>#<URI::HTTP:0x00000106859f10 URL:http://localhost:59322/204>,
:request_headers=>{"User-Agent"=>"Faraday v0.8.9"},
:parallel_manager=>nil,
:request=>{:proxy=>nil},
:ssl=>{},
:status=>204,
:response_headers=>{"server"=>"WEBrick/1.3.1 (Ruby/2.1.0/2013-12-25)",
"date"=>"Sun,
16 Feb 2014 03:19:01 GMT",
"connection"=>"close"},
:response=>#<Faraday::Response:0x000001045c7628 @env={...},
@on_complete_callbacks=[]>
}
#<Faraday::Response:0x000001045c7628
@env={
:method=>:get,
:body=>nil,
:url=>#<URI::HTTP:0x00000106859f10 URL:http://localhost:59322/204>,
:request_headers=>{
"User-Agent"=>"Faraday v0.8.9"
},
:parallel_manager=>nil,
:request=>{:proxy=>nil},
:ssl=>{},
:status=>204,
:response_headers=>{
"server"=>"WEBrick/1.3.1 (Ruby/2.1.0/2013-12-25)",
"date"=>"Sun, 16 Feb 2014 03:19:01 GMT",
"connection"=>"close"
},
:response=>#<Faraday::Response:0x000001045c7628 ...>
},
@on_complete_callbacks=[]
>Whereas in Faraday 0.9.x, the called #<Faraday::Env
@method=:get
@body=""
@url=#<URI::HTTP:0x00000101c49288 URL:http://localhost:59094/204>
@request=#<Faraday::RequestOptions (empty)>
@request_headers={"User-Agent"=>"Faraday v0.9.0"}
@ssl=#<Faraday::SSLOptions (empty)>
@response_headers={
"server"=>"WEBrick/1.3.1 (Ruby/2.1.0/2013-12-25)",
"date"=>"Sun, 16 Feb 2014 03:17:51 GMT",
"connection"=>"close"
}
@status=204>
nilIt doesn't appear to carry a |
|
@nbibler, @stuliston and I took a look tonight. I really appreciate you posting this. We should have a PR to merge in 24-48 hours. |
|
Thanks for looking into this, @jfelchner, @nbibler and @stuliston! Ideally, I'd like for Faraday < 0.9 to still be supported so that VCR users aren't blocked from upgrading because they are still on Faraday 0.8 or earlier. When there was a similar large change for Typhoeus, I set things up to run the build against Typhoeus 0.4 and the current version of Typhoeus -- see ec845e2. Would be nice to do something similar for Faraday. In fact, I think we can re-use the |
|
I'm adding this comment just for future readers. I think I may have tracked down the problem. It has nothing to do with the changes made to how the response is handled as far as I can tell. What appears to be the issue is that something along the way is creating a new If I lock So what is happening is that Faraday is setting the response on the I'm about 90% sure this is where the issue is. |
|
Ok, I just tracked down the issue. I think this may very well be a Apologies in advance for my liberal use of italics. 😉 SetupInside of rack_builder.rb there's a bit that looks like this and has not changed substantially for quite some time: def app
@app ||= begin
lock!
to_app(lambda { |env|
response = Response.new
response.finish(env) unless env.parallel?
env.response = response
})
end
endWhat has changed is the commit I pointed to above. Specifically the liberal usage of For easier storytelling here's a snippet from the VCR Faraday Adapter Middleware Thingamabob(tm). There is a method in there called def on_recordable_request
@has_on_complete_hook = true
app.call(env).on_complete do |env|
VCR.record_http_interaction(VCR::HTTPInteraction.new(vcr_request, response_for(env)))
invoke_after_request_hook(response_for(env)) if delay_finishing?
end
endScenarioNow that we have the setup out of the way, here's the 🍖
Here's the problem
I hope that was clear enough, but if not, just let me know and I'll try to describe it in a different way. I would love to get this thing wrapped up and send someone a PR, but I figured @myronmarston and @technoweenie needed to have a discussion on whether the middleware is being used incorrectly or if there is actually a bug. |
|
Thanks for the detailed writeup, @jfelchner. Sounds like a pretty subtle issue. @mislav -- you've helped a bit with VCR maintenance and I know you work on Faraday as well. Can you advise if you think this is a Faraday bug or if VCR's middleware is making invalid assumptions about the faraday env? |
|
So is it safe to say that VCR is not usable with the latest version of Faraday? We're getting the |
|
@zuk: Correct. I'm still not sure if it's a bug in Faraday or VCR. Can someone who works on Faraday weigh in? /cc @mislav @sferik @technoweenie |
|
It's a bug with Faraday, but can be worked around in VCR. I got a fix a week ago, I just need to get around to testing it and submitting a PR. I was battling with VCR's own test suite and latest Typhoeus being broken with Faraday 0.9. |
|
Thanks for the update, @mislav. If Faraday will have a fix released in the near future I'd prefer not to work around it in VCR (unless the work around is pretty trivial). |
|
I'm affected by this too in a new project with latest versions of Faraday and VCR. I'd like to know when a fix becomes available. |
|
A fix will be available whenever someone submits one. |
|
@myronmarston I agree that if the bug is in Faraday we shouldn't fix it here so I'm going to forgo a PR. However if @mislav can point me to anything he needs help with on getting the PR submitted to Faraday, I'd be happy to help. |
|
Is there a way to work around this issue until it is fixed? |
You can downgrade to Faraday 0.8.x. You can also try changing |
|
Is there a Faraday PR/issue page for this? Having run my head into it repeatedly this morning, I'd like to keep an eye on the problem. |
|
@mislav again, I'd love to help, just let me know what you need. This is a fairly bad bug and I'd like to get a fix in faraday ASAP. I'll have a little time this weekend to submit a PR. |
|
Has there been any resolution for this? |
|
Not as far as I know. |
|
to clarify this to whoever ends up reading this, since I've gotten a few questions regarding this... VCR can hook into Faraday and handle the mocking of requests cleanly, without needing to worry about which HTTP backend you are using with Faraday. However, this clean mocking is broken with Faraday 0.9 Even though this is broken, you can still choose to use one of VCR's other mocking backends, but it means that you must choose the mocking backend that matches the HTTP backend you are using. @myronmarston described which you should use above. The default HTTP backend for Faraday is Net::HTTP, so that means that you need to use webmock. To do so, switch to saying |
|
I honestly have zero idea why there has been no response on this from @mislav since March. I'd be happy with a 'I am not going to fix this for another 6 months. Deal with it.' But the radio silence is kinda weird. |
|
@myronmarston @ctaintor: thanks for the detailed explanation of workarounds! @jfelchner: @mislav is very busy and has lots of responsibilities and is traveling the world :-) That said, @mislav do you have that fix you mentioned handy that you can throw into a gist so someone else can put together a PR? |
|
@jjb not saying he's not busy, but he did say he had a fix and was going to implement it. I also asked him repeatedly if he would like me to do the fix. It doesn't take a lot of time to say: "Yeah, I'm not going to have time to do this, can you submit a PR and we can look at it?" but I don't want to do the work if he already has a fix that he wants to use and will reject my PR. Maybe if we ping @sferik as well... |
|
@jfelchner sure, but everyone contributes as much as they can, and things fall between the cracks. there's no perfect way to prioritize. @mislav maintains a ton of projects. |
|
@jjb @jfelchner: @mislav arrived in Berlin yesterday and will be working next to me today, so I can nag him about this. |
|
🍠 |
|
Fixing this now |
See a similar issue there : vcr/vcr#386
…onse object. a fix is available but has not yet been released: vcr/vcr#386
See a similar issue there : vcr/vcr#386
|
I had this issue and could not use an older version of Faraday but found another solution using webmock instead of faraday in the config: That worked for me with vcr 2.9.3 and faraday 0.9.1 |
|
I ran into this with 2.9.3 and faraday 0.9.1, rolling back to 0.8.9 fixed it for me |
This shouldn't be strictly necessary, but we were encountering this problem which stems from a Faraday bug: vcr/vcr#386 There's a fix for VCR but it isn't released yet and the maintainer has expressed that he can't get to it: vcr/vcr#439 This fix shouldn't even need to be in VCR when it's Faraday's bug, but that's apparently unlikely to get fixed either: lostisland/faraday#454 Sheesh. We'll move away from Faraday but that's going to be a substantial change...
Synopsis -------- The `Env` that gets stored on the `Response` does not contain a reference to that response. This env is a copy of the env that was passed through the middleware stack. Thus if the env passed to `app.call` is used, it has the response, but if the one from the `Response` is used, then it does not. This happens when, for example, you invoke the `on_complete` hook on the response, after it has finished. Fixes Longstanding VCR issue ---------------------------- Fixes vcr/vcr#386 A longstanding issue in VCR's integration with v0.9.x It was worked around in this commit: vcr/vcr#439 And is merged into master, but is not released. Merging this will fix the issue, without the changes to VCR needing to be released. The old code, which is currently released, and gets its env from the Request can be seen [here](https://github.com/vcr/vcr/blob/cb9696c44dee4c0d39b4f531e72516f4bc57272f%5E/lib/vcr/middleware/faraday.rb#L99-105) Fixes Faraday Issue ------------------- Fixes lostisland#441 which identifies this same problem. Closes Faraday PR ----------------- I'm going to recommend closing https://github.com/lostisland/faraday/pull/360/files as the issue was probably introduced [here](lostisland@04a8514#diff-fc9726b31994223d11a212b5169100fdL68) when `Response#finish` changed from storing the `env` directly, to storing a copy: ``` - @env = env + @env = Env.from(env) ``` Presumably this behaviour is still desired, but the mentioned pull essentially undoes this by returning the same `env` from `Env.from` The commit that introduced the issue is probably good, but is called by the app in the middleware stack that creates the response, before it sets the rsponse onto the env. This means that the copy does not have the link to the response. https://github.com/lostisland/faraday/blob/81f16593a0138ec58bb6f25e1c2804e91589662f/lib/faraday/rack_builder.rb#L152-155 Not sure about the test name/location ------------------------------------- I had difficulty figuring out where to put this test, and what to name it. I eventually went with the test on the `middleware_stack_test.rb`, because it deals with the `RackBuilder`, but if there's a better place, or a better name, let me know and I'll update it. One other weird thing --------------------- Depending on when the `Request#on_complete` callback is invoked, you're either getting the original env, or the copy: https://github.com/lostisland/faraday/blob/81f16593a0138ec58bb6f25e1c2804e91589662f/lib/faraday/response.rb#L64-65
|
I have a PR to Faraday to fix this issue. But thought It'd be fun to offer a command-line script that will apply a workaround to VCR, until it's merged. Obviously not sufficient for anyone deploying or sharing their code, but it will get your installed VCR working: $ ruby -i -pe 'gsub "(env)", "(@env)" if /\.on_complete/../end/' `gem which vcr/middleware/faraday`(side note: how cool would it be if Bundler had support for things like this :P) |
|
Actually, pointing your Gemfile at master probably works, b/c @mislav has a commit on master to do the same thing: https://github.com/vcr/vcr/pull/439/files#diff-6c87378c151835838393fe52399d0448R103 |
…onse object. a fix is available but has not yet been released: vcr/vcr#386
See a similar issue there : vcr/vcr#386
As much help as 'VCR 2.8 w/ Faraday 0.9.0 Failures After Upgrading' is, I think a backtrace would be a bit more helpful 😄
FWIW, Faraday 0.8.9 works correctly.
The text was updated successfully, but these errors were encountered: