Record and respect request duration

[dnaeon]

Probably a better way would be to use here net/http/httptrace instead. I'm opening this up here for discussion.

Related to #62

[JVecsei]

Hey @dnaeon,

first of all, thanks for the quick reaction!
Works like a charm 👍 . I agree, you might get more accurate numbers with httptrace, on the other hand I'm not sure if there are many use cases where this is required, especially cause real request times also generally vary.

Does it make sense to have the duration feature configurable? I'm sure there are people who use this library for different reasons and might want to keep the "fast" tests without latency.

Also, this change should probably go to a new major version as this is a breaking change, especially since the parsing of existing recordings doesn't work anymore because of the empty duration strings and the changed datatype.

[dnaeon]

Hey @JVecsei ,

I've just added another commit which allows skipping the recorded request latency during replay. In order to do that simply set SkipRequestLatency to true after creating a new recorder.

The code as it is now should be compatible with older clients as well, since the recorded duration was never set before.

Can you please also test the new commit and let me know if it looks good?

Thanks!

[JVecsei]

Hi @dnaeon,

the flag works great. Thank you!
Regarding the older clients: Since the duration didn't have an omitempty in its struct tag, the existing recordings all have an empty duration string:

duration: ""

So this would fail now with this error:

simple_test.go:40: yaml: unmarshal errors:
          line 27: cannot unmarshal !!str `` into time.Duration
          line 178: cannot unmarshal !!str `` into time.Duration

So I think either it has to be changed back to manually parsing it after reading the file or having this as a feature in the next major release. What do you think?

[dnaeon]

So I think either it has to be changed back to manually parsing it after reading the file or having this as a feature in the next major release. What do you think?

You are totally correct. I'll have to finish my cup of coffee next time I reply :)

I think having this as a new major version makes sense, even if it is for this little feature. I'll make sure to tag this one as v2.0.0 before releasing.

[JVecsei]

Perfect - I think this is a great addition to the library.
Thanks again for the quick realization! 👍

[dnaeon]

Just tagged v2.0.0.

https://github.com/dnaeon/go-vcr/releases/tag/v2.0.0

Thanks!

[JVecsei]

@dnaeon In case of a new major release there are a few more things to change to make it usable: major version suffix, see also here: Releasing Modules (v2 or Higher)