-
Notifications
You must be signed in to change notification settings - Fork 17
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
discuss tradeoffs of mocking and faking #91
Conversation
btw, I love that you guys are hard-wrapping by sentence in the rmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!! Two remarks
-
I like the content but feel it should come later in the book are things are complicated enough at this stage. Maybe a compromise would be to have a sentence in the intro (or conclusion of the Whole Games section) in a very small section "Can one use packages for HTTP testing as little as possible?" or so and have a link to a chapter later in the book, that'd feature this current content (in advanced topics).
-
In your own packages, are there examples of code or tests that you could permalink and comment to help make things less theoretical? Or even false code? E.g. when you write "other functions", you could explain what those do e.g. data munging, what the functions interacting with the API return, to help readers visualize things a bit more?
btw, I love that you guys are hard-wrapping by sentence in the rmd relaxed.
Hehe that was my good resolution one year, thanks to Gábor Csárdi's commenting on this once in a R-hub post review. It's the last item in rOpenSci blog guidelines: https://blogguide.ropensci.org/technical.html#styleguide
Great + agreed! |
thanks, no hurry! |
I was just thinking about proper attribution of work @maxheld83
|
…s#90 also adresses feedback also explains rationale for https://github.com/subugoe/biblids/issues/69
yikes, sorry I took so long to follow up:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @maxheld83, that's a very interesting discussion to have!
My main comments are around
- moving this to Advanced Topics rather than the conclusion;
- making the reference to the chapter in the introduction less overwhelming;
- making the example more central so that the theory looks less dry;
- adding references to other chapters of the book.
Thanks a ton!
@@ -113,5 +113,8 @@ Now that you have an idea of the tools we can use for HTTP testing, we'll now cr | |||
|
|||
Our minimal package will use httr. However, it will help you understand concepts even if you end up using crul or curl.[^limits] | |||
|
|||
Test doubles such as mocks and fakes are helpful tools, but they also introduce their own complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"programs design" is a bit unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the mention of the chapter should best happen in the conclusion as it might be confusing and overwhelming in the intro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I said otherwise earlier. 🤔 Maybe the paragraph should be much shorter and not mention code design / code smell as it makes it look potentially scary right from the intro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a very real risk here to overbuild and overcomplicate the testing, while missing deeper flaws.
So I think a warning is warranted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough!
tradeoffs.Rmd
Outdated
Test doubles such as mocks or fakes can help you isolate your tests from the state of external HTTP API. | ||
This can be a good thing, because it allows you to test your code against a rare responses, or when the API is not available. | ||
|
||
Unfortunately, mocking and faking add their own complexity to a project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But fortunately this book makes it very clear how to 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but complexity still remains ... complex 🙃
tradeoffs.Rmd
Outdated
You now have two relationships to keep in mind: your code must pass your mocked/faked tests, and your mocks/fakes must correspond to the live API. | ||
|
||
This tradeoff cannot be resolved, but it is eased considerably by decoupled (or modular) code. | ||
Much as with other side effects, it can help to concentrate your API calls in as few functions as possible and test these with mocks or fakes, where necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a repetition of https://books.ropensci.org/http-testing/graceful.html#graceful-code?
Could you add some mock tests (ahah)? I.e. two functions calling an API, a version without and a version with utility functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a repetition of https://books.ropensci.org/http-testing/graceful.html#graceful-code?
Yes, sorry -- added a reference.
I think the angle here is somewhat different, drawing the parallel to side effects, which we generally want to avoid and minimise/concentrate in as few functions as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some mock tests (ahah)? I.e. two functions calling an API, a version without and a version with utility functions?
I'm not sure I fully understand.
I think the hypothetical below get_doi_url()
might be this example, where I mention that it (and other upstream functions) won't need to query the API themselves, but can rely on get_doi_handle()
instead.
I guess this is the kind of thing that would benefit from integrating the examples with all of the text as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops I meant mock code not mock tests!!
tradeoffs.Rmd
Outdated
httr::stop_for_status(resp) # covers outcomes 1 *and* 3 from above list | ||
if (httr::content(resp)$responseCode == 200) { | ||
# covers outcome 2b | ||
stop("The handle exists, but the queried value(s) could not be found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way one could imagine your function would not fail but return a NULL instead or some data.frame with some sort of explanatory column?
I can see the next function does something linked to that but it'd mean you call the API twice? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I fully understand.
Do you mean the function should return a NULL
or data.frame()
with some output if the queried value does not exist?
I haven't fully thought this through, but I like functions to fail early and to maintain type/length-stability at almost any cost, which is hard to do if you build these kind of special cases ...
But maybe I misunderstood your comment ...
and other minor edits
thanks so much for taking the time to so carefully review this again @maelle! I've fixed some of the smaller things and changed the chapter location. But I'm not sure about the overall tone and direction. I appreciate that the book is trying to be very hands-on and welcoming, but I think my concern here is/was indeed that there's a very real danger to jump into the tools before carefully thinking through the concepts. I also personally find opinionated heuristics ("strong opinions weakly held") helpful, though that's a matter of style. I wonder whether this chapter actually fits into the book, because of these (potential) differences. Let me know how you feel. |
Thank you! Maybe you could publish it as a blog post first, and then, once you've received a bit of feedback, come back to adding it as a chapter here if you so wish? 🤔 |
(I'll be happy to link to the blog post in any case, as again, this is important :-) ) |
That's a great idea. |
Closing for lack of recent activity (but please ping me if you wrote a blog post, and thanks a lot for contributing!) |
closes #90