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

feature request: freeze time along with http state #266

Closed
rdpoor opened this issue Feb 20, 2013 · 22 comments · Fixed by #335
Closed

feature request: freeze time along with http state #266

rdpoor opened this issue Feb 20, 2013 · 22 comments · Fixed by #335

Comments

@rdpoor
Copy link

rdpoor commented Feb 20, 2013

Since some web sites have time dependencies, it would be useful if, upon playback, VCR could set Ruby's time and date to correspond to the time that the original cassette was recorded.

I'm imagining a construct such as:

VCR.use_cassette("captured", :freeze_time => true) { ... }

I haven't recently looked at timecop, delorean, time-travel, etc, which presumably could do something similar. But freezing time is closely related to freezing web state, so it makes sense to incorporate it into VCR.

@rdpoor
Copy link
Author

rdpoor commented Feb 20, 2013

Digging in, I see that HTTPInteraction now captures recorded_at. Is there a way to get a handle on the HTTPInteraction in a before_playback hook or (preferably) a before_http_request hook? If so, I could see how to write a helper that works with timecop to create this functionality.

@rdpoor
Copy link
Author

rdpoor commented Feb 20, 2013

A bit more on the topic: I can get the functionality I want with:

VCR.configure do |c|
  ... usual stuff ...
  c.before_playback(:with_time_frozen) do |interaction|
    Timecop.freeze(interaction.recorded_at)
  end
end

The problem then is deciding when and how to call Timecop.return. There's no after_playback hook, so my RSpec code ends up looking like this:

    it 'runs test with frozen time' do
      VCR.use_cassette("237kelvin", :tag => :with_time_frozen) do
        begin
          expect {frozen_time_test}.to_not raise_error
        ensure
          Timecop.return
        end
      end
    end

Works, but slightly messy. Unless there's a clearly better way, perhaps I'll post a StackOverflow Q&A.

@myronmarston
Copy link
Member

This sounds like a useful feature. It would need to be optional, though. Do you have an idea for what the API should be?

@rdpoor
Copy link
Author

rdpoor commented Feb 28, 2013

Maybe just add a :with-time-frozen => true option on the use_cassette method, such as:

    it 'runs test with frozen time' do
      VCR.use_cassette("237kelvin", :with_time_frozen => true) do
        expect {frozen_time_test}.to_not raise_error
      end
    end

@myronmarston
Copy link
Member

I like that or maybe freeze_time: true. That said, I'm not sure how this would work without being coupled to a particular mocking library (e.g. rspec-mocks, mocha, minitest/mock or timecop). VCR isn't coupled to any of those yet and I don't want to add a coupling just for this feature. Thoughts?

@rdpoor
Copy link
Author

rdpoor commented Feb 28, 2013

As a minor point, I like the :with_time_frozen tag only because it emphasizes the fact that time is only frozen for the scope of the block.

As for the mocking library, I'm not sure I understand your question: is timecop a 'mocking library'? Can rspec-mocks and mocha freeze time in the same way that timecop can? Regardless, you could make it a configuration parameters, such as:

VCR.configure do |c|
  c.hook_into :webmock
  c.freeze_time_with :timecop  # or :delorean or :time_travel
end

Having glanced at delorean and time_travel, I'd not complain if VCR only supported timecop -- it seems the best thought out and supported.

But having said as much, it's worth pointing out that the user doesn't need to call any timecop methods within the use_cassette block, so you're not really exposing the timecop package. In fact, VCR could implement time-freezing in just a few lines of code.

@myronmarston
Copy link
Member

As for the mocking library, I'm not sure I understand your question: is timecop a 'mocking library'?

You're right; it's more of a stubbing library.

But having said as much, it's worth pointing out that the user doesn't need to call any timecop methods within the use_cassette block, so you're not really exposing the timecop package. In fact, VCR could implement time-freezing in just a few lines of code.

It would be great if this could work without involving a stubbing library like timecop or any of the others. My concern here is that to properly freeze time, you've got to:

  • stub Time.now
  • stub Date.today
  • stub DateTime.now
  • stub Time.new (it acts like Time.now when given no arguments)
  • there may be others I'm not thinking of
  • ...and then VCR has to reset/undo these stubs when the cassette is ejected

Safely stubbing and restoring methods can get complicated (I've worked a lot in rspec-mocks and seen the complications), so I'm not sure how you'd get all this to work with "just a few lines of code". And actually, VCR is virtually monkey-patch free, and I'd like to keep it that way. What if VCR::Cassette exposed a recorded_at attribute? Then maybe you could do:

VCR.use_cassette("cassette_name") do |cassette|
  Timecop.freeze(casssette.recorded_at) do
    # do stuff
  end
end

...which isn't much more code than the a cassette option, doesn't add any additional monkey patching to, and doesn't couple VCR to anything new.

@rdpoor
Copy link
Author

rdpoor commented Feb 28, 2013

That feels exactly right, especially since cassette is already passed as an argument to the block. I like it!!

@markevans
Copy link

+1 this would be useful!
My current way of getting it working is with a spec helper method:

def vcr_with_frozen_time(cassette_name)
  VCR.use_cassette cassette_name do |cassette|
    time = if cassette.recording?
      Time.now
    else
      time = cassette.http_interactions.interactions.map(&:recorded_at).sort.first
    end
    Timecop.freeze time.to_s do
      yield
    end
  end
end

then in the test

it "does some shizzle" do
  vcr_with_frozen_time 'some/cassette' do
    # do some stuff
  end
end

@nbibler
Copy link
Collaborator

nbibler commented Jun 7, 2013

👍 for exposing the cassette.recorded_at. It would be useful for both freezing time or having local dynamic logic or warnings when cassettes are getting too old.

@myronmarston
Copy link
Member

@markevans -- you touch on one of the complexities of this (which is a big reason I haven't taken a stab at implementing it yet): how to handle the case where there is no recorded time. You probably still want to freeze time as your example shows. Would something like this be sufficient?

VCR.use_cassette("some/cassette") do |cassette|
  Timecop.freeze(cassette.recorded_at || Time.now) do
    # do stuff
  end
end

Basically, make recorded_at return nil when it has not previously been recorded, and then you can use Time.now instead.

@markevans
Copy link

@myronmarston yes, I think so. It's probably better that the user knows what they're actually doing rather than VCR do magic behind the scenes, and having recorded_at return nil when not recorded is a valid interface in itself I think.

@myronmarston
Copy link
Member

@markevans -- sounds good.

One other little thing...I'm thinking maybe the method should be called first_recorded_at since with :new_episodes you can record HTTP interactions at multiple points in time.

@nbibler
Copy link
Collaborator

nbibler commented Jun 9, 2013

Agreed. recorded_at may be ambiguous depending on the record method used. I also don't think we should necessarily hide the fact that the cassette may not have been previously recorded and therefore may not have a recorded timestamp.

It would be nice if there were a way to provide a null object rather than nil for easier debugging, but I can't think of a straightforward way to do that. I think raising a "NotRecorded" exception would be a bad idea, as it makes it more difficult to code for (rescues and whatnot).

@myronmarston
Copy link
Member

One other idea:

VCR.use_cassette("some/cassette") do |cassette|
  Timecop.freeze(cassette.first_recorded_at { Time.now }) do
    # do stuff
  end
end

Make VCR::Cassette#first_recorded_at call a provided block if it doesn't have a value. The default block used if non given could raise a NotRecorded exception. IIRC, @avdi's Exceptional Ruby shows/recommends this pattern and I tend to like it when I think of it.

@markevans
Copy link

Good point - yes first_recorded_at is clear and more meaningful.

Personally I'm not keen on the block syntax as the returned block value (e.g. Time.now) has nothing to do with the "first recorded value", so in my view shouldn't feature as part of that method call.

If raising a NotRecorded exception (which is a valid alternative to returning nil I think) then rather than using that exception as control flow, I'd favour doing

Timecop.freeze(cassette.recorded? ? cassette.recorded_at : Time.now) do

which I know is verbose, but seems more correct somehow

@nbibler
Copy link
Collaborator

nbibler commented Jun 9, 2013

I'm not against the block format. It follows with the Hash syntax for setting default values.

I could go either way on the cassette.recorded?. I feel like it might introduce tell-don't-ask issues in the future. But, it could be valuable in other circumstances to recognize whether or not you're running a recorded cassette.

@rdpoor
Copy link
Author

rdpoor commented Jun 9, 2013

Correct me if I'm wrong, but Myron could implement the block style to raise
NotRecorded, but Mark could program the explicit check for
cassette.recorded? if he prefers that style, no?

On Sun, Jun 9, 2013 at 11:09 AM, Nathaniel Bibler
notifications@github.comwrote:

I'm not against the block format. It follows with the Hash syntax for
setting default values.

I could go either way on the cassette.recorded?. I feel like it might
introduce tell-don't-ask issues in the future. But, it could be valuable in
other circumstances to recognize whether or not you're running a recorded
cassette.


Reply to this email directly or view it on GitHubhttps://github.com//issues/266#issuecomment-19170505
.

@myronmarston
Copy link
Member

Honestly, I'm on the fence about if the block makes for a better API or not. I'd definitely want the method to support a block if we made it default to raise an error if the cassette had not already been recorded, so that users can override what it should do in that case. I'm OK with just returning nil, though.

@markevans
Copy link

sorry I meant to put recording? not recorded? in the example and was using it just because the method is already there (though recorded? could be nice to have to as recording?'s negative)

although I personally prefer just returning nil because it's simpler, the block one would be fine to use if that's what people prefer. @rdpoor yes that would work

@myronmarston
Copy link
Member

OK, I've implemented this in #335. I decided to call the method VCR::Cassette#originally_recorded_at.

@markevans
Copy link

nice one thanks!

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 a pull request may close this issue.

4 participants