-
Notifications
You must be signed in to change notification settings - Fork 35
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
Remove usage of TZ
envvar
#1192
Conversation
expect_equal(as.POSIXct(as.vector(pl_naive_time)), as.POSIXct(clock_naive_time)) | ||
expect_equal(as.POSIXct(as.vector(pl_zoned_time_1)), as.POSIXct(clock_zoned_time_1)) |
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 am really confused. What are these tests testing?
Is it guaranteed that these tests will work with other systems' time zone settings?
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.
They test that the clock
- polars
- base
conversion works.
Actually those lines could be removed since they're duplicated. The only difference was that they were wrapped in withr::with_envvar()
.
Is it guaranteed that these tests will work with other systems' time zone settings?
I don't know, but as we have seen in #1188, setting TZ
wasn't enough to check this so I don't know the path to follow here. Do you or @ju6ge have any ideas on this?
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.
The part to understand them is found here:
r-polars/tests/testthat/test-as_polars.R
Lines 362 to 378 in d26074e
patrick::with_parameters_test_that("clock package class support", | |
{ | |
skip_if_not_installed("clock") | |
naive_time_chr = c( | |
"1900-01-01T12:34:56.123456789", | |
"2012-01-01T12:34:56.123456789", | |
"2212-01-01T12:34:56.123456789" | |
) | |
clock_naive_time = clock::naive_time_parse(naive_time_chr, precision = precision) | |
clock_sys_time = clock::sys_time_parse(naive_time_chr, precision = precision) | |
clock_zoned_time_1 = clock::as_zoned_time(clock_naive_time, "America/New_York") | |
pl_naive_time = as_polars_series(clock_naive_time) | |
pl_sys_time = as_polars_series(clock_sys_time) | |
pl_zoned_time_1 = as_polars_series(clock_zoned_time_1) |
So basically a list of timestamps is first parsed via clock
package, once as naive
date without and time zone aware ness and then with time zone aware ness. Afterwards the the list is converted to a polars series.
So the test are checking that after conversion to polars series, the timestamps actually stayed the same. It is checking for naive and time_zoned timestamps.
# Note: do not set the "TZ" envvar here as it doesn't work consistently | ||
# https://github.com/pola-rs/r-polars/issues/1188 |
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.
Under what conditions does this not work? I suspect this is a function bug, not a testing issue.
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 #1188 should have proved that envvar is not reliable.
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 mean to say that there may be a bug in polars that does not respect envvar.
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 don't think envvars are universal. Unless you can reference a standard that explicitly was agreed upon by all operating systems that TZ
is to be regarded as the source of truth when checking what time zone to operate in. So this is quite irrelevant, polars
behaves correctly when explicitly told to use a specific time zone. And since these test are time_zone specific it is very sensible to explicitly state the time zone.
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.
Worth noting that Sys.timezone()
uses this envvar (and then has a complicated fallback, I had no idea it was that hard to recover the system's timezone):
function (location = TRUE)
{
if (!location)
.Deprecated(msg = "Sys.timezone(location = FALSE) is defunct and ignored")
if (!is.na(tz <- get0(".sys.timezone", baseenv(), mode = "character",
inherits = FALSE, ifnotfound = NA_character_)))
return(tz)
cacheIt <- function(tz) {
unlockBinding(".sys.timezone", baseenv())
assign(".sys.timezone", tz, baseenv())
lockBinding(".sys.timezone", baseenv())
}
tz <- Sys.getenv("TZ")
...
However I don't see any TZ
in the polars source code upstream.
expect_grepl_error( | ||
pl$lit(non_existent_time_chr)$str$strptime(pl$Datetime(time_zone = "America/New_York"), "%F %T")$to_r(), | ||
"non-existent" | ||
) | ||
expect_grepl_error( | ||
pl$lit(ambiguous_time_chr)$str$strptime(pl$Datetime(time_zone = "America/New_York"), "%F %T")$to_r(), | ||
"ambiguous" |
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.
What should be tested here is the event of an error in converting naive time to R, which has been replaced by another test that tests for errors when converting datetime with time zone to R.
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.
No, the point of both these test is to ensure that invalid timestamps are caught. The only difference to the previous test is the explicit parameter for time_zone
in the call to polars. This has nothing to do with naive time. Both parsed values are valid times without knowledge of time_zone, they are only invalid if the time zone is America/New_York
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.
No, the point of both these test is to ensure that invalid timestamps are caught. The only difference to the previous test is the explicit parameter for
time_zone
in the call to polars. This has nothing to do with naive time. Both parsed values are valid times without knowledge of time_zone, they are only invalid if the time zone isAmerica/New_York
I believe I added these tests, but you claim to know more about these tests than I?
Please check #878.
@etiennebacher In my opinion this change is premature. |
Close #1188
Thanks for the report and explanations @ju6ge, it would be great if you could check this PR