VCR testing setup and VCR testing for ebirdchecklistfeed

[slager]

This implements CI test-coverage method number 2 from #91.

Old test coverage for ebirdchecklistfeed.R 0.00%
New test coverage for ebirdchecklistfeed.R 92.31%

See the HTTP Testing in R book and the vcr package.

With this method, no API secrets are used on CI or CRAN. CI or CRAN can use recordings of HTTP requests produced locally by the VCR package as YML files. The EBIRD_KEY api key is automatically redacted from the HTTP headers in the YML files based on the setup in helper-vcr.R. Local testing (or testing on CI using GitHub secrets) could periodically be done with real API keys to make sure the API output hasn't changed.

[sebpardo]

It took me a while to wrap my head around vcr but I think I finally understand it well enough to see it as the way forward for maintaining tests with CI. I've added you as a maintainer so you should be able to merge PRs and the likes; not sure if it's better to also have you added to team_rebird instead, @maelle?

Please go ahead and implement this across the board. One thing I'm wondering if it's worth having a single test that accesses the API using GH Secrets? It would have to be skipped for CI but should work for all commits on the main branch. Just a thought...

[maelle]

I can add @slager to the GitHub team if you prefer, @sebpardo (the GitHub team has admin access to this repo)

[slager]

@sebpardo, sure, great, I'll start working on these

[slager]

Getting started with VCR testing with this PR.

[slager]

@sebpardo Merged in upstream changes, ready for review / merge

[sebpardo]

Looks great. Can't contribute much to code specifics on this as I am a vcr noob.

[slager]

More soon!

[sebpardo]

One thing I'm wondering more generally, if you think there's any need to keep a EBIRD_KEY in a GH Secret for any reason? In other words, do you see any reason for wanting/needing to test any key-related functionality remotely in committed tests?

[slager]

Some of the VCR documentation mentions it being useful to have a test run on CI with a key, but I don't think it's necessary at all, especially if it's a pain to set up. We could keep a "skip_on_ci" and "skin_on_cran" test in the test suite that uses the key, and just periodically run that test on our local machines.

[sebpardo]

It's pretty straightforward to set up for tests in merged commits, but GH Secrets are not loaded for PRs so they would fail during any CI tests in the PR process. The issue is that getting around this and loading these secrets in the PR process has potential security hazard as anyone could submit a PR that exploits this (e.g., sending a PR that changes R-CMD-CHECK.yaml so explicitly print the API_KEY), so it would have to be a test that is somehow only run during merges/commits. Not sure if this is even feasible...

Regardless, the key that would be used as a GH Secret is linked to a rebird package specific eBird account, so it wouldn't be an issue to withdraw/cancel if required.

[slager]

Not sure why the Windows CI is failing all of a sudden.

https://github.com/ropensci/rebird/actions/runs/8166208700/job/22325144642

I suspect it could be something up (temporarily?) with the runner system, since it worked fine before and this works on my machine.

withr::local_envvar(c(EBIRD_KEY = ''), {print(Sys.getenv('EBIRD_KEY')); devtools::test(filter = 'ebirdtaxonomy')})
[1] ""
ℹ Testing rebird
✔ | F W S OK | Context
✔ | 11 | ebirdtaxonomy [13.3s]

══ Results ═
Duration: 13.5 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 11 ]

I probably won't worry about it for now.

[sebpardo]

See https://github.com/ropensci/rebird/actions/runs/8166208700/job/22325144642#step:6:119:

── Error ('test-ebirdtaxonomy.R:29:3'): ebirdtaxonomy works without an API key ──

I think it's because ebirdtaxonomy() should work without an API key and we're now creating a fake key for vcr so the test fails here: https://github.com/ropensci/rebird/blob/82771e63ff6466f10488c1d2a47962b65d8068eb/tests/testthat/test-ebirdtaxonomy.R#L28C1-L32C3

[slager]

I don't think that fully explains it, because the same stuff with VCR is working fine on the MacOS and Ubuntu runners.

Most of what I can find online suggests it's probably not really an error with rebird itself. Hopefully it goes away soon with an update to the Windows GH actions runner image.

rust-lang/cargo#12296

[maelle]

loading these secrets in the PR process has potential security hazard as anyone could submit a PR that exploits this (e.g., sending a PR that changes R-CMD-CHECK.yaml so explicitly print the API_KEY), so it would have to be a test that is somehow only run during merges/commits. Not sure if this is even feasible...

Secrets are not accessible during PRs. I can't remember from where I know this, I found https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Tests with real requests can be run on schedule, for instance once a week https://books.ropensci.org/http-testing/real-requests-chapter.html

[slager]

@sebpardo I have an idea that I think will fix some or all of the "no key variable set" errors. I'll test it in a PR tonight.