Implement mocking #61
Conversation
Fixes #30
nealrichardson
left a comment
nealrichardson
left a comment
There was a problem hiding this comment.
Generally makes sense to me and I ❤️ how small the diff is
| if (!is.null(mock)) { | ||
| mock <- as_function(mock) | ||
| mock_resp <- mock(req) | ||
| if (!is.null(mock_resp)) { |
There was a problem hiding this comment.
Why check for NULL specifically? If anything I'd think stopifnot(is_response(mock_resp)) would make sense, or just let the mock function return whatever it wants.
There was a problem hiding this comment.
So the function can opt out of handling a request
There was a problem hiding this comment.
Ah I see you said that in the docs. Is there a use case you had in mind for this? It's not something I had considered; I'd think either you're mocking everything or nothing.
There was a problem hiding this comment.
I dunno, it just seems weird to completely take over all http requests with no way to opt out.
There was a problem hiding this comment.
Mocking is weird ;) I think it would be problematic if you're mocking responses but some requests silently pass through and call out to the internet, so I can't imagine a use case for this. But I guess here you'd have to explicitly design your mock function to return NULL in those cases, so you'd have control over that?
There was a problem hiding this comment.
Right. I think this design lets you opt out if you want to, but you'd have to deliberately design your function to do so.
| return(mock_resp) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if the mock should instead be run after the next few lines since they appear to modify req (though I haven't looked at what they do). I vaguely remember some having to handle some subtlety in httptest because of default values getting set somewhere deep down that weren't a part of the request object earlier. It seems that here, for example, you wouldn't be able to test anything about auth or oauth tokens being set in the request because that doesn't get added to the request object until after the mock is run.
There was a problem hiding this comment.
I don't think req_verbosity() is meaningful for mocking; it's only used for debugging live requests.
And the challenge with OAuth is that signing process might need to perform its own requests (e.g. in order to refresh a token) — you'd then have to be careful to also mock those requests, which is going to be hard because they only occur occasionally.
|
It looks good to me. I played around with it locally and it looks like it will work with webmockr and vcr (though I didn't test an actual integration with httr2). |
|
Thanks for all the feedback! |
|
BTW I'm aiming to submit the first release of httr2 to CRAN next Friday. httr2 certainly isn't done, but I think it's at a point where it makes sense for other people to try it out. |
@sckott, @nealrichardson how does this look to you?