Skip to content
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

Open
TheCorp opened this issue May 20, 2020 · 21 comments
Open

Vacuum:Response body parsing in RSpec #92

TheCorp opened this issue May 20, 2020 · 21 comments
Labels

Comments

@TheCorp
Copy link

TheCorp commented May 20, 2020

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.

@skatkov
Copy link
Collaborator

skatkov commented May 22, 2020

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.

@TheCorp
Copy link
Author

TheCorp commented May 22, 2020

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!

@hakanensari
Copy link
Owner

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?

@TheCorp
Copy link
Author

TheCorp commented May 25, 2020

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
Copy link
Author

TheCorp commented May 25, 2020

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 :)

@krainboltgreene
Copy link

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

@TheCorp
Copy link
Author

TheCorp commented May 25, 2020

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.

@hakanensari
Copy link
Owner

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

@TheCorp
Copy link
Author

TheCorp commented May 27, 2020

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

@stale
Copy link

stale bot commented Jul 26, 2020

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 stale bot added the wontfix label Jul 26, 2020
@skatkov
Copy link
Collaborator

skatkov commented Jul 27, 2020

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

@stale stale bot removed the wontfix label Jul 27, 2020
@TheCorp
Copy link
Author

TheCorp commented Jul 27, 2020

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?

@hakanensari
Copy link
Owner

@skatkov we can bump daysUntilStale to something higher?

@stale
Copy link

stale bot commented Oct 4, 2020

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 stale bot added the wontfix label Oct 4, 2020
@skatkov
Copy link
Collaborator

skatkov commented Oct 7, 2020

@TheCorp any updates on an issue?

@stale stale bot removed the wontfix label Oct 7, 2020
@TheCorp
Copy link
Author

TheCorp commented Oct 7, 2020

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.

@skatkov
Copy link
Collaborator

skatkov commented Oct 8, 2020

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.

@TheCorp
Copy link
Author

TheCorp commented Oct 8, 2020

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.

@stale
Copy link

stale bot commented Mar 20, 2021

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 stale bot added the wontfix label Mar 20, 2021
@farverio
Copy link

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

@TheCorp
Copy link
Author

TheCorp commented Nov 26, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants