-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Upkeep 2024-10 #6156
Upkeep 2024-10 #6156
Conversation
`pkgdown::build_favicons(overwrite = TRUE)`
* replace `expect_error()` by `expect_snapshot(error = TRUE)` * censor machine dependent paths in snapshots * commit snapshots * skip gnarly snapshots
The following bullet:
Was only exercised for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good and non-controversial. But we should change expect_warning()
to expect_snapshot()
while we are at it
test_that("accessing an undefined variable results in an error", { | ||
skip_if(getRversion() <= "4.4.0") | ||
expect_snapshot(get_layer_data(p), error = TRUE) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended you put a test_that()
call inside another test_that()
call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that way I can conditionally skip a single expectation without throwing out all other expectations in a test. I improvised this to skip some snapshots on older R versions. This was due to error formatting that has changed across R versions.
Thanks for the review Thomas! |
This PR aims to fix #6155.
Marked as a draft for now as things are checked off.