Travis, VCR Cassettes and unused HTTP interactions


#1

While working on a recent pull request, I discovered a number of things that I thought I should draw attention to.

The changes I was making required that I rerecord the VCR cassettes for the associated spec tests. After rerecording the cassettes and ensuring the tests passed on my local branch, I pushed my changes to the remote, only to have the tests fail when run through Travis. I found this puzzling, because my remote and local branches were in sync and the tests passed on my local branch.

After further investigation, I determined that the global setting for “:allow_unused_http_interactions” is different when run on Travis vs locally - it’s false in Travis and true when run locally. Ah! so that’s why it’s not failing when I run it locally! So, I set it to false, run it locally, and sure enough it fails. Now I know why the behavior was different, but I don’t know why it’s failing.

Now that I can duplicate the failure, I try rerecording the cassettes again. Lo and behold, It fails while trying to rerecord the cassettes! “But how can you have unused HTTP interactions when you’re recording the interactions?” you may ask. “Good question!” I’d reply. But then I realized the tests were being looped through twice.

The first time through it records the interactions, and the second time it reads from the recording. Hmmm, the second time through, the same code is making fewer requests - there must be some caching going on. Sure enough, caching in the model object was the culprit. Clearing the cache between iterations resolved the problem.

So, the moral of the story is:

  • Don’t assume Travis runs tests with the same settings as when you run tests locally.
  • Beware of caching in VCR based tests that iterate more than once.

#2

It turns out, the difference in behavior is by design. The value of :allow_unused_http_interactions will always be false in the CI environment, and true otherwise. My pr explicitly set the value for the test to false, which led to the following discussion on git:

##From: jrefanie
@roliveri I’m concerned about setting this different than our dynamic value

@brandondunne @blomquisg thoughts?

I mean, it’s annoying to have to know that you should change the
value in the spec_helper.rb if there’s a travis failure you want to
recreate locally but there’s obviously a reason @Fryguy changed it globally for the CI environment.

##From: roliveri
It should be set in a central place, but it has to be set to a
consistent value. The fact that the value differs between Travis and
local runs is more than annoying, it can result in a tremendous waste of
time.

Given the code in spec_helper.rb, this line in the pr will only effect local test runs, not Travis.

I can remove it, but I think the way it’s being set now is a big problem.

##From: jrefanie
I agree @roliveri
it’s frustrating but I’d rather we do the right thing across the board
and possibly enhance the output to make it easier to troubleshoot CI vs.
local testing with VCR.

I’ll let @brandondunne @blomquisg
comment as I don’t really have the experience with VCR to know why it’s
set one way locally and another in CI and the benefit of doing it that
way. I only point out that it’s inconsistent if some tests change the
logic. If it doesn’t make sense, it should be “false” globally and not
in a subset of tests.

##From: brandondunne
I’m the one that originally changed that. The reasoning was that in the
CI environment we want to ensure that all transactions in the VCR
cassette are used. However in a dev environment, the “you have unused
transactions” error masks any real errors and makes it difficult to
troubleshoot unless you know about this setting.

##From: roliveri
I really don’t think this is the right thing to do.

If you get “you have unused transactions” in the dev environment, it
probably means that you’ve changed the code in such a way as to require
the rerecording of the cassettes. In that case, you don’t need to know
about the setting, you just need to remove the local cassettes so
they’ll be rerecorded - then you can just continue testing. If you make a
pr without rerecording the cassettes, Travis will fail, forcing you to
rerecord them anyway.

If you want to ensure that all the transactions are used in the CI
environment, then you should ensure that that they’re all used in the
dev environment as well.

##From: brandondunne
@roliveri IIRC when you re-record the cassette and some of the parameters have changed (vm count for instance) and you have :allow_unused_http_interactions => false
it eats all errors and rather than getting something like “expected
vm.count to eq 17, got 42” you just get “you have unused transactions”.

##From: roliveri
After you get “you have unused transactions” then you re-record the
cassette. Then you’ll get “expected vm.count to eq 17, got 42”. You
shouldn’t get “you have unused transactions” after re-recording the
cassette, unless there’s a caching issue with multiple iteration tests.

###Let’s take it from here…


#3

@bdunne had a follow-on comment in the PR:

My apologies… It’s not inconsistent data in the spec, it’s when anything goes wrong in the refresher / parser. If you put a raise “Some Error” anywhere in the parser, that error will be rescued and the spec will fail with:

VCR::Errors::UnusedHTTPInteractionError:
There are unused HTTP interactions left in the cassette:

This seems to make sense. However, I can definitely see how having development and CI environments configured differently will cause problems.

So, the two situations we’re dealing with are:

  1. Code where legitimate functionality causes consecutive VCR runs to fail. In this case, caching of external requests. The first run records the VCR cassette with the complete set of data requested from the external source. The second run (in the same test cycle) bypasses several calls to the external source by relying on a cache that’s purposefully constructed in the model layer.
  2. Code where an actual bug prevents certain calls to an external source from occurring and where the test might not explicitly fail because of the bug.

In the second case, having :allow_unused_http_interactions=false will cause VCR to spit out an error indicating that some HTTP interactions were not used. There’s a possibility that this will mask the actual underlying bug. However, this should only really happen if VCR fails fast on unused http interactions (which it may).

However, I think @rpo’s point is that having the setting differ in development environments and the CI environment causes additional headaches when you might get the local tests working only to find out they fail in CI with a different setup.

My opinion is that the settings should match between dev and CI. If you set :allow_unused_http_interactions=false in development and you receive that error. There’s probably something more you have to look at. Perhaps it’s nothing, and you just have to re-record the cassettes. Or, maybe it’s something more important. Regardless, it’s an error that let’s you know something is amiss. If we have to learn to change the setting to true in order to debug further, then so be it.

However, postponing the grief of finding failing tests until you’ve made a PR seems circuitous to me.


#4

Maybe we should look into upgrading the VCR gem. I don’t see this as a configuration option in the latest version. Maybe they fail with a useful error now?


#5

Heh, nice call. https://github.com/vcr/vcr/pull/336

That PR is from about 10 months ago. v2.4.0 (according to our Gemfile.locK) was about 2 years ago.

I didn’t look at the commits closely, but one is actually titled: “Don’t allow allow_unused_http_interactions option to silence other errors.”


#6

Yeah, I had already tested locally and everything looked ok so here’s the PR: https://github.com/ManageIQ/manageiq/pull/291 We’ll see if Travis has any complaints.


#7

Ha, good. I was going to say that this sounds like a vcr bug. So, if this works out, can we unconditionally set the value of allow_unused_http_interactions to false in spec_helper.rb?


#8

Yes sir, I updated the PR to reflect that. It now raises the actual error if there is one or the unused transactions error if there are any.