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

Use snapshot tests #538

Merged
merged 12 commits into from
Sep 19, 2024
Merged

Use snapshot tests #538

merged 12 commits into from
Sep 19, 2024

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Sep 18, 2024

This PR moves tests around errors and warnings to our current standard:

  • expect_error(regexp = NA) is now expect_no_error() (and not expect_no_condition() due to a <dplyr_regroup> condition like in expect_error(regexp = NA) -> expect_no_condition() parsnip#1188 (comment))
  • expect_error() otherwise is now expect_snapshot(error = TRUE)
  • expect_warning() is now expect_snapshot()
  • expect_snapshot_error() and expect_snapshot_warning() are now expect_snapshot() (with and without error = TRUE, respectively) to add the context to the snapshot

I didn't see any expect_message() or expect_condition().

I'll open separate issues for anything in the snapshots that needs fixing after we merge this because then I can link to the snapshot more easily.

@hfrick
Copy link
Member Author

hfrick commented Sep 18, 2024

Any suggestions to solve #539 welcome! I think I'll start with making tests self-contained because looks like it's about when purrr is loaded and I think I recall a comment about code in tests/testthat/ scripts outside of test_that() calls potentially having side-effects like this.

@hfrick hfrick requested a review from simonpcouch September 18, 2024 18:29
Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

Awesome—will defer to your judgment on which snaps deserve attention. :) Just a couple Find + Replace Wonk suggestions.

This PR helped me deal with my feelings about:

  expect_snapshot(error = TRUE, {
    int_pctl(bt_small, id)
  })

vs.

  expect_snapshot(error = TRUE, int_pctl(bt_small, id))

vs.

expect_snapshot(
  error = TRUE,
  int_pctl(bt_small, id)
)

So thank you for that😝

tests/testthat/test-bootci.R Outdated Show resolved Hide resolved
tests/testthat/test-reg_intervals.R Outdated Show resolved Hide resolved
tests/testthat/test-rolling_origin.R Outdated Show resolved Hide resolved
tests/testthat/test-rolling_origin.R Outdated Show resolved Hide resolved
tests/testthat/test-rset.R Outdated Show resolved Hide resolved
@hfrick
Copy link
Member Author

hfrick commented Sep 19, 2024

Thanks for catching the indentation bits! 🙌

@hfrick hfrick merged commit bdf12e1 into main Sep 19, 2024
12 checks passed
@hfrick hfrick deleted the use-snapshot-tests branch September 19, 2024 09:22
Copy link

github-actions bot commented Oct 4, 2024

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants