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

REVDEP: How to disable breaking lifecycle_warning_deprecated warning? #213

Closed
HenrikBengtsson opened this issue Apr 21, 2022 · 7 comments · Fixed by #214
Closed

REVDEP: How to disable breaking lifecycle_warning_deprecated warning? #213

HenrikBengtsson opened this issue Apr 21, 2022 · 7 comments · Fixed by #214

Comments

@HenrikBengtsson
Copy link

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
Copy link
Owner

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
Copy link
Author

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

@DavisVaughan
Copy link
Owner

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
Copy link
Author

I see - that makes sense.

@HenrikBengtsson
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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

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 a pull request may close this issue.

2 participants