nealrichardson / httptest

A Test Environment for HTTP Requests in R
https://enpiar.com/r/httptest/
Other
79 stars 10 forks source link

How to implement for higher level API functions #6

Closed MarkEdmondson1234 closed 7 years ago

MarkEdmondson1234 commented 7 years ago

Hello, this looks perfect for my caching issues as you noted in your review of googleLanguageR (and others based on googleAuthR)

I can record the API requests ok (the files appear in /mock/), but when trying to use the cache it looks like it expects the function responses to be httr responses.

I tried this:

library(httptest)
library(googleLanguageR)

local_auth <- Sys.getenv("GL_AUTH") != ""
if(!local_auth){
  cat("\nNo authentication file detected - skipping integration tests\n")
} else {
  cat("\nPerforming API calls for integration tests\n")
}

.mockPaths("mock")

context("Creating record of API tests if online")

test_that("Record requests", {
  skip_if_disconnected()
  skip_if_not(local_auth)

  ## test reqs
  test_text <- "The cat sat on the mat"
  capture_requests(
    path = "mock", {
      nlp <- gl_nlp(test_text)
    })

})

with_mock_API({
  context("Unit tests - NLP")

  test_that("NLP returns expected fields", {

    test_text <- "The cat sat on the mat"
    nlp <- gl_nlp(test_text)

    expect_equal(nlp$sentences$text$content, test_text)

  })
})
#Error : POST https://language.googleapis.com/v1/documents:annotateText/ {"document":
#{"type":"PLAIN_TEXT","language":"en","content":"The cat sat on the
# mat"},"encodingType":"UTF8","features":
#{"extractSyntax":true,"extractEntities":true,"extractDocumentSentiment":true}} #(language.googleapis.com/v1/documents:annotateText-45bb10-POST.json)
#Error : is.response(x) is not TRUE

If this working as intended, should I then perhaps look at integrating with_mock_API() into googleAuthR at the time of the request?

nealrichardson commented 7 years ago

I did manage to get this test passing with the captured request fixture, but there were a few complications:

  1. For recording fixtures, keep in mind that when doing capture_requests, the destination path is relative to the current working directory of the process, which when running tests on an installed package may not be the same as your code repository. So either record the requests in an interactive session, or you may have to specify an absolute path if you want to record when running package tests.
  2. Once I got the fixture saved and in the right place, https://github.com/MarkEdmondson1234/googleAuthR/blob/master/R/googleAuthR_generator.R#L463 didn't match https://github.com/nealrichardson/httptest/blob/master/R/mock-api.R#L42 due to case sensitivity, so googleAuthR rejected the response as invalid. According to the HTTP RFC, header names are case insensitive, so arguably your check for "content-type" should match "Content-Type" too. (I generally wrap all checks for HTTP headers in tolower to match case-insensitively.)
  3. Patching that, then the next line errored because https://github.com/MarkEdmondson1234/googleAuthR/blob/884ca7b9440a9584efeb124f22f4939c44c0d987/R/googleAuthR_options.R#L16 looks for the presence of a "charset" definition, which is not strictly required. I'd suggest making that check be either startsWith or grepl and not mandating the charset. (Aside: both (2) and (3) sound like functionality that httr should provide. If it does not already, I'm happy to work with you on a PR to there that would export tools for handling HTTP headers in a RFC-compliant way.)
  4. The error message you pasted there means that with_mock_API didn't find the mock file in the location it was looking, "tests/testthat/mock/language.googleapis.com/v1/documents:annotateText-45bb10-POST.json". This isn't bubbling up and stopping execution (in a way that httptest would allow you to make assertions about the request) because https://github.com/MarkEdmondson1234/googleAuthR/blob/master/R/googleAuthR_generator.R#L223 treats client-side errors (things in R or libcurl) the same as 500 Internal Server Errors and attempts to retry. I'm not sure the circumstances under which you'd expect that a client-side error would succeed on retry, but I'm guessing you had one at some point since this code exists. If you want to preserve that behavior in googleAuthR, you might want to set options(googleAuthR.tryAttempts) to 1 or 2 for your test suite, and you might want to consider re-raising the error on https://github.com/MarkEdmondson1234/googleAuthR/blob/master/R/googleAuthR_generator.R#L268 if (is.error(f)). From a testing perspective, that would enable you to do some of the kinds of request assertions that I mentioned in the ropensci review, such as making requests of "gs://" resources without needing to record response fixtures for those requests. From a general code perspective, if you don't re-raise that error there, it will just break more opaquely downstream on code that assumes that it's receiving a httr response object (hence Error : is.response(x) is not TRUE in your output).
  5. Addressing 1-3, I got the test to pass, but then R CMD check failed because
Found the following file with a non-portable file name:
  tests/testthat/language.googleapis.com/v1/documents:annotateText-45bb10-POST.json
These are not valid file names on all R platforms.

I fixed that issue just now in https://github.com/nealrichardson/httptest/commit/76eacacb786c8fa38e29a17cf322088e121c6a94. So, in order to use httptest in your project today, you'll need to install from GitHub; I'll cut a release and get this on CRAN shortly.

Let me know if you have any other questions or obstacles and I'll do my best to help!

MarkEdmondson1234 commented 7 years ago

Amazing, thanks so much. This is going to help a lot of packages.

I had testing on Travis enabled with encrypted auth tokens but a security breach made me decide not to do that anymore, so I've been trying to build a mocking system since, if I can make the changes above and get Travis testing back it'll give me a lot more peace of mind :)

MarkEdmondson1234 commented 7 years ago

(Aside: both (2) and (3) sound like functionality that httr should provide. If it does not already, I'm happy to work with you on a PR to there that would export tools for handling HTTP headers in a RFC-compliant way.)

Yes these are both things that came from the original googlesheets auth that this code derives from so at the very least it should be checked there, or into httr.

MarkEdmondson1234 commented 7 years ago

I've implemented the changes and using the GitHub version and all tests work now :) But I still get the build check note you mentioned:

Status: 1 NOTE
checking for portable file names ... NOTE
Found the following non-portable file paths:
  googleLanguageR/tests/testthat/mock/language.googleapis.com/v1/documents-annotateText-45bb10-POST.json
  googleLanguageR/tests/testthat/mock/translation.googleapis.com/language/translate/v2-c543e7-POST.json
  googleLanguageR/tests/testthat/mock/translation.googleapis.com/language/translate/v2/detect-05d795-POST.json
  googleLanguageR/tests/testthat/mock/translation.googleapis.com/language/translate/v2/languages-bfb420.json

Tarballs are only required to store paths of up to 100 bytes and cannot
store those of more than 256 bytes, with restrictions including to 100
bytes for the final component.
nealrichardson commented 7 years ago

The message is similar but the cause is slightly different. build "notes" but check does not because of the addition of the package-named directory in the tarball pushes its paths over 100 characters:

> nchar("googleLanguageR/tests/testthat/mock/language.googleapis.com/v1/documents-annotateText-45bb10-POST.json")
[1] 102
> nchar("tests/testthat/mock/language.googleapis.com/v1/documents-annotateText-45bb10-POST.json")
[1] 86

I don't know if the build note is critical, i.e. whether CRAN would reject even though check passes. You could save 13 characters by moving them up two levels, which makes your longest path work in build:

> nchar("googleLanguageR/tests/translation.googleapis.com/language/translate/v2/detect-05d795-POST.json")
[1] 94

and I believe then you'd set .mockPaths("../"). If that's not enough (if you hit URLs that are even longer), we can explore other options.

MarkEdmondson1234 commented 7 years ago

Thanks for your help, now have working, no auth, Travis tests :) https://travis-ci.org/MarkEdmondson1234/googleLanguageR

nealrichardson commented 7 years ago

I tagged a 2.1.0 release and it is on CRAN now. Might take a day or two for it to populate to all of the mirror repositories. I added a couple of extra goodies, including a "verbose" argument to capture_requests to help debugging where the files are being written, and I incorporated some of our discussion from this issue into a FAQ so that others can learn from our troubleshooting.

MarkEdmondson1234 commented 7 years ago

Hi @nealrichardson I wondered if you could see why the tests are no failing with the #Error : is.response(x) is not TRUE again on Travis (but not failing locally with and without auth)

Log: https://travis-ci.org/MarkEdmondson1234/googleLanguageR/builds/262538207

I'm using CRAN httptest and GitHub googleAuthR with the changes which worked on Travis for this build https://travis-ci.org/MarkEdmondson1234/googleLanguageR/builds/261950263

It has me puzzled. Do I need a certain dependency again for Travis?

MarkEdmondson1234 commented 7 years ago

Nevermind, I blocked my mock folder from .Rbuildignore ....