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

Consider suppressing deprecation warnings in expect_warning(, NA) #1680

Closed
hadley opened this issue Sep 19, 2022 · 8 comments · Fixed by #1699
Closed

Consider suppressing deprecation warnings in expect_warning(, NA) #1680

hadley opened this issue Sep 19, 2022 · 8 comments · Fixed by #1699
Labels
expectation 🙀 feature a feature request or enhancement
Milestone

Comments

@hadley
Copy link
Member

hadley commented Sep 19, 2022

Since this is causing revdep failures

@hadley
Copy link
Member Author

hadley commented Sep 19, 2022

And in expect_silent()

@nealrichardson
Copy link

Maybe suppress only on CRAN?

@hadley hadley changed the title Consider suppressing deprecating in expect_warning(, NA) Consider suppressing deprecation warnings in expect_warning(, NA) Sep 19, 2022
@hadley
Copy link
Member Author

hadley commented Sep 19, 2022

I don't think we need to because the deprecations will still bubble up in other tests (just not the tests where you're asserting there are no warnings). But I think we'd probably couple this with #1679, where we'd start to steer people away from saying there are no warnings whatsoever in favour of asserting there are no warnings of a specific type/message.

@hadley
Copy link
Member Author

hadley commented Sep 19, 2022

Hmmmm, but there are advantages to just suppressing the warnings on CRAN: then we're guaranteeing that a deprecation will never break R CMD check. But then your local checks will return different results to CRAN, which we try to avoid as much as possible (snapshot tests not withstanding).

@nealrichardson
Copy link

But then your local checks will return different results to CRAN

To me this feels more like a variation of skip_on_cran(), where I have checks that I want to be alerted about early but don't want to cause my package to get (r)ejected, though I understand your concern.

@hadley hadley added feature a feature request or enhancement expectation 🙀 labels Sep 19, 2022
@hadley hadley added this to the 3.1.5 milestone Sep 20, 2022
@hadley
Copy link
Member Author

hadley commented Sep 21, 2022

We've determined this is actually but lifecycle's responsibility: r-lib/lifecycle#134

@hadley hadley closed this as completed Sep 21, 2022
@hadley
Copy link
Member Author

hadley commented Sep 27, 2022

We've now decided this is not lifecycle's responsibility: r-lib/lifecycle#144

@hadley hadley reopened this Sep 27, 2022
@hadley
Copy link
Member Author

hadley commented Sep 27, 2022

We've decided that it's simpler for lifecycle to be consistent, and generate deprecations regardless of where the code is run. So this means that testthat has to be a bit smarter, because we don't want a test like this:

test_that("no warnings", {
  expect_warning(foo(), NA)
}

to fail just because foo() calls a function that is now deprecated.

The deprecation warnings are no longer captured by the catch all expectation like expect_warning(code, NA) and expect_no_warning(code) ignore deprecation warnings so that they bubble up and appear in your tests. (They will still be included in snapshots.) This means that deprecation won't cause your package to break in CI or on CRAN, but when you're next working on it interactively, you'll be compelled to eliminate the warnings.

In most cases, we'll use soft deprecation in tidyverse packages which mean that users of your package won't see the warnings either. So all up, only developers who are actively working on the package will see them. This might be slightly too little pain (since if you're not actively developing your package, you might never see the deprecations), but I think we can address that problem separately, possibly by some additional tooling that lets us alert package developers via Github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expectation 🙀 feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants