paws-r / paws

Paws, a package for Amazon Web Services in R
https://www.paws-r-sdk.com
Other
309 stars 37 forks source link

Update to testthat 3e #708

Closed fh-mthomson closed 8 months ago

fh-mthomson commented 8 months ago

Ran usethis::use_testthat(3) (reference) and incorporated suggested tweaks to tests to reflect testthat deprecations.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (428aad6) 85.32% compared to head (53e1a5c) 85.26%.

:exclamation: Current head 53e1a5c differs from pull request most recent head 22f5e1f. Consider uploading reports for the commit 22f5e1f to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #708 +/- ## ========================================== - Coverage 85.32% 85.26% -0.06% ========================================== Files 204 136 -68 Lines 14599 9732 -4867 ========================================== - Hits 12456 8298 -4158 + Misses 2143 1434 -709 ``` [see 68 files with indirect coverage changes](https://app.codecov.io/gh/paws-r/paws/pull/708/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=paws-r)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fh-mthomson commented 8 months ago

@DyfanJones I'm scratching my head over why this fails on the CI, when it runs on Workbench and Mac with no errors:

── R CMD check results ───────────────────────────────── paws.common 0.6.3.9000 ────
Duration: 1m 19.4s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

R CMD check succeeded

Specifically, the error is consistently:

Error ('test_custom_s3.R:382:3'): redirect error without region ─────────────
Error in `get_region(cfgs[["credentials"]][["profile"]])`: No region provided

Of note, it includes all the changes in #707, so I'm wondering if there's a subtle bug in that branch such that, when using testthat 3e, with slightly more rigid standards, something isn't up to snuff?

DyfanJones commented 8 months ago

I don't think there is a bug in #707 as it passed all the tests 🤔 will have a look at this PR now

DyfanJones commented 8 months ago

I am not sure what Config/testthat/parallel: true would do. I guess if it fails again we can turn that part off to see if that is causing the weirdness 🤔

fh-mthomson commented 8 months ago

I am not sure what Config/testthat/parallel: true would do. I guess if it fails again we can turn that part off to see if that is causing the weirdness 🤔

It should only impact how devtools::test(), not devtools::check() runs (edit: I'm not 100% sure).

That said, it's possible that it's exposing some underlying impurities in how tests are run. Let's say there are two test files (A, B) run in parallel. If test A sets an env var that isn't unset, and A runs before B, then B would be indirectly affected by the env var set in A. withr::local_environment() (and other withr friends) could be a longer-term mitigation here.

If B runs before A, then B runs as expected and A has no side effects on anything

fh-mthomson commented 8 months ago

Well, that confirms it!