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

Record json-encoded bodies #130

Merged
merged 8 commits into from
Dec 19, 2019
Merged

Record json-encoded bodies #130

merged 8 commits into from
Dec 19, 2019

Conversation

aaronwolen
Copy link
Member

@aaronwolen aaronwolen commented Dec 13, 2019

Apologies for the barrage PRs, I'm working on adding mocked-tests to osfr 🙂.

This is related to the issue addressed in webmockr PR#82, which prevented json-encoded bodies from being recorded to cassettes.

I added a new test file that basically mirrors tests/testthat/test-ause_cassette_match_requests_on.R albeit with JSON encoded requests. This might be overkill.

It'd be nice to have an exported version of pluck_body() somewhere that could be used in both packages but I'm not sure where that should go...

Let me know if you have any questions/suggestions.

@sckott
Copy link
Collaborator

sckott commented Dec 13, 2019

thanks - and glad to see more usage! First to address the pluck_body issue:

I think it makes sense to export pluck_body - since webmockr is in Imports in vcr and vcr is only in Suggests in webmockr I think we move the function to webmockr, export it from there, then vcr can use it from webmockr - sound good?

@aaronwolen
Copy link
Member Author

Totally agree. Will update the PRs accordingly.

@sckott
Copy link
Collaborator

sckott commented Dec 13, 2019

thanks

@aaronwolen
Copy link
Member Author

aaronwolen commented Dec 17, 2019

Still working on this...

Currently the following test fails

out4 <- use_cassette("httr_post_upload_file", {
  b <- POST("https://httpbin.org/post",
    body = list(y = httr::upload_file(system.file("CITATION"))))
})
...
expect_match(strj$data, "bibentry\\(")

because webmockr::pluck_body() string-encodes form_file objects.

@sckott
Copy link
Collaborator

sckott commented Dec 19, 2019

I think i got webmockr and vcr working locally - for vcr:

add if (inherits(x, "form_file")) return(FALSE) as the first line in this function https://github.com/ropensci/vcr/blob/master/R/request_class.R#L171-L175

and in webmockr in RequestSignature:

in the print method, replace cat_foo(self$body) with

if (inherits(self$body, "form_file"))
    cat(paste0("     ",
        sprintf("type=%s; path=%s", self$body$type, self$body$path)),
        sep = "\n")
else
    cat_foo(self$body)

and replace the to_string method at the bottom of the RequestSignature file with:

to_string <- function(x) {
  if (inherits(x, "list") && all(nchar(names(x)) > 0)) {
    tmp <- paste0(paste(names(x), x, sep = ": "), collapse = ", ")
  } else if (inherits(x, "list") && any(nchar(names(x)) == 0)) {
    tmp <- paste0(paste(names(x), x, sep = ": "), collapse = ", ")
  } else if (inherits(x, "form_file")) {
    tmp <- sprintf("type=%s; path=%s", x$type, x$path)
  } else {
    tmp <- paste0(x, collapse = ", ")
  }
  return(sprintf("{%s}", tmp))
}

With those changes, everything works locally for me, thoughts?

@aaronwolen
Copy link
Member Author

I'll check it out!

Should I apply these patches to the respective master branches or on top of the current PRs?

@sckott
Copy link
Collaborator

sckott commented Dec 19, 2019 via email

aaronwolen added a commit to aaronwolen/webmockr that referenced this pull request Dec 19, 2019
@aaronwolen
Copy link
Member Author

aaronwolen commented Dec 19, 2019

Looks like that did the trick! 🎉

Of course we're being robbed of the satisfaction of passing tests because the minimum required version of webmockr is now 0.5.1.92 but I verified everything checks out in a fresh docker container.

@sckott sckott added this to the v0.4.2 milestone Dec 19, 2019
@sckott
Copy link
Collaborator

sckott commented Dec 19, 2019

thanks for all your work on this!

sckott pushed a commit to ropensci/webmockr that referenced this pull request Dec 19, 2019
@sckott sckott merged commit e4c81a5 into ropensci:master Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants