You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/12 20:08:53 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
nealrichardson opened a new pull request #10706:
URL: https://github.com/apache/arrow/pull/10706
Also fixes some test warnings. Still to-do: address the failing (now skipped) string reverse functions added in #10589.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#discussion_r668227062
##########
File path: r/tests/testthat/test-compute-aggregate.R
##########
@@ -209,8 +209,20 @@ test_that("Edge cases", {
for (type in c(int32(), float64(), bool())) {
expect_equal(as.vector(sum(a$cast(type), na.rm = TRUE)), sum(NA, na.rm = TRUE))
expect_equal(as.vector(mean(a$cast(type), na.rm = TRUE)), mean(NA, na.rm = TRUE))
- expect_equal(as.vector(min(a$cast(type), na.rm = TRUE)), min(NA, na.rm = TRUE))
- expect_equal(as.vector(max(a$cast(type), na.rm = TRUE)), max(NA, na.rm = TRUE))
+ expect_warning(
+ expect_equal(
+ as.vector(min(a$cast(type), na.rm = TRUE)),
+ min(NA, na.rm = TRUE)
+ ),
+ "no non-missing arguments"
+ )
+ expect_warning(
+ expect_equal(
+ as.vector(max(a$cast(type), na.rm = TRUE)),
+ max(NA, na.rm = TRUE)
+ ),
+ "no non-missing arguments"
+ )
Review comment:
The warning that's being captured here is the base-R warning from `max(NA, na.rm = TRUE)` and we are not asserting that arrow will provide that warning (cause it does not as far as I can tell...)
It might be slightly clearer that our intention is that if we do:
```suggestion
expect_equal(
as.vector(min(a$cast(type), na.rm = TRUE)),
suppressWarnings(min(NA, na.rm = TRUE))
)
expect_equal(
as.vector(max(a$cast(type), na.rm = TRUE)),
suppressWarnings(max(NA, na.rm = TRUE))
)
```
Or put the `expect_warning()` inside of `expect_equal()`:
```
expect_equal(
as.vector(min(a$cast(type), na.rm = TRUE)),
expect_warning(min(NA, na.rm = TRUE), "no non-missing arguments")
)
expect_equal(
as.vector(max(a$cast(type), na.rm = TRUE)),
expect_warning(max(NA, na.rm = TRUE), "no non-missing arguments")
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#issuecomment-878555664
https://issues.apache.org/jira/browse/ARROW-12994
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#discussion_r668820854
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -70,30 +70,51 @@ verify_output <- function(...) {
testthat::verify_output(...)
}
-expect_dplyr_equal <- function(expr, # A dplyr pipeline with `input` as its start
- tbl, # A tbl/df as reference, will make RB/Table with
- skip_record_batch = NULL, # Msg, if should skip RB test
- skip_table = NULL, # Msg, if should skip Table test
+#' @param expr A dplyr pipeline with `input` as its start
+#' @param tbl A tbl/df as reference, will make RB/Table with
+#' @param skip_record_batch string skip message, if should skip RB test
+#' @param skip_table string skip message, if should skip Table test
+#' @param warning string expected warning from the RecordBatch and Table paths,
+#' passed to `expect_warning()` Default is `NA`, for no warning message.
+#' `TRUE` is a special case to mean to check for the
+#' "not supported in Arrow; pulling data into R" message.
Review comment:
```suggestion
#' @param warning string expected warning from the RecordBatch and Table paths,
#' passed to `expect_warning()`. Special values:
#' * `NA` (the default) for ensuring no warning message
#' * `TRUE` is a special case to mean to check for the
#' "not supported in Arrow; pulling data into R" message.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #10706:
URL: https://github.com/apache/arrow/pull/10706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#issuecomment-879137555
Looks like a good way of solving the problem of the wrong warnings being given, and adding the warning text in the call to the test function instead of wrapping it in `expect_warning` makes for more readable tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#discussion_r668820854
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -70,30 +70,51 @@ verify_output <- function(...) {
testthat::verify_output(...)
}
-expect_dplyr_equal <- function(expr, # A dplyr pipeline with `input` as its start
- tbl, # A tbl/df as reference, will make RB/Table with
- skip_record_batch = NULL, # Msg, if should skip RB test
- skip_table = NULL, # Msg, if should skip Table test
+#' @param expr A dplyr pipeline with `input` as its start
+#' @param tbl A tbl/df as reference, will make RB/Table with
+#' @param skip_record_batch string skip message, if should skip RB test
+#' @param skip_table string skip message, if should skip Table test
+#' @param warning string expected warning from the RecordBatch and Table paths,
+#' passed to `expect_warning()` Default is `NA`, for no warning message.
+#' `TRUE` is a special case to mean to check for the
+#' "not supported in Arrow; pulling data into R" message.
Review comment:
```suggestion
#' @param warning string expected warning from the RecordBatch and Table paths,
#' passed to `expect_warning()` special meanings:
#' * `NA` for ensuring no warning message (default)
#' * `TRUE` is a special case to mean to check for the
#' "not supported in Arrow; pulling data into R" message.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #10706: ARROW-12994: [R] Fix tests that assume UTC local tz
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10706:
URL: https://github.com/apache/arrow/pull/10706#discussion_r668227062
##########
File path: r/tests/testthat/test-compute-aggregate.R
##########
@@ -209,8 +209,20 @@ test_that("Edge cases", {
for (type in c(int32(), float64(), bool())) {
expect_equal(as.vector(sum(a$cast(type), na.rm = TRUE)), sum(NA, na.rm = TRUE))
expect_equal(as.vector(mean(a$cast(type), na.rm = TRUE)), mean(NA, na.rm = TRUE))
- expect_equal(as.vector(min(a$cast(type), na.rm = TRUE)), min(NA, na.rm = TRUE))
- expect_equal(as.vector(max(a$cast(type), na.rm = TRUE)), max(NA, na.rm = TRUE))
+ expect_warning(
+ expect_equal(
+ as.vector(min(a$cast(type), na.rm = TRUE)),
+ min(NA, na.rm = TRUE)
+ ),
+ "no non-missing arguments"
+ )
+ expect_warning(
+ expect_equal(
+ as.vector(max(a$cast(type), na.rm = TRUE)),
+ max(NA, na.rm = TRUE)
+ ),
+ "no non-missing arguments"
+ )
Review comment:
The warning that's being captured here is the base-R warning from `max(NA, na.rm = TRUE)` and we are not asserting that arrow will provide that warning (cause it does not as far as I can tell...)
It might be slightly clearer that our intention is that if we do:
```suggestion
expect_equal(
as.vector(min(a$cast(type), na.rm = TRUE)),
# Suppress the base R warning about no non-missing arguments
suppressWarnings(min(NA, na.rm = TRUE))
)
expect_equal(
as.vector(max(a$cast(type), na.rm = TRUE)),
suppressWarnings(max(NA, na.rm = TRUE))
)
```
Or put the `expect_warning()` inside of `expect_equal()`:
```
expect_equal(
as.vector(min(a$cast(type), na.rm = TRUE)),
expect_warning(min(NA, na.rm = TRUE), "no non-missing arguments")
)
expect_equal(
as.vector(max(a$cast(type), na.rm = TRUE)),
expect_warning(max(NA, na.rm = TRUE), "no non-missing arguments")
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org