Remove fix of missing how data (fix #102) - #103
Conversation
|
I added a few small changes to the test to this pull request as they mainly ensure they fail less often spuriously, and the failures are more clear |
|
@bart1 I can get to this by the end of the week. |
|
Prima ik denk dat dit ook de checks voor andere moet fixen dan dan moet die terug gemerged worden in #97 |
|
As there is no historic data available the is no need to implement a conditional check if it needs to be added |
PietrH
left a comment
PietrH
left a comment
There was a problem hiding this comment.
Made some minor suggestions for using testthat expectations and for some clarification. Super small stuff, nice changes!
You can accept or ignore, and then merge. No need to resubmit for review for me :+1
| dt_int <- lubridate::interval(time_utc, time_utc + lubridate::minutes(9)) | ||
| suppressMessages(pvols <- getRad::get_pvol(c("KABR", "czska"), dt_int)) | ||
| expect_true(is.list(pvols)) | ||
| expect_gt(length(pvols), 2) |
There was a problem hiding this comment.
Could pvols ever have a length over 2? If not, expect_length()
There was a problem hiding this comment.
Its a list of pvols in a 9 minute interval. In the US 2 pvols might be returned so that is why we use the greater then
| expect_true(all(purrr::map_lgl(pvols, ~ inherits(.x, "pvol")))) | ||
| }) | ||
|
|
||
| test_that("Caching of keys works", { |
There was a problem hiding this comment.
nitpick: I'm not the biggest fan of the system.time() based testing, there might be slowdowns on the test runner. I see why you skip this test on CRAN.
Is checking for the presence for the key in the cache not enough?
We could also use withr to set a local option to overwrite the default cache, and that way we'll be super certain both of cache location and that it starts as empty.
There was a problem hiding this comment.
I think test that is anyway run in a separate environment. I already check the files are not present now (see expect_false). I do the time based check to ensure the cache is actually used. I think this design should be pretty robust looking for a relative speed increase
small improvements Co-authored-by: Pieter Huybrechts <48065851+PietrH@users.noreply.github.com>
|
@PietrH once merges I will move this into the Sweden branch and also merge that one. I commited your suggestions. |
2 participants
No description provided.