Skip to content
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

Avoid nested expect_error() in snapshot tests #361

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Oct 23, 2024

I guess the goal was to have the class of the error.

This PR uses the cnd_class argument from expect_snapshot() instead.

I decided to use

expect_snapshot(error = TRUE, cnd_class = TRUE, {
  code
})

to avoid having a large diff if it is decided the condition class is no longer important in certain tests.

Fix typo in env var (no effect)

Use `expect_no_warning()` instead of using regexp = NA.
@lionel-
Copy link
Member

lionel- commented Oct 24, 2024

I guess the goal was to have the class of the error.

I think it was also to see the error call at a time where testthat would not show it.

@@ -116,7 +113,7 @@ test_that("vars_pull() produces correct backtraces", {
h <- function(base) if (base) stop("foo") else abort("foo")

local_options(
rlang_trace_trop_env = current_env(),
Copy link
Member

Choose a reason for hiding this comment

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

wat 😆

@@ -79,22 +79,19 @@
Error in `f()`:
! `var` is absent but must be supplied.

# vars_pull() has informative errors
# vars_pull() are base errors
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this new title

@lionel- lionel- merged commit 7c7e309 into r-lib:main Oct 24, 2024
13 checks passed
@lionel-
Copy link
Member

lionel- commented Oct 24, 2024

Thanks!

@olivroy olivroy deleted the snapshot branch October 24, 2024 12:02
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.

2 participants