search

ropensci / vcr

Record and replay HTTP requests
https://docs.ropensci.org/vcr
Other
77 stars 12 forks source link

Use again body_from for the body of the response #252

Closed llrs closed 2 years ago

llrs commented 2 years ago

Description

This PR reverts some changes as discussed in #249 to handle the body again with the body_from function and adds a new test for it.

Related Issue

249

Example

I'm not sure how to add tests for this, but I added the test as reported in #249 in a new file test-issues.

Tests and checks pass but I'm not sure if the issue is fully resolved.

sckott commented 2 years ago

@llrs can you add back body_from in request class as well. In the offending commit https://github.com/ropensci/vcr/commit/cf5ee5bc88b1bb840e8b7c0d6e1a556151b0454c#diff-a947054c47f80b7252dac92d05ce9294c02a7671022a290403913f0bcfa4c197R150-R151 it was removed from both request and response class nevermind, it's already changed on master

sckott commented 2 years ago

that test I don't think really tests the change - not sure quite how to do it yet, but maybe we merge and think about that later

llrs commented 2 years ago

Before the change it didn't pass and after the change it did, but I wasn't sure if it was the best way to test the effects (I knew it was not minimal).

KevCaz commented 2 years ago

@sckott @llrs maybe this is rather minimal and reflects the change: 

body <- NULL
x <- VcrResponse$new(200, list('content-length' = nchar(body)), body, "HTTP/2") # took this piece from one of your examples

test_that('issue 249 is correctly handled.', {
  expect_equal(x$from_hash(x$to_hash())$body, character(1))
})

before was NULL now it's "".

sckott commented 2 years ago

thanks @KevCaz - may add that after