Added support for the POST requests #4
Conversation
Mock full url
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 129 130 +1
=====================================
+ Hits 129 130 +1
Continue to review full report at Codecov.
|
Improvements and version bump
R/mock-api.R
Outdated
| f <- buildMockURL(req$url) | ||
| if (req$method == "GET" && file.exists(f)) { | ||
| f <- buildMockURL(req$url, req$method) | ||
| if (req$method %in% c("GET", "POST") && file.exists(f)) { |
There was a problem hiding this comment.
Why only GET and POST? Particularly given #5, I think we'll want to stop checking req$method here and L50 below and allow mocks for any verb.
There was a problem hiding this comment.
Yes, these checks have negative value for the package.
tests/testthat/test-mock-api.R
Outdated
| @@ -18,10 +18,13 @@ public({ | |||
| expect_GET(GET("api/NOTAFILE/", query=list(a=1)), | |||
| "api/NOTAFILE/?a=1") | |||
| }) | |||
| test_that("POST method reads from correct file", { | |||
| b <- POST("api/object1") | |||
| expect_identical(content(b), list(method="POST")) | |||
There was a problem hiding this comment.
This is a good start, but we should also have tests for (1) POST with body, (2) POST with query string, (3) POST with both.
There was a problem hiding this comment.
Does API with POST with query string actually exist? What will be the acceptance for this?
There was a problem hiding this comment.
I have added tests for buildMockURL. I think it easier if tests cover "units" first.
There was a problem hiding this comment.
Fair point about POST with query string being uncommon/not advised, but httr::POST will do it if you ask it to:
> POST("http://httpbin.org/post", query=list(bar="foo"), config=verbose())
-> POST /post?bar=foo HTTP/1.1
-> Host: httpbin.org
-> User-Agent: libcurl/7.51.0 r-curl/2.3 httr/1.2.1
-> Accept-Encoding: gzip, deflate
-> Accept: application/json, text/xml, application/xml, */*
-> Content-Length: 0
->
<- HTTP/1.1 200 OK
<- Server: nginx
<- Date: Wed, 15 Mar 2017 04:33:12 GMT
<- Content-Type: application/json
<- Content-Length: 415
<- Connection: keep-alive
<- Access-Control-Allow-Origin: *
<- Access-Control-Allow-Credentials: true
<-
Response [http://httpbin.org/post?bar=foo]
Date: 2017-03-15 04:33
Status: 200
Content-Type: application/json
Size: 415 B
{
"args": {
"bar": "foo"
},
"data": "",
"files": {},
"form": {},
"headers": {
"Accept": "application/json, text/xml, application/xml, */*",
"Accept-Encoding": "gzip, deflate",
...
Getting the POST body into the mock URL seems more important in any case, and that's not something that buildMockURL can take currently without adding an argument. Or, we could extend buildMockURL to take a "request" object instead of a string url (or, to maintain the current API, just check the class of the object passed in, character means URL, else "response" object).
tests/testthat/test-mock-api.R
Outdated
| test_that("Other verbs error too", { | ||
| expect_PUT(PUT("api/"), "api/") | ||
| expect_PATCH(PATCH("api/"), "api/") | ||
| expect_POST(POST("api/"), "api/") |
There was a problem hiding this comment.
Why delete this? Shouldn't it still error (and thus match expect_POST) if no mock exists?
Note that the current behavior for POST requests in with_mock_API is identical to https://github.com/nealrichardson/httptest/blob/master/tests/testthat/test-without-internet.R#L33-L35. We should preserve that behavior (the error raised by stopRequest is the URL followed by the request body, if exists), and add after that the expected mock file name. If I read the code correctly, what will now happen in this PR is that the error raised would be URL, then mock file name, then request body.
I'm guessing that this means you should factor this logic out of stopRequest so that you can extend it here.
There was a problem hiding this comment.
Sorry, misread this line
…sts to document expected behaivior.
| @@ -18,6 +18,12 @@ public({ | |||
| expect_GET(GET("api/NOTAFILE/", query=list(a=1)), | |||
| "api/NOTAFILE/?a=1") | |||
| }) | |||
| test_that("POST method reads from correct file", { | |||
| b <- POST("api/object1", body = "", content_type_json(), | |||
There was a problem hiding this comment.
This test should fail once you include the POST body in the mock file path.
There was a problem hiding this comment.
I probably would stop here for now, as it allows me to complete the outstanding tests in my packages. I think it is a different issue if anyone needs it.
tests/testthat/test-mock-api.R
Outdated
| # POST method with query in URL | ||
| file <- buildMockURL("http://www.test.com/api/call?q=1", method = "POST") | ||
| expect <- "www.test.com/api/call-a3679d-POST.json" | ||
| expect_identical(file, expect, "POST method without query string") |
There was a problem hiding this comment.
Bad copy/paste of the extra label.
| @@ -56,3 +62,29 @@ public({ | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| context("Mock URL") | |||
There was a problem hiding this comment.
Please help maintain consistency in the code by following the conventions that exist (four-space indentation, for one). And if you think these unit tests are worthy of their own "context", make a new file for them.
Added support for the POST requests
Resolve #3