Deprecate #use_vcr_cassette #224

austenito · 2012-11-28T18:10:00Z

This commit fixes Deprecate #use_vcr_cassette RSpec macro #212
Moved macro to deprecations.rb
Added deprecated message.

austenito · 2012-11-28T18:10:53Z

Sorry, having trouble attaching this PR to an existing issue.

myronmarston · 2012-11-29T05:01:56Z

Thanks, @austenito, this looks great. One random thought...given that use_vcr_cassette is only available as an RSpec macro if folks extend the VCR::RSpec::Macros module, we can consider printing a deprecation notice when the module is extended (by defining an extended hook). It would be less noisy to get one deprecation warning for the entire module being used rather than one warning each time the use_vcr_cassette macro is used. On the other hand, maybe it's useful to get a deprecation warning each time the method is used to help track all of them down. I could go either way here. Thoughts?

I noticed that we're getting the deprecation notice in VCR's spec suite now--can you think of a way to silence it?

myronmarston · 2012-11-29T05:02:21Z

lib/vcr/test_frameworks/rspec.rb

-
-        group_descriptions.compact.reverse.join('/')
-      end
+      include VCR::Deprecations::Macros


I think maybe the module should be VCR::Deprecations::RSpecMacros so it's more clear that it's for RSpec.

austenito · 2012-11-29T18:01:08Z

Yeah the extended approach is better I think. They'll get the hint if they see the message once.

Also, I noticed the builds are failing. What is the process for failing builds? I remember seeing something about failures for different versions. In any case, I'll also work on getting them passing.

myronmarston · 2012-11-29T18:04:32Z

Also, I noticed the builds are failing. What is the process for failing builds? I remember seeing something about failures for different versions. In any case, I'll also work on getting them passing.

The build is failing due to an unreproducible failure on 1.9.3. It passes locally for me, but fails consistently on Travis. It even passed for me when I installed the travis vagrant VM and ran the build there.

It'd be great if you (or anyone) could get the build green again, but don't sweat it if you can't. I'm going to get in touch with the travis guys about.

BTW, here's a twitter conversation about it, in case that helps:

https://twitter.com/myronmarston/status/272423993775116288

austenito · 2012-12-01T09:21:06Z

@myronmarston I've updated the PR with the following changes

Created module VCR::Deprecations::RSpecMacros
Added extended hook
Spec no longer prints deprecated warning.

myronmarston · 2012-12-02T16:41:53Z

@austenito -- Much improved, thanks. A couple more thoughts:

Do you think we should move the entire VCR::RSpec::Macros module into the deprecations file? Currently there's an extra layer of indirection (the VCR::Deprecations::RSpecMacros), and it's a bit odd that the actual deprecation warning is printed from a different file, not in lib/vcr/deprecations.rb. I think moving it would simplify things, and keep all deprecation code in one place (lib/vcr/deprecations.rb).' ( BTW: I realize now that you created the RSpec::Deprecations::RSpecMacros module based on my comments before, and now it sounds like I'm suggesting something different. Sorry about that; I tend to comment on the code that currently exists (e.g. suggesting RSpec::Deprecations::RSpecMacros as a better module name than RSpec::Deprecations::Macros) while also mentioning more high-level ideas for how it might improved that may actually remove the need for one of my specific code suggestions, and I think that's what happened here.)
The travis build failed. It's been failing consistently on 1.9.3 recently (so you can ignore that) but there's a new failure showing up on 1.8.7 and ree from your changes. Do you want to take a look at those and see if you can repro them locally? BTW, I just pushed some fixes to master that I hope will get us a green build on 1.9.3 again so that the "build failed" notification is actually meaningful.
There's a yardoc deprecated tag. It'd be good to use that to mark the VCR::RSpec::Macros module and the use_vcr_cassette method as deprecated.

ajsharp · 2012-12-03T20:46:51Z

I'm confused -- is this a refactor of the use_vcr_cassette method, or does it remove the functionality altogether? I find that macro extremely useful, and it'd be a shame to see it go away. Just my 2 cents.

austenito · 2012-12-03T21:20:15Z

Do you think we should move the entire VCR::RSpec::Macros module into the deprecations file?

Yeah you're right. Having all of the deprecated code in one spot is the right move. Just to be clear, you want the deprecated code to be moved into lib/vcr/deprecations.rb. I'm not clear on what the VCR::Deprecations::RSpecMacros module is for.

The travis build failed. It's been failing consistently on 1.9.3 recently (so you can ignore that) but there's a new failure showing up on 1.8.7 and ree from your changes. Do you want to take a look at those and see if you can repro them locally?

I'll take care of that. Sorry about the failures. I didn't check since the builds were failing :( It be great if the builds didn't have false failures. Hopefully that gets resolved soon.

There's a yardoc deprecated tag.

I'll add that as well.

It might be good to have some docs on how to deprecate modules. I can write that up if you see value in it. Then again, how often do things get deprecated? Probably not all that often.

austenito · 2012-12-03T21:21:49Z

@ajsharp Just a deprecation. I'm not sure when it will go away completely. You can achieve the same effect as use_vcr_cassette by specifying RSpec metadata on your specs.

myronmarston · 2012-12-03T21:29:30Z

Yeah you're right. Having all of the deprecated code in one spot is the right move. Just to be clear, you want the deprecated code to be moved into lib/vcr/deprecations.rb. I'm not clear on what the VCR::Deprecations::RSpecMacros module is for.

You won't need that VCR::Deprecations::RSpecMacros module once you move the code. Sorry about the confusion.

I'll take care of that. Sorry about the failures. I didn't check since the builds were failing :( It be great if the builds didn't have false failures. Hopefully that gets resolved soon

It's been resolved...vcr master is green now:

https://travis-ci.org/vcr/vcr/builds/3462761

It might be good to have some docs on how to deprecate modules. I can write that up if you see value in it. Then again, how often do things get deprecated? Probably not all that often.

Yeah, I don't think it's terribly often, and I often take it on a case-by-case basis. Not sure if there's value in it yet.

myronmarston · 2012-12-03T21:32:33Z

@ajsharp -- as @austenito said, this is just a deprecation for now. As per SemVer, this'll remain in all 2.x releases. If/when a VCR 3.0 ships, I would expect it to be removed (but I have no plans yet for a 3.0).

I'm deprecating it because I think the rspec metadata integration is superior in every way. There are some gotchas related to the use_vcr_cassette macros and the fact that it uses one cassette for all examples in the example group can cause problems (particularly if the examples make different HTTP requests).

austenito · 2012-12-06T03:51:55Z

When moving VCR::RSpec::Macros to deprecations, I needed to update the module loading in lib/vcr.rb

myronmarston · 2012-12-06T07:10:01Z

@austenito -- this looks fantastic....except for the travis build failures :(. It's getting a weird error on 1.8.7:

https://travis-ci.org/vcr/vcr/jobs/3525755/#L223

Do you want to take a look at that before I merge?

austenito · 2012-12-06T08:04:36Z

Yes I'll investigate! I apologize for all the back and forth with Travis.

On Dec 5, 2012, at 9:10 PM, Myron Marston notifications@github.com wrote:

@austenito https://github.com/austenito -- this looks fantastic....except
for the travis build failures :(. It's getting a weird error on 1.8.7:

https://travis-ci.org/vcr/vcr/jobs/3525755/#L223

Do you want to take a look at that before I merge?

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/224#issuecomment-11075209.

myronmarston · 2012-12-06T15:34:40Z

No need to apologize! The VCR travis build was consistently failing for a while, and that was my fault.

- Fixes #212 - Moved macro to deprecations.rb - Move RSpec::Macros to deprecations - Add deprecated tag

Deprecate #use_vcr_cassette

myronmarston · 2012-12-13T16:05:12Z

@austenito -- Sorry about not merging until now. I wasn't notified that you had pushed more code and the build passed. In the future, you should add a comment when you push new code so I get notified.

sjtipton · 2013-05-14T20:07:25Z

A couple of questions:

Reading the above comments from @austenito, am I incorrect in assuming that the deprecation warnings should only appear once, vs. every time use_vcr_cassette?
While I can understand keeping context and describe verbiage short since VCR structures its file naming based upon this verbiage, I think problems could be encountered now that it verbiage is now part of the naming scheme. For example, let's assume I have the following:

context "when the Company Admin is creating a Job Posting", :vcr do

  it "displays a notification that the job posting was successfully created and advise the Company Admin to continue with the selected Distributions" do
    # ... my test assertions
  end

  ...

end

Now, granted, I can most likely reword something like this--but that may not always be the case. What would you suggest in situations like this in which the it is really long, and would thereby make the cassette name considerably long?

myronmarston · 2013-05-14T20:30:58Z

What would you suggest in situations like this in which the it is really long, and would thereby make the cassette name considerably long?

You can either pass a cassette name to the :vcr metadata optoin:

context "when the Company Admin is creating a Job Posting", vcr: { cassette_name: "some cassette" } do

  it "displays a notification that the job posting was successfully created and advise the Company Admin to continue with the selected Distributions" do
    # ... my test assertions
  end

  ...

end

...or wrap your code in VCR.use_cassette.

marnen · 2013-07-29T21:25:31Z

@austenito:

You can achieve the same effect as use_vcr_cassette by specifying RSpec metadata on your specs.

No, so far as I can tell, you can't. There is one use case, quite useful for me, which the new syntax doesn't cover.

Right now, I can do

context 'some context' do
  use_vcr_cassette

  it 'has an example' { 1.should == 1 }

  it 'has another example' { 2.should == 2 }
end

This will use one common cassette (some_context.yml) for both specs within that context.

By contrast, if I do

context 'some context', :vcr do
  # same specs as before
end

then two cassettes will be created (has_an_example.yml and has_another_example.yml). This is not the behavior I want.

Sure, I could achieve a common cassette with the new syntax by doing vcr: {cassette_name: 'some_context'}, but that is exactly what I don't want to do—the beauty of the RSpec metadata approach is that the cassette name is generated automatically from the description text, so I don't need to type it twice.

I don't want this feature to go away or give me deprecation warnings. Am I missing something? Is this possible with the new syntax?

myronmarston · 2013-07-29T22:45:01Z

@marnen -- while use_vcr_cassette if totally fine for your use case there, the more common thing I kept seeing users do was this:

context 'some context' do
  use_vcr_cassette

  it "makes some http requests" do
    make_some_requests
  end

  it "makes some other http requests" do
    make_some_other_requests
  end
end

VCR is best used where one cassette == one unique sequence of requests. When you mix multiple sequences of requests in the same cassette, it causes problems and VCR's not really meant to work that way. I got lots of questions from users where they were having problems related to using use_vcr_cassette in this manner. So, while I think it's completely OK to use use_vcr_cassette for your case, I didn't want to keep a construct that makes it easy to wind up with problems.

If you want to use the metadata approach, it's easy to handroll your own. VCR's integration with RSpec isn't meant to cover all cases, just the 95% common case. Here's one way you could roll your own:

shared_context "use group VCR cassette", :vcr_group_cassette do |options = {}|
  cassette_name = metadata[:example_group].container_stack.reverse.map { |c| c[:description] }.join('/')
  around(:each) { |ex| VCR.use_cassette(cassette_name, options, &ex) }
end

# use with:

describe MyClass, :vcr_group_cassette do
end

# or

describe MyClass do
  include_context "use group VCR cassette", record: :new_episodes, other: :vcr_options
end

marnen · 2013-07-29T23:42:59Z

@myronmarston:

VCR is best used where one cassette == one unique sequence of requests. When you mix multiple sequences of requests in the same cassette, it causes problems and VCR's not really meant to work that way.

I know that. I'm not using it that way. In the case I was talking about, every spec in the context is making the same HTTP requests, but different assertions are being made about them. I think this is a proper use of VCR and of use_vcr_cassette, right?

Thanks for the code sample. I hadn't really known where to start looking to implement this myself.

myronmarston · 2013-07-30T00:25:22Z

I know that. I'm not using it that way. In the case I was talking about, every spec in the context is making the same HTTP requests, but different assertions are being made about them. I think this is a proper use of VCR and of use_vcr_cassette, right?

Yep. The way you are using it is fine. That doesn't change the fact that use_vcr_cassette is problematic with the way that many people have used it. The metadata support VCR ships with now works with either style of writing specs, and I think it's best for a library like VCR to favor constructs that don't have a gotcha like "you need to structure your specs in a particular way or you might get confusing, hard-to-understand failures".

marnen · 2013-07-30T01:09:51Z

Yeah, I can understand that. I feel a lot better knowing that there's a way to implement it for those times when it is useful. Thanks.

myronmarston reviewed Nov 29, 2012
View reviewed changes

Deprecate use_vcr_cassette macro

8bbe6aa

- Fixes #212 - Moved macro to deprecations.rb - Move RSpec::Macros to deprecations - Add deprecated tag

myronmarston added a commit that referenced this pull request Dec 13, 2012

Merge pull request #224 from austenito/issue_212

633679b

Deprecate #use_vcr_cassette

myronmarston merged commit 633679b into vcr:master Dec 13, 2012

ecnalyr mentioned this pull request Dec 6, 2016

version 4.0 #578

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate #use_vcr_cassette #224

Deprecate #use_vcr_cassette #224

austenito commented Nov 28, 2012

austenito commented Nov 28, 2012

myronmarston commented Nov 29, 2012

myronmarston Nov 29, 2012

austenito commented Nov 29, 2012

myronmarston commented Nov 29, 2012

austenito commented Dec 1, 2012

myronmarston commented Dec 2, 2012

ajsharp commented Dec 3, 2012

austenito commented Dec 3, 2012

austenito commented Dec 3, 2012

myronmarston commented Dec 3, 2012

myronmarston commented Dec 3, 2012

austenito commented Dec 6, 2012

myronmarston commented Dec 6, 2012

austenito commented Dec 6, 2012

myronmarston commented Dec 6, 2012

myronmarston commented Dec 13, 2012

sjtipton commented May 14, 2013

myronmarston commented May 14, 2013

marnen commented Jul 29, 2013

myronmarston commented Jul 29, 2013

marnen commented Jul 29, 2013

myronmarston commented Jul 30, 2013

marnen commented Jul 30, 2013

Deprecate #use_vcr_cassette #224

Deprecate #use_vcr_cassette #224

Conversation

austenito commented Nov 28, 2012

austenito commented Nov 28, 2012

myronmarston commented Nov 29, 2012

myronmarston Nov 29, 2012

Choose a reason for hiding this comment

austenito commented Nov 29, 2012

myronmarston commented Nov 29, 2012

austenito commented Dec 1, 2012

myronmarston commented Dec 2, 2012

ajsharp commented Dec 3, 2012

austenito commented Dec 3, 2012

austenito commented Dec 3, 2012

myronmarston commented Dec 3, 2012

myronmarston commented Dec 3, 2012

austenito commented Dec 6, 2012

myronmarston commented Dec 6, 2012

austenito commented Dec 6, 2012

myronmarston commented Dec 6, 2012

myronmarston commented Dec 13, 2012

sjtipton commented May 14, 2013

myronmarston commented May 14, 2013

marnen commented Jul 29, 2013

myronmarston commented Jul 29, 2013

marnen commented Jul 29, 2013

myronmarston commented Jul 30, 2013

marnen commented Jul 30, 2013