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

Added vcr support for consistent tests #109

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ludehon
Copy link
Contributor

@ludehon ludehon commented Sep 10, 2019

Only tests functions that use HTTP will have the vcrpy decorator

@johnwmillr
Copy link
Owner

Hi @ludehon, sorry for the delay. Do you mind adding a demonstration of how to run these VCR tests?

@allerter
Copy link
Collaborator

Reading the Rationale on VCR's docs, I think this might be a good idea. Currently, the tests take about 3:20 minutes on Travis CI and even longer on local machines.

@allerter allerter mentioned this pull request Sep 29, 2020
@allerter
Copy link
Collaborator

Using VCR without the libyaml extension the tests took 105 seconds on my PC. It's also nice to be able to run the tests offline.

@johnwmillr
Copy link
Owner

Do you mind addressing these merge conflicts, @allerter?

@allerter
Copy link
Collaborator

allerter commented Nov 8, 2020

@johnwmillr, not at all. I had actually tried this out a while ago. So this won't take much time.

@allerter
Copy link
Collaborator

allerter commented Nov 8, 2020

A few notes:

  • The format of the cassettes can be changed to JSON when initiating test_vcr by changing serializer='yaml'. But for some of the tests, I explicitly set the serializer to YAML because those tests directly or indirectly call genius.lyrics or genius.tag which the response of its request is binary and JSON can't normally encode that (unless we make our own serializer for vcrpy.)
  • These cassettes only work from the second time tests are run, since at the first run vcrpy records the cassettes. So Travis CI won't be using recorded cassettes. This won't reduce test time on Travis CI but has two advantages: 1. It's best that Travis CI test all endpoints live to check if an endpoint has become unavailable. 2. Putting the recorded cassettes will increase the size of the repository, especially more since we're downloading whole HTML pages when using genius.lyrics and genius.tag.
  • Although the recorded cassettes won't be in the repo, I still added filtering the authorization headers to avoid revealing them even in local machines.

@allerter
Copy link
Collaborator

allerter commented Nov 8, 2020

  • Ready to be merged

tests/test_album.py Outdated Show resolved Hide resolved
@allerter
Copy link
Collaborator

@johnwmillr if you think using vcrpy isn't worth the trouble, you can close this pull request. It's not necessary and it's only useful when testing locally.

@johnwmillr
Copy link
Owner

You're right, @allerter. For the sake of moving forward with 3.0, let's hold off on this PR for now. I won't close it just yet, but we can move forward with the 3.0 work without merging this in.

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

Successfully merging this pull request may close these issues.

None yet

3 participants