API catchup - redo

[mustberuss]

This PR aligns the package with the latest release of the Patentsview API. It also incorporates changes from mustberuss#3 by @JamesHWade to improve test performance and documentation.

Changes

Test infrastructure

• Added vcr for HTTP mocking (API keys auto-filtered from recordings)
• Separated live API bug tests (skipped by default)
• Added tests/testthat/README.md for developer guidance

TL;DR Workflows (fully below)

• R-CMD-check.yaml ropensci only- just changed to not run elsewhere (should incorporate R-CMD-orig.yaml changes)
• R-CMD-orig.yaml runs for non-ropensci pushes and PRs (using safe pull_request, not pull_request_target )
  if actor has access to API key, rerecord cassettes, run the dont_run examples and run live API bug tests
  otherwise run recorded tests without live API bug tests and dont_run examples are not run

API bug work arounds

• process-resp.R repair_resp()
  Paging broke with the November API release and was not fixed in the December release. The API may now return dissimilar pages of data, breaking the rbind(). Now we remove unrequested fields to try to help the rbind() succeed but it can still fail (see "Paging structure mismatch bug" in test-api-bugs.R and/or its reprex).
• The November API release added a :80 to the HATEOAS links. It was fixed in the December release but our code remains to remove it in case it ever returns. A test will also fail if it returns.

Vignettes

• The original vignettes have been updated to work with the new version of the API. New vignettes were also added to explain the new version of the API, including a reworking of the original blog post that announced the creation of this package (indexed as an application).
• The rendered vignettes are temporarily hosted as github pages.
• There's also a potential tech note that isn't indexed.

Documentation

• Added NEWS.md entry for 1.0.0 release with all breaking changes
• Added the runiverse badge in Readme.Rmd

[mustberuss]

I pushed changes to R-CMD-orig.yaml so it succeeds with and without API key access:

• Without API key access, the tests run with pre-recorded cassettes, no live API bug testing and dont_run examples are not run
• With API key access, the ubuntu job tests live with the API and rerecords the cassettes, does live API bug testing, runs the dont_run examples and checks in the new recordings. The other two jobs run the dont_run examples and (I think) use the new recordings when they check out the code.
• The check in does not trigger another workflow run, according to the documentation
• Uses safe pull_request, not pull_request_target

I think someone with API key access could push something to trigger fully-live testing after a PR by someone without API key access. Here are two runs in my repo, with API key access and a rerun of those jobs after deleting the API key

R-CMD-check.yaml should do the same things here unless I'm overlooking something. Hopefully that's the CI builds rework we need!

[crew102]

Thanks for the follow up. Question for you, can we arrange some kind of simple toggle for running all tests locally where we ignore the VCR recordings? I can get that kind of functionality by putting vcr::turn_off(ignore_cassettes = TRUE) in the individual test scripts but I imagine there is a better way. Here's what I'm thinking would be ideal:

• A single toggle for using VCR or not when running tests locally. This would include all tests, including the test-api-bugs.R script. Hence we wouldn't need the PATENTSVIEW_LIVE_TESTS toggle
• All tests skipped if on CRAN
• Default should be to run all tests (no VCR usage) on CI. So basically, whatever toggle is used to achieve the "running tests locally and not using VCR thing" I mentioned before, is set in the CI config file.

Thoughts?

[mustberuss]

• A single toggle for using VCR or not when running tests locally. This would include all tests, including the test-api-bugs.R script. Hence we wouldn't need the PATENTSVIEW_LIVE_TESTS toggle

We can flip the environment check so it skips those tests when an environmental variable is set. That's the way @JamesHWade submitted it but I suggested he flip it. Local testing would be faster then as it's unlikely a bug would be fixed when running the tests repeatedly during developmnet.

• All tests skipped if on CRAN

We'll have to add back skip_on_cran()s, currently the recorded tests would run on CRAN

• Default should be to run all tests (no VCR usage) on CI. So basically, whatever toggle is used to achieve the "running tests locally and not using VCR thing" I mentioned before, is set in the CI config file.

It's the presence of a recorded file that triggers a non-live test. I have the ubuntu job deleting all the recordings when the API key is available so all tests are live.
rm tests/testthat/_vcr/*.yml
The windows and mac jobs could delete them too. They'd each run the checked in recordings if the user doesn't have API key access.

The ubuntu job is currently checking in the recordings but it wouldn't have to do that if the other two jobs are running live.

[crew102]

We can flip the environment check so it skips those tests when an environmental variable is set.

Can we treat all tests, whether they are for the test-api-bugs.R file or else wise, the same? In other words, can we remove anything that distinguishes between tests that are in that file vs not? Stated more to the point, what is the purpose of having that file be tested separate than the others?

We'll have to add back skip_on_cran()s, currently the recorded tests would run on CRAN

Cool, can we do this?

It's the presence of a recorded file that triggers a non-live test

Great, so no need to change anything as it relates to testing on CI then, as the vcr files aren't checked in

[mustberuss]

Can we treat all tests, whether they are for the test-api-bugs.R file or else wise, the same? In other words, can we remove anything that distinguishes between tests that are in that file vs not? Stated more to the point, what is the purpose of having that file be tested separate than the others?

We can. I think James was trying to separate out the bug testing for speed. It's a bit backwards, the tests fail when bugs are fixed (making the use of a recorded test a bit strange). Without an environmental variable you're running live bug tests as you develop something. It's highly unlikely that an API bug would go away (like if you are running tests every 10 minutes). test-api-bugs.R isn't looking for recordings but it could so local bug testing wouldn't necessarily run live all the time. Or we could get rid of test-api-bugs.R all together.

We'll have to add back skip_on_cran()s, currently the recorded tests would run on CRAN

Cool, can we do this?

Sure, but the vcr doc explicitly mentions that it can be used to run on CRAN

Great, so no need to change anything as it relates to testing on CI then, as the vcr files aren't checked in

They are currently checked in. We'd need to delete them and add them to .gitignore if that's what you want. I was trying to have builds run with and without API key access but that won't be possible w/o checking in the recordings.

The thought was builds triggered by PRs would work with recorded tests. I think if you then push something to the PR , the jobs would run live (running as you), getting us close to the original API builds (when there wasn't an API key). In the previous PR we thought you could rerun the failed jobs but that seems to be wrong (they run as the original actor, not as you).

If this isn't what you want, the recordings would still be useful locally.

[mustberuss]

In the latest push:

• the skip_on_cran()'s are back
• workflows leverage vcr's VCR_TURN_OFF, R-CMD-check.yaml never uses recordings
• test-api-bugs.R uses recordings like the other tests do, no env opt in or out
• /tests/testthat/README.md updated accordingly
• updated links that threw errors in a local CRAN build

[crew102]

Hi Russ, sorry for the delay. I think I bungled that last review, apologies. Things looked good when I reviewed, though, certainly good enough to merge. If you're happy with things, let's merge and push to CRAN (assuming the current version of the API is stable).

[mustberuss]

Let me do one more push, I noticed I hadn't run pkgdown::build_articles() or pkgdown::build_reference().

assuming the current version of the API is stable

It is they, haven't made changes since mid-December.

It would be great to be back on CRAN!

[mustberuss]

I'm ready to merge, I just did a push to update the html files.

There is an unindexed possible tech note we could delete if you'd like. I would like to submit it when were back on CRAN. I don't think many people are using the new version of the API (based on the inactivity in the forum)

[crew102]

Great, looks good!