vacuum icon indicating copy to clipboard operation
vacuum copied to clipboard

Vacuum:Response body parsing in RSpec

Open TheCorp opened this issue 5 years ago • 21 comments

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

api_response = Vacuum::Request.new(all_the_usual_options).get_items(item_ids: [some_keys], resources: some_resources)
p api_response.parse

Test

VCR.use_cassette('some_VCR') do
    [Test that runs the Code block above]
end

When I run the test above the output of the print api_response.parse returns this error JSON::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:

api_response = Vacuum::Request.new(all_the_usual_options).get_items(item_ids: [some_keys], resources: some_resources)
p JSON.parse(api_response.body.to_s)

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:

     HTTP::StateError:
       body has already been consumed

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.

TheCorp avatar May 20 '20 20:05 TheCorp

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 .to_s is used.

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.

skatkov avatar May 22 '20 19:05 skatkov

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!

TheCorp avatar May 22 '20 19:05 TheCorp

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?

hakanensari avatar May 25 '20 13:05 hakanensari

It seems like we are, we're on webmock (3.8.3) (and http (4.4.1) btw) but it seems like even on these versions, according to that github issue, there are still some issues. As mentioned in the issue though, this workaround in the test seems to resolve the problem by sidestepping VCR entirely.

before { WebMock.disable! }

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:

  • Why this issue doesn't seem to happen for other people?
  • What exactly changes between runs such that the body doesn't look empty on the second run considering the VCR file doesn't change at all between runs?

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!

TheCorp avatar May 25 '20 14:05 TheCorp

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 .to_s on it. Anyways, would love your thoughts if you have a moment and no worries if you have no time, just curious. I had no idea you were a VCR maintainer, thank you for hard work, we use the lib every single day :)

TheCorp avatar May 25 '20 14:05 TheCorp

Do you have a minimum case I can I look at?

krainboltgreene avatar May 25 '20 17:05 krainboltgreene

Not right now but there was a minimal case for the underlying WebMock/HTTP lib issue here in case it helps - https://github.com/httprb/http/issues/212#issuecomment-258587716

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 avatar May 25 '20 17:05 TheCorp

@TheCorp, how about you try to add a failing test to the repo here? Current tests with VCR seem to run fine.

hakanensari avatar May 27 '20 12:05 hakanensari

Good call, will try and repro there and get a minimal case.

TheCorp avatar May 27 '20 13:05 TheCorp

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.

stale[bot] avatar Jul 26 '20 13:07 stale[bot]

Maybe not a great idea to close stale issues? @hakanensari

skatkov avatar Jul 27 '20 09:07 skatkov

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?

TheCorp avatar Jul 27 '20 12:07 TheCorp

@skatkov we can bump daysUntilStale to something higher?

hakanensari avatar Jul 28 '20 10:07 hakanensari

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.

stale[bot] avatar Oct 04 '20 02:10 stale[bot]

@TheCorp any updates on an issue?

skatkov avatar Oct 07 '20 07:10 skatkov

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.

TheCorp avatar Oct 07 '20 14:10 TheCorp

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.

skatkov avatar Oct 08 '20 09:10 skatkov

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.

TheCorp avatar Oct 08 '20 15:10 TheCorp

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.

stale[bot] avatar Mar 20 '21 01:03 stale[bot]

just a note i found a workaround to this by extracting the body via .parse. Per the http gem docs, it seems like it keeps body method available for streaming. i'm limited on my http knowledge (both gem and the general term), but maybe absent a header describing the transfer encoding, it's keeping the connection open.

hope this helps anyone else with the same issue

farverio avatar Nov 26 '21 05:11 farverio

@farverio thats a great solution and better than what I was doing!

TheCorp avatar Nov 26 '21 06:11 TheCorp