ropensci / rredlist

IUCN Red List API Client

https://docs.ropensci.org/rredlist

Other

50 stars 15 forks source link

Update helper-rredlist.R #41

Closed maelle closed 4 years ago

maelle commented 4 years ago

I hope this will fail to prove my point.

maelle commented 4 years ago

The first commit failed :tada:, adding my workaround. :popcorn:

maelle commented 4 years ago

Yay!!

@sckott so in this case for recording cassettes one would need to comment out / not run the line setting the API key to foobar.

maelle commented 4 years ago

Things I need to define for a happy vcr workflow

Steps to run before recording fixtures. (vcr setup)
Steps to run after recording fixtures. (vcr teardown)
Steps to run before running tests when not recording (not-vcr setup), in this case the setting of the API key to foobar.

sckott commented 4 years ago

thanks @maelle ! This is great.

I wonder if a solution would have two parts:

look for the env var required, in this case IUCN_REDLIST_KEY, and if not found, set some default like you did IUCN_REDLIST_KEY = "foobar"
and only do the above thing if on a CI service && if cassettes already exist

thoughts?

maelle commented 4 years ago

Ah yes that sounds smarter

codecov-io commented 4 years ago

Codecov Report

Merging #41 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #41   +/-   ##
=======================================
  Coverage   65.88%   65.88%           
=======================================
  Files          22       22           
  Lines         170      170           
=======================================
  Hits          112      112           
  Misses         58       58

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4682b45...eb2e356. Read the comment docs.

maelle commented 4 years ago

I tried something but have two doubts

How do I correctly identify the fixtures folder, is the current code correct?
Why only on CI and would I detect that the code is run on CI?

maelle commented 4 years ago

I don't have an API key set locally and tested the behaviors of tests (via testthat::test_package())

with the fixtures folder, they work.
after renaming the fixtures folder, then the error message appears.

sckott commented 4 years ago

How do I correctly identify the fixtures folder, is the current code correct?

A user can name that anything they like. But it should be in the vcr_configure call in the helper file.

Why only on CI and would I detect that the code is run on CI?

I don't quite follow the question.

maelle commented 4 years ago

I was asking about the last part of https://github.com/ropensci/rredlist/pull/41#issuecomment-582581836

sckott commented 4 years ago

Yeah, maybe that doesn't make sense to only do on CI. Then if the env var is not found tests can still run without failing locally, or on CRAN, or CI

maelle commented 4 years ago

Ok so is there anything I should edit in this PR?

maelle commented 4 years ago

Should CONTRIBUTING.md be updated?

sckott commented 4 years ago

yes, sounds good