Skip to content

Conversation

@olivroy
Copy link
Contributor

@olivroy olivroy commented Oct 23, 2024

  • update standalone with usethis::use_standalone("r-lib/rlang")
  • Update snapshot tests to use a single exect_snapshot(error = TRUE) for each error
  • Remove expect_error() inside snapshots as we no longer need the error class
  • replace expect_warning(code, regexp = NA) by expect_no_warning()
  • Require more recent testthat version
  • Used testthat::set_max_fails(Inf) for faster iteration during development

…or()` was used.

Put cnd_class on the first line to make it easy to remove it as needed.
Copy link
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

Thanks! A few things to tweak across the board but otherwise looks great

test_that("unnesting column of mixed vector / data frame input is an error", {
df <- tibble(x = list(1, tibble(a = 1)))
expect_snapshot((expect_error(unnest(df, x))))
expect_snapshot(unnest(df, x), error = TRUE, cnd_class = TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

I took a look at the updated snapshots. In all cases I believe we can drop cnd_class = TRUE.

Our advice lately is that if you really care about the error class, then you do

testthat::expect_error(class = )

and if you also care about the error message, you do a separate

testthat::expect_snapshot(error = )

But lately we have not been combining both features into a single snapshot

And in all cases here in tidyr I don't think we care about the error class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b49c272

@olivroy olivroy requested a review from DavisVaughan October 24, 2024 20:20
@DavisVaughan DavisVaughan merged commit 3a9a93c into tidyverse:main Oct 25, 2024
14 checks passed
@DavisVaughan
Copy link
Member

Looks awesome thanks so much!

@olivroy olivroy deleted the stand branch October 25, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

None yet

2 participants