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/10/25 12:31:06 UTC
[GitHub] [arrow] dragosmg opened a new pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
dragosmg opened a new pull request #11534:
URL: https://github.com/apache/arrow/pull/11534
@romainfrancois is this what you [meant](https://github.com/apache/arrow/pull/11502#discussion_r735351673)?
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961862486
How this would look like in the snapshot reviewing tool.
<img width="1304" alt="image" src="https://user-images.githubusercontent.com/13176361/140511168-a067a6a9-d810-4b7c-8d06-7a3e6c636343.png">
--
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 edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
--
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] romainfrancois commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r735734467
##########
File path: r/tests/testthat/test-dplyr-funcs-string.R
##########
@@ -120,10 +120,10 @@ test_that("paste, paste0, and str_c", {
# sep is literal NA
# errors in paste() (consistent with base::paste())
- expect_error(
- nse_funcs$paste(x, y, sep = NA_character_),
- "Invalid separator"
- )
+ expect_snapshot({
+ (expect_error(nse_funcs$paste(x, y, sep = NA_character_)))
Review comment:
I tend to have it like this:
```r
(expect_error(
nse_funcs$paste(x, y, sep = NA_character_)
))
```
so that I can more easily select the `nse_funcs$paste(x, y, sep = NA_character_)` to e.g. copy and paste when developing the tests, but that makes more lines ...
`rlang` has this defined in its helpers:
```r
err <- function(...) (expect_error(...))
```
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745521131
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
`expect_snapshot(..., error = TRUE)` fails to capture the "ancestry" of the call. See more [here](https://github.com/tidyverse/dplyr/issues/6044).
--
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 edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
I share some of Neals concerns about readability, but overall I think this pattern will help us more than it will hurt us going forward. The [improvement suggested of adding a helper](https://github.com/apache/arrow/pull/11534#discussion_r735734467) that wraps some of the extra parens, etc. will address some of that (could we add that in on this PR and see how it looks?)
We should also see what others think about this pattern and if we want to start using it more broadly (and slowly move our current tests over, but I think we can do that as we interact with them and don't need to dedicate time to moving everything over all at once).
We could 👍 / 👎 on this comment to indicate which way we want to go
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961845756
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r746398407
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_snap <- function(...) (expect_error(...))
Review comment:
added
--
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 commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-950957131
> Add a License header to the `.md` file results in failing tests. Back to _no header_ for the time being.
You can add the _snaps directory to the rat_exclude_files.txt (it's called something like that) to prevent the linter from checking them
--
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] paleolimbot commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745512223
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
My apologies if I'm missing this from a previous thread, but is there a reason we're not just using `expect_snapshot(..., error = TRUE)`? From the docs:
``` r
test_that("you can't add a number and a letter", {
expect_snapshot(1 + "a", error = TRUE)
})
```
The messageness or warningness of the output I believe is captured in the snap (since the snap contains the class of the condition).
--
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 #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-963937359
> > Maybe worth noting expect_snapshot() works best with pure R (or R-captured errors). If the error is surfaced from C++ the snapshot ends up including a bunch of information specific to the user's machine.
>
> What does this look like? Is there anyway we can construct the helper to avoid some of that? It would be unfortunate (though not at all the end of the world!) if we had to avoid these / have two patterns depending on where the error comes from
How many examples of these are there? I know it's a bit controversial but I'd be in favour of adopting the practice of always adding our own messages (as well as the C++ error output) and just testing that bit.
I think there is some objection to this on the basis of the idea that if we don't think the C++ error messages are informative enough, we should suggest updates to those, but I think that in most cases there is information which could be added to the error for the R user which doesn't make as much sense to the C++ user.
I'm thinking of things like the statements containing the word "must" and suggestions for what the user could try doing.
This might not fit every error though - but am unsure without thinking about concrete examples.
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745521131
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
`expect_snapshot(..., error = TRUE)` fails to capture the "ancestry" (the _cause_) of the call. More context [here](https://github.com/tidyverse/dplyr/issues/6044).
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-964349317
Thanks. `expect_snapshot()` has an argument called `transform` that I thought might help, but I wasn't able to get that to do what I wanted.
--
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] romainfrancois commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r746373495
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
@paleolimbot one thing that really helped me was `testthat::snapshot_review()`.
Also the `expect_snapshot(` + `(expect_error()` pattern has the benefit of explicitly expecting an error from a specific expression, as opposed to merely allow errors in one of the expressions in `expect_snapshot(error = TRUE)`, and so have the test fail otherwise.
--
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] romainfrancois commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r746373495
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
@paleolimbot one thing that really helped me was `testthat::snapshot_review()`.
Also the `(expect_snapshot(` + `expect_error()` pattern has the benefit of explicitly expecting an error from a specific expression, as opposed to merely allow errors in one of the expressions in `expect_snapshot(error = TRUE)`, and so have the test fail otherwise.
--
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 #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745152656
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
Yeah, `err_snap()` and `warn_snap()` are nice to me, I would be curious if @thisisnic @romainfrancois or @paleolimbot have any thoughts about these helper names while we still only have a small number to find/replace.
--
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 edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
I share some of Neals concerns about readability, but overall I think this pattern will help us more than it will hurt us going forward. The [improvement suggested of adding a helper](https://github.com/apache/arrow/pull/11534#discussion_r735734467) that wraps some of the extra parens, etc. will address some of that (could we add that in on this PR and see how it looks?)
We should also see what others think about this pattern and if we want to start using it more broadly (and slowly move our current tests over, but I think we can do that as we interact with them and don't need to dedicate time to moving everything over all at once).
We could 👍 (I support changing to snapshots) / 👎 (I don't support changing to snapshots) on this comment to indicate which way we want to go
--
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 pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-962067428
That helper looks good, could you implement it in this PR (one thing I would like to see with it is what the snapshots looks like with the helper)
--
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] jimhester commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jimhester commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-965458681
Yeah I think extending `cpp11::stop()` to throw classed errors is a good future plan.
For the more immediate issue if you are trying to remove the machine specific parts of the callback you can use the `transform=` argument to `expect_snapshot()` to strip them out, assuming you can detect and filter them in some way, which I think shouldn't be too hard.
--
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] paleolimbot commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745529945
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
My vote (with no string feelings) is still for `expect_error()` so that newbies to snapshot testing like me understand what's going on when we have to debug one of them or write a new one.
--
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 a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745814775
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_snap <- function(...) (expect_error(...))
Review comment:
Please could you add a comment about why these functions are needed?
--
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] romainfrancois commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-964978449
It's tricky because when `ARROW_EXTRA_ERROR_CONTEXT` is defined, these lines are automatically added by each `RETURN_NOT_OK` and there's no runtime opt-out.
https://github.com/apache/arrow/blob/1ddd644210888a0996754d5f5d517699dcf6c446/cpp/src/arrow/status.h#L28
The extra context info is merely added to the status message: https://github.com/apache/arrow/blob/1ddd644210888a0996754d5f5d517699dcf6c446/cpp/src/arrow/status.cc#L135
I can offer this to remove the extra context information, this might be enough here:
```r
err_remove_context <- function(cnd) {
msg <- strsplit(cnd$message, "\n")[[1]]
msg <- msg[!grepl("^.*:\\d+ ", msg)]
cnd$message <- paste(msg, collapse = "\n")
rlang::cnd_signal(cnd)
}
err_snap <- function(expr){
(expect_error(
withCallingHandlers(expr, simpleError = err_remove_context)
))
}
```
But it feels fragile. Perhaps one thing that we could do to improve on this would be to improve `StopIfNotOk()` so that it would identify the error as something that originated from an arrow status:
```cpp
static inline void StopIfNotOk(const Status& status) {
if (!status.ok()) {
// ARROW-13039: be careful not to interpret our error message as a %-format string
std::string s = status.ToString();
cpp11::stop("%s", s.c_str());
}
}
```
at the moment, it just calls `cpp11::stop()`
https://github.com/r-lib/cpp11/blob/d89706a83324a57c2ff25fb46bbe9cc3770caee0/inst/include/cpp11/protect.hpp#L254
which in turns calls `Rf_errorcall()`. This does not give a mechanism to add a class to the error 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] romainfrancois commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-951933363
You would then locally use `testthat::snapshot_preview()` to see what has changed, and/or the diff view in the pull request when you propose changes to error messages, etc ...
It does take some getting used to, but at least in my experience of embracing this in `dplyr`, it really helped curate the error messages.
--
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 #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-950876908
--
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 pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
I share some of Neals concerns about readability, but overall I think this pattern will help us more than it will hurt us going forward. The [improvement suggested of adding a helper](https://github.com/apache/arrow/pull/11534#discussion_r735734467) that wraps some of the extra parens, etc. will address some of that (could we add that in on this PR and see how it looks?)
We should also see what others think about this pattern and if we want to start using it more broadly (and slowly move our current tests over, but I think we can do that as we interact with them and don't need to dedicate time to moving everything over all at once).
We could ➕ / ➖ on this comment to indicate which way we want to go
--
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 #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745149729
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
I like the helpers and that they reduce some of the boiler plate. Any reason not to make them simply `err()` and `warn()` or maybe `err_snap()` and `warn_snap()` to make it clear that they are for use with snapshots?
If the reason you were thinking of avoiding them is to avoid running into testthat's version if they implement them, it [looks like they aren't planning to](https://github.com/r-lib/testthat/issues/1471#issuecomment-962982383)
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745151267
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
I have no problem with `err()`, but there already is a `warn()` function in {rlang} so I was a bit concerned with that. I like `err_snap()` and `warn_snap()` though, so it looks like that would be the way to go.
--
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 pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-963643310
> Maybe worth noting expect_snapshot() works best with pure R (or R-captured errors). If the error is surfaced from C++ the snapshot ends up including a bunch of information specific to the user's machine.
What does this look like? Is there anyway we can construct the helper to avoid some of that? It would be unfortunate (though not at all the end of the world!) if we had to avoid these / have two patterns depending on where the error comes from
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-964014256
This is what the C++ surfaced error looks like after switching to `expect_snapshot()` for the test on row 835.
test:
<img width="995" alt="image" src="https://user-images.githubusercontent.com/13176361/140906815-12b84035-b4f3-43e3-a70e-4133566016d7.png">
snapshot:
<img width="997" alt="image" src="https://user-images.githubusercontent.com/13176361/140906862-babd2b3c-fd89-44c2-8cb9-4e4e48e742ea.png">
As far as I can tell the snapshot includes some sort of C++ traceback but with paths referencing stuff on my machine. We were making use of the `regexp` argument of `expect_error()` for partial matching of error messages in the original test:
```
expect_error(
times %>%
Table$create() %>%
mutate(x = strftime(datetime, format = "%c")) %>%
collect(),
"%c flag is not supported in non-C locales."
)
```
--
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 #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745150642
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
Aaah I didn't see this earlier: https://github.com/apache/arrow/pull/11534#issuecomment-962983868 yeah, avoiding the rlang conflict is probably a good one, how about `_snap()` so it's clear where to use them? `_helper()` is a _little bit_ too generic to me
--
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] paleolimbot commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745442398
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
I'm with Nic... I'd like to know when I'm reading the code that I have to invoke the snapshot workflow!
--
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 pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961860067
This is how the same chunk of code would look like with a helper function:
```
expect_snapshot({
err(
nse_funcs$str_to_lower("Apache Arrow", locale = "sp")
)
})
```
without the helper function:
```
expect_snapshot({
(expect_error(
nse_funcs$str_to_lower("Apache Arrow", locale = "sp")
))
})
```
where `err` is `err <- function(...) (expect_error(...))`. imho it makes the code a bit easier on the eyes. let me know what you think.
--
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] romainfrancois commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-962980817
I believe it looks better with the helper. Wondering if testthat can help, I raised the issue here: https://github.com/r-lib/testthat/issues/1471
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-950938758
Add a License header to the `.md` file results in failing tests. Back to _no header_ for the time being.
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-962983868
I'm not sure what the best name for the helper(s) would be (as I think we might need more than one). Initially I went with `err()` for the `expect_snapshot()` + `(expect_error())` pattern, but I think a similar approach is useful for `expect_warning()` too. Naming the helper `warn()` would conflict with {rlang} so I went with `err_helper()` and `warn_helper()` as names.
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-963010954
Maybe worth noting `expect_snapshot()` works best with pure R (or R-captured errors). If the error is surfaced from C++ the snapshot ends up including a bunch of information specific to the user's machine.
--
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] dragosmg edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961862486
--
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 edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961271239
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745461259
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
In this specific instance - a wrapper that is very close to the actual function and does not add much in terms of complexity + is not user facing - I'd be in favour of a shorter name (like the initial `err()`). In order to prevent conflicts with {rlang} I think adding the __snap_ prefix should do. So my vote goes for `err_snap()` and `warn_snap()` and maybe, if needed a `mess_snap()`.
--
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] romainfrancois commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-964347496
🤔 maybe `err_snap()` can remove the C++ backtrace information. I'll have a look in the morning.
--
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 commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-951920606
How exactly is this better? To my eyes, there are a bunch of extra parentheses everywhere, and the expected behavior (error message) has been moved so that it's no longer next to the test code, which makes it harder to read the tests to see what's happening.
--
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] dragosmg commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961845756
Sure. I'll add a helper function.
--
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] dragosmg edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-961862486
How the use of the helper function would look like in the snapshot reviewing tool.
<img width="1304" alt="image" src="https://user-images.githubusercontent.com/13176361/140511168-a067a6a9-d810-4b7c-8d06-7a3e6c636343.png">
--
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] dragosmg commented on a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745151267
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
I have no problem with `err()`, but there already is a `warn()` function in {rlang} so I was a bit concerned with a potential conflict. I like `err_snap()` and `warn_snap()` though, so it looks like that would be the way to go.
--
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 a change in pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#discussion_r745398724
##########
File path: r/tests/testthat/helper-expectation.R
##########
@@ -317,3 +317,7 @@ split_vector_as_list <- function(vec) {
vec2 <- vec[seq(from = min(length(vec), vec_split + 1), to = length(vec), by = 1)]
list(vec1, vec2)
}
+
+err_helper <- function(...) (expect_error(...))
Review comment:
More verbose, but how about `expect_error_snapshot()` and `expect_warning_snapshot()`? Or without the `expect` if that makes more sense (I haven't read this bit of code in-depth yet). I'm in favour of leaving full words as-is if it's not causing conflicts or overly long names, as it makes easier to guess what's happening.
--
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 edited a comment on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-962067428
That helper looks good, could you implement it in this PR? (one thing I would like to see with it is what the snapshots looks like with the helper)
--
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] romainfrancois commented on pull request #11534: ARROW-14458 [R] Use expect_snapshot() to improve tests in dplyr funcs strings
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #11534:
URL: https://github.com/apache/arrow/pull/11534#issuecomment-964995310
One thing we could do maybe is to add a `call` to the error, this would still be a `simpleError` but at least we could identify that it is from a status, and then only attempt to simplify in that case, e.g.
```cpp
static inline void StopIfNotOk(const Status& status) {
if (!status.ok()) {
SEXP call = Rf_lang1(Rf_install("StopIfNotOk"));
cpp11::safe.noreturn(Rf_errorcall)(call, "%s", status.ToString().c_str());
}
}
```
```r
err_remove_context <- function(cnd) {
if (identical(cnd$call, call("StopIfNotOk"))) {
msg <- strsplit(cnd$message, "\n")[[1]]
msg <- msg[!grepl("^.*:\\d+ ", msg)]
cnd$message <- paste(msg, collapse = "\n")
rlang::cnd_signal(cnd)
}
}
```
but perhaps there are plans in `cpp11` to be able to throw classed exceptions ? cc @jimhester
--
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