Skip to content

Conversation

@nealrichardson
Copy link
Member

I've currently made it up through the dplyr tests, so lots more to go. Most changes are not substantive but there have been a few where the tests weren't doing exactly what we thought they were, and this exposed them.

@github-actions
Copy link

@nealrichardson nealrichardson marked this pull request as ready for review September 22, 2021 13:55
@nealrichardson
Copy link
Member Author

@github-actions crossbow submit -g r

@github-actions
Copy link

Revision: 831f39408a24e88a58e4ced34f8abe0bc2614e18

Submitted crossbow builds: ursacomputing/crossbow @ actions-854

Task Status
conda-linux-gcc-py36-cpu-r40 Azure
conda-linux-gcc-py37-cpu-r41 Azure
conda-osx-clang-py36-r40 Azure
conda-osx-clang-py37-r41 Azure
conda-win-vs2017-py36-r40 Azure
conda-win-vs2017-py37-r41 Azure
homebrew-r-autobrew Github Actions
test-r-depsource-auto Azure
test-r-depsource-system Github Actions
test-r-devdocs Github Actions
test-r-gcc-11 Github Actions
test-r-install-local Github Actions
test-r-linux-as-cran Github Actions
test-r-linux-rchk Github Actions
test-r-linux-valgrind Azure
test-r-minimal-build Azure
test-r-offline-maximal Github Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-ubuntu-gcc-release-latest Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure
test-r-ubuntu-21.04 Github Actions
test-r-version-compatibility Github Actions
test-r-versions Github Actions
test-ubuntu-18.04-r-sanitizer Azure

Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I've made two small comments. I presume the windows failures (it looks like all with timezone issues?) were there already we were ignoring (all) attributes before (it doesn't look like check.tzone gets passed through waldo)

expect_equal
}
expect_fun(as.vector(x), y, ...)
expect_as_vector <- function(x, y, ...) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor, but I wonder if we should take this opportunity to rename this expect_as_vector_equal to be super clear we are expecting them to be equal (via as.vector())

@nealrichardson
Copy link
Member Author

This looks great. I've made two small comments. I presume the windows failures (it looks like all with timezone issues?) were there already we were ignoring (all) attributes before (it doesn't look like check.tzone gets passed through waldo)

check.tzone is an argument to all.equal(), which waldo replaces. We need to instead pass ignore_attr = "tzone", which I did in the places that failed for me locally. Need to inspect the windows failures (now that they finally did not time out) to see why they need this added now.

@jonkeane
Copy link
Member

Rok ran into issues with timezones on windows when working on strftime which might be related: https://issues.apache.org/jira/browse/ARROW-13168 (and to a lesser extent, but could be related: #11105 (comment))

@nealrichardson
Copy link
Member Author

The windows failures are actually ARROW-13588, which I think we're seeing here because of a difference between waldo and base::all.equal

@jonkeane
Copy link
Member

The windows failures are actually ARROW-13588, which I think we're seeing here because of a difference between waldo and base::all.equal

😂 / 😭

@nealrichardson nealrichardson deleted the testthat-3e branch September 29, 2021 12:44
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
I've currently made it up through the dplyr tests, so lots more to go. Most changes are not substantive but there have been a few where the tests weren't doing exactly what we thought they were, and this exposed them.

Closes apache#11208 from nealrichardson/testthat-3e

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants