ropensci-books / http-testing

HTTP testing for R
https://books.ropensci.org/http-testing/
Creative Commons Zero v1.0 Universal
52 stars 17 forks source link

Some comments about vcr chapter #84

Closed llrs closed 3 years ago

llrs commented 3 years ago

I'm reading the book and using http testing for the first time. Here are few comments on the sections I needed to pause and reread/explore more to understand what the book is telling me:

On the chapter about vcr at the end of section 6.1 there's a code block that I think would need more explanation what is going on.

Explain that the filter_request_headers argument is to filter headers with the token and should match where the authentication is send (not just blindly Authentication, if the API uses another header) and use the same environment variable as the one the package uses. Also it would be nice to briefly explain that first we set up a mock token or raise an error if no token is found.

Also in some sentences leading to that code block the past tense is used, I think it would make more sense to say it in present tense: "We have to tweak a bit vcr ..." as some of the following paragraphs.


Not sure if this is for vcr package or the book. But the first time I used cassette was for a function that doesn't use the API. The calls created an empty yml file on the fixtures. Maybe it is not always a good solution when there are calls to internet but not to APIs? Not sure if there are alternatives, it uses read_xml("https://host.org/file.xml").


I tested a failing API call, but instead of recording it with webmockr I simply did:

vcr::use_cassette("get_comment_fails", {
    test_that("get_comment issue fails", {
        expect_error(failing_api_call(), "Oops")
    })
})

Not sure if this should be mentioned when commenting on the vcr::use_cassette position (section 6.5).


Couldn't find a section when testing how to write to databases using an API. I don't want to open a fake issue but not sure what are the alternatives...

maelle commented 3 years ago

@llrs thanks a ton!

Regarding your comments about read_xml("https://host.org/file.xml") and the one about the databases, could you please open two issues in the vcr repo?

maelle commented 3 years ago

Regarding the failing API call, if the repo is public, could you please point me to the repo so I might see more precisely what you mean?

maelle commented 3 years ago

For the code block at the end of 6.1 I added code comments reminding the tweaks that are discussed in the text just before so that the two things (code block and text just before) are hopefully more connected. I also added something about header names in the chapter about security. Thank you!

llrs commented 3 years ago

The repo I'm implementing testing the API calls. But sincerely I'm not sure it is worth if I need to say when to run them or not. I like the skip_on_cran(), skip_if_offline(). The API is the R bug tracker which doesn't have an issue 2, so all calls to issue 2 will fail. (I am now tweaking the error messages). Will open the two issues on vcr. Close this issue whenever you wish.

llrs commented 3 years ago

So for requests that POST on the database behind the API, the only solution with vcr or webmockr is to either record and then use, or if one doesn't want to create even one entry on the API create from scratch/copy-paste from other similar requests.

About the blank cassettes it is now added as a future feature. This might also help with other functions like read.* that can read from any connection, but don't use httr or crul. (xml2::readxml uses curl and it "is unlikely to ever be supported").

maelle commented 3 years ago

Reg

The repo I'm implementing testing the API calls. But sincerely I'm not sure it is worth if I need to say when to run them or not. I like the skip_on_cran(), skip_if_offline(). The API is the R bug tracker which doesn't have an issue 2, so all calls to issue 2 will fail. (I am now tweaking the error messages). Will open the two issues on vcr. Close this issue whenever you wish.

So you made a real request that generates a real error and recorded that, correct? That's another approach but I am not sure we should advertise creating errors? It could be an item inhttps://books.ropensci.org/http-testing/errors-chapter.html though, if I followed correctly (PR welcome btw!).

Now regarding POST-ing, what you say makes perfect sense. With webfakes, with more work you'd create a webfakes app that does store data (not sure it'd make the testing better, it'd just feel more realistic maybe, it's a subjective tooling choice).

I do not understand your comment about blank cassettes. Do you mean there's an open issue in vcr?

Thanks a ton for all your feedback. :slightly_smiling_face:

llrs commented 3 years ago

Yes, I made a real request that generates a real error and recorded that in the cassette. Not sure I'll send a PR, at the moment I'm with too many things.

About POST-ing, I would consider webfakes if I could control also the backend of the API but it is not the case.

Yes, about the blank cassettes there's an open issue in vcr https://github.com/ropensci/vcr/issues/224, and hopefully it will be solved.

maelle commented 3 years ago

Thanks again!!

llrs commented 3 years ago

No problem, thanks to you for all the content you provide. I've learned a lot from you trough the years happy to give a bit more of feedback than just a like or a silent visit.