-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vacuum:Response body parsing in RSpec #92
Comments
Thanks for reporting. I think I did see something similar in my codebase, not only in a testsuite, but also in production enviroment. It does seem to be working for me without any issues if Not sure what exactly is causing this, but my first guess would be to double-check some of our dependencies. Unfortunately, don't have time right now to dig into this. But will be happy to help you merge and review if you have a proposed solution. |
No problem, our solution seems to work fine now and the only reason I am using it is mainly to make our tests work correctly on the first time but it would definitely be more satisfying to use the code correctly as you guys had intended. I wonder if there's something about the HTTP library and how Vacuum uses it where streaming the content body somehow doesn't finish the stream and flush the contents to the body variable until it is run a second time in the test environment. I'll dig into it as well and thanks for your comments! |
This seems to be related to this. That said, I can't replicate it here in the tests, neither have I run into this elsewhere, where I use the library. Perhaps a silly question, but are you using the latest version of Webmock and so on? |
It seems like we are, we're on
Once I include that obviously things work but it's a little depressing since really it just seems like there's some issue with streaming and how the HTTP response object isn't closed which makes VCR and Vacuum think the body is empty. Maybe there's a better way to flush the response in Vacuum (or our app code). It's always a little unsatisfying to actually change your app code to make your tests work but I will survive if we have to go to production with code like this :) Things that are still confusing:
I'm going to play around with some streaming configuration and see if that helps. Appreciate the responses here guys and thanks for building the gem, it's been a life saver for us! |
Hey @krainboltgreene (long time no talk, its Jason Joseph, Arjan Singh's friend!) I just realized you did the most recent VCR release (https://github.com/vcr/vcr/releases/tag/v5.1.0) and I saw a few mentions in the commits about how VCR handles streaming (I think for Excon and Faraday). Sorry if it's rude to pull you into this issue but was just curious if you thought there were any issues with how VCR interacts with the HTTP.rb lib and streamed responses. I'm getting a very weird error above where a VCR'd response will look blank upon the first test run (even though the file itself looks completely correct and valid) and then works fine the second run. My workaround is basically sidestepping streaming and flushing the entire response by calling |
Do you have a minimum case I can I look at? |
Not right now but there was a minimal case for the underlying WebMock/HTTP lib issue here in case it helps - httprb/http#212 (comment) I'll see if I can build a minimal case for this specific issue with vacuum/VCR that seems like it might be related to the underlying issue above. |
@TheCorp, how about you try to add a failing test to the repo here? Current tests with VCR seem to run fine. |
Good call, will try and repro there and get a minimal case. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Maybe not a great idea to close stale issues? @hakanensari |
Still trying to get time to dig into this issue, work has been a monster during covid :( I understand if you want to close this, I can just re-open later or make a new issue? |
@skatkov we can bump |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@TheCorp any updates on an issue? |
It's still affecting me and I still haven't had time to build a minimal repro case unfortunately. If you guys want to prune this and close it I can always reopen later with the minimal case. |
Since the issue is still relevant, don't want to close it -- so I bumped up daysUntilStale in #96 let's see how that will work out. |
Appreciate that! The issue itself is still quite confusing to me but when I tried spending some time isolating the issue I couldn't really get it to work. Will get back to you guys once I have a bit more time. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
just a note i found a workaround to this by extracting the body via hope this helps anyone else with the same issue |
@farverio thats a great solution and better than what I was doing! |
I am not sure what I am doing wrong here but I have a weird issue in my rspec tests, this is psuedocode but should give you an idea of what I am trying to do:
Code being tested
Test
When I run the test above the output of the print
api_response.parse
returns this errorJSON::ParserError: Empty input () at line 1, column 0 [sparse.c:838]
because the body value is blank on the first run of the test but returns results on all subsequent runs. There is no change in the actual contents of the VCR file between runs.If I change my code to this, it works the first time:
The closest I have come to understanding what's going on is that during one of my tests when I tried to just unload the entire Vacuum::Response object
.to_json
and it returned this error on the first run:Any idea what I might be doing wrong here? I don't know if it matters but I did this using the Vacuum::Matcher as well as without it and it didn't seem to make a difference.
The text was updated successfully, but these errors were encountered: