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

isolate tests from live APIs #77

Open
maxheld83 opened this issue Feb 4, 2021 · 3 comments
Open

isolate tests from live APIs #77

maxheld83 opened this issue Feb 4, 2021 · 3 comments

Comments

@maxheld83
Copy link
Contributor

tests will currently ocassionally invalidate when cr data changes, and some of these changes percolate through.
that is causing noise.

@maxheld83 maxheld83 added this to the Jour Fixe DFG milestone Feb 4, 2021
maxheld83 added a commit that referenced this issue Feb 4, 2021
@maxheld83 maxheld83 linked a pull request Feb 11, 2021 that will close this issue
10 tasks
@maxheld83 maxheld83 removed a link to a pull request Feb 16, 2021
10 tasks
@maxheld83 maxheld83 changed the title make tests stateless isolate tests from live APIs Mar 10, 2021
@maxheld83
Copy link
Contributor Author

I thought and read a little bit about testing against (live) APIs, also including the recent ropensci book on the topic: https://books.ropensci.org/http-testing/
Some comments on that in this PR and related issue: ropensci-books/http-testing#91

I think generally we should:

  • not test any live (or mocked) APIs in metacheck; this is a matter for the API wrappers, not for metacheck.
  • we should be able to remove most dependencies on the CR APIs in our testing
  • ... perhaps with the exception of 1-2 integration tests.
  • this should also reduce the test fixtures in size

@njahn82
Copy link
Collaborator

njahn82 commented Mar 15, 2021

To test, if a compliance check works, we would record the HTTP calls and responses just once and re-use them, right? Like https://github.com/ropensci/vcr

@maxheld83
Copy link
Contributor Author

Mmh, I am not 💯sure yet, but I don't think we should be mocking anything in metacheck (as would be done via vcr).

There's basically two sets of tests relevant here:

  1. unit testing: Those should only test the transformations, plots, etc. -- all the metacheck-specific stuff, but never anything dependent on APIs, mocked or otherwise.
    It's the API wrapper packages job to unit test their handling of the API, not metacheck's job.
    I'm doing this for biblids in https://github.com/subugoe/biblids/issues/69.
    Not sure yet how / if this is done in rcrossref, might be related to protect against missing cols from cr_works / missing fields from crossref API #183.
  2. integration tests: We should test whether metacheck works with other services, in this case, the current crossref (and doi) API.
    This should be a very sparse test (i.e. maybe happen only on main, and should be only one snapshotted test).
    Being an integration test, this should be against the live API, because that's the point.

So in summary, I don't think there's any need for mocking in metacheck.
Mocking/faking also always adds a level of complexity that needs to be justified, and I don't think it is here.

This issue should still stay open until:

  • the code (and tests) are factored out so that any possible API change are isolated to affect only one integration-tested funs
  • we have some idea of how the upstream API wrappers unit test the APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants