download.AmerifluxLBL: respect overwrite argument
#3382
Conversation
| amf_var_info = \(...) data.frame( | ||
| Site_ID = "US-Akn", | ||
| BASE_Version = "6-5"), | ||
| .package = "amerifluxr" |
There was a problem hiding this comment.
@meetagrawal09 Can you comment in general on whether my mocking approach makes sense here, and specifically check my reasoning for going against the testthat recommendations?
The testthat docs advise not mocking objects from other namespaces because [my paraphrasings]: (1) it'll affect anything else that calls into that namespace during the life of the mock, even if from outside the code under test, and (2) the point of namespaces is to be sealed, dangit. They recommend instead to either import the functions in question or to encapsulate them in wrapper functions and then mock those.
For this case both of those options seem like a big extra layer of indirection and I don't see any risk of the mock affecting unintended code. Thoughts?
There was a problem hiding this comment.
I think that the approach you have taken by mocking these internal functions which take care of the data download is actually the right way to test the emphasis of overwrite. Honestly when I write unit tests the motive is to make them run as isolated as possible and mocking functions that deal with a third party data download is one such step towards isolation.
|
|
||
| # Case 2: overwrite existing download | ||
| dl_akn(overwrite = TRUE) | ||
| expect_gt(file.mtime(zippath), ziptime) |
There was a problem hiding this comment.
The approach you took for checking if the overwrite option is actually working seems clean. File modification time is definitely a good way to go about it.
With that said I would have done something like using mockery to mock the download function to return different data (some random mock text) for subsequent calls. Then we check if the file text was indeed updated when overwrite was set to true on the function call.
Description
Fixes #3381
@meetagrawal09 This is my first time using the "new" testthat mocking functions, so I'd appreciate your feedback on the approach. If I'm doing it all wrong, let's fix it now before I develop bad habits ;)
Motivation and Context
Review Time Estimate
Types of changes
Checklist: