DavisVaughan/furrr

REVDEP: How to disable breaking lifecycle_warning_deprecated warning?

Closed this issue · 7 comments

HenrikBengtsson commented

Hi. When I run revdep checks on future, furrr has been giving be the following test error since quite a while:

 Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
    "Code"
    "  future_options()"

  - "Warning <lifecycle_warning_deprecated>"
  + "Condition"
  + "  Warning:"
    "  `future_options()` was deprecated in furrr 0.2.0."
    "  Please use `furrr_options()` instead."
    "Output"
  
  * Run `snapshot_accept('deprecation')` to accept the change
  * Run `snapshot_review('deprecation')` to interactively review the change
  
  [ FAIL 1 | WARN 0 | SKIP 1 | PASS 811 ]
  Error: Test failures
  Execution halted

For an example, see https://github.com/HenrikBengtsson/future/runs/6104651301?check_suite_focus=true#step:10:102.

Do you know if there is a setting (environment variable or R option) that I can set that prevents this warnings from triggering this error? Alternatively, is this something you need to fix in that check?

DavisVaughan commented

Thanks, I just need to update the snapshot tests. There was a change in testthat related to how condition information is printed out (it no longer shows the class).

https://testthat.r-lib.org/news/index.html#testthat-313

HenrikBengtsson commented

Thanks. I'm curious though, why isn't this showing up on https://cran.r-project.org/web/checks/check_results_furrr.html?

DavisVaughan commented

Because snapshot tests don't run on CRAN (by design). See the cran argument here:
https://testthat.r-lib.org/reference/expect_snapshot.html

Should these expectations be verified on CRAN? By default, they are not, because snapshot tests tend to be fragile because they often rely on minor details of dependencies.

These kinds of tests are useful for us to check the structure of warnings / errors / output. We want to be notified by CI when these kinds of tests change, but they usually aren't mission critical and are a little fragile (because they are very strict in what they test), so we don't want to get kicked off CRAN if one happens to change

HenrikBengtsson commented

I see - that makes sense.

HenrikBengtsson commented

Follow-up question: Looking at testthat:::expect_snapshot() -> testthat:::expect_snapshot_helper(), shouldn't NOT_CRAN=true be able to disable this check?

I've tried that:

https://github.com/HenrikBengtsson/future/blob/46282ae68e48643e73c964482c651c7f8e4ef7da/.github/workflows/revdepcheck-top.yaml#L43-L44

but it made no difference:

https://github.com/HenrikBengtsson/future/actions/runs/3705829844/jobs/6280255577

DavisVaughan commented

We talked about that over here
HenrikBengtsson/globals#85 (comment)

You want false not true, i.e. the very confusing logic is "is this not cran? no? oh it must be CRAN!" - and you want to mimic that

HenrikBengtsson commented

Doh. Good, I thought I was going mad making up a memory of you helping me resolve this in the past, but I couldn't find it. Got it working now. Thanks again, and thanks for being a backup to my poor memory

  • 🎉1