Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

maxheld83
Copy link

closes #90

@maxheld83
Copy link
Author

btw, I love that you guys are hard-wrapping by sentence in the rmd ☺️.
Been trying to lobby the tidyverse to adopt that elsewhere: tidyverse/style#162

Copy link
Member

@maelle maelle left a 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

@maxheld83
Copy link
Author

Great + agreed!
I'll work in those suggestions @maelle -- could be a day or two.

@maelle
Copy link
Member

maelle commented Mar 11, 2021

thanks, no hurry!

intro-pkgs.Rmd Outdated Show resolved Hide resolved
intro-pkgs.Rmd Outdated Show resolved Hide resolved
intro-pkgs.Rmd Outdated Show resolved Hide resolved
@maelle
Copy link
Member

maelle commented Mar 12, 2021

I was just thinking about proper attribution of work @maxheld83

  • Please add a sentence to the beginning of the chapter you'll add, mentioning it was contributed by you.
  • Add a sentence just before
    You can also read the [PDF version](/http-testing/main.pdf) or [epub version](/http-testing/main.epub) of this book.
    (we'll now start listing contributor as in https://devguide.ropensci.org/preface.html)

@maxheld83 maxheld83 marked this pull request as draft July 8, 2021 18:14
@maxheld83
Copy link
Author

yikes, sorry I took so long to follow up:

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).

  • The discussion now lives in its own chapter, with a short pointer in the beginning.

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?

  • I've added an example of how to minimise / tradeoff mocking/faking from a real-life package (http://subugoe.github.io/biblids/).
    I've simplified the code a bit, but it works.

  • added attribution as requested.

@maxheld83 maxheld83 marked this pull request as ready for review July 11, 2021 19:31
Copy link
Member

@maelle maelle left a 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!

conclusion.Rmd Outdated Show resolved Hide resolved
@@ -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.
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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 Show resolved Hide resolved
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.
Copy link
Member

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 😜

Copy link
Author

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.
Copy link
Member

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?

Copy link
Author

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.

Copy link
Author

@maxheld83 maxheld83 Jul 12, 2021

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.

Copy link
Member

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.")
Copy link
Member

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?

Copy link
Author

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 ...

tradeoffs.Rmd Outdated Show resolved Hide resolved
tradeoffs.Rmd Outdated Show resolved Hide resolved
tradeoffs.Rmd Outdated Show resolved Hide resolved
tradeoffs.Rmd Outdated Show resolved Hide resolved
@maxheld83
Copy link
Author

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 prefer to first worry about the concepts (the purpose of TDD in this case?), and then jump into the tooling.
I don't have any formal training, and I think I've in the past often excitedly jumped onto new tools and built myself into a crazy complicated corner.

I also personally find opinionated heuristics ("strong opinions weakly held") helpful, though that's a matter of style.
(And I may just be wrong).

I wonder whether this chapter actually fits into the book, because of these (potential) differences.
This is your show after all, and a book should speak with one voice/tone, so I'd be happy to move this over to biblids or a blog, and then perhaps a short link to that can point people to it, if they're curious.

Let me know how you feel.

@maelle
Copy link
Member

maelle commented Jul 12, 2021

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? 🤔

@maelle
Copy link
Member

maelle commented Jul 12, 2021

(I'll be happy to link to the blog post in any case, as again, this is important :-) )

@maxheld83
Copy link
Author

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

That's a great idea.
I'll let this linger as a "draft" then until there's some feedback on another venue.

@maelle
Copy link
Member

maelle commented Jan 30, 2024

Closing for lack of recent activity (but please ping me if you wrote a blog post, and thanks a lot for contributing!)

@maelle maelle closed this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discuss tradeoffs for mocking
3 participants