Ittest improve #425
Ittest improve #425
Conversation
PR Verification Succeeded: Coverage >= |
test/ittest/httpvcr_hooks.go
Outdated
| @@ -132,18 +146,25 @@ func DefaultValueSanitizer() ValueSanitizer { | |||
| // InteractionIndexAwareHook inject interaction index into stored header: | |||
| // httpvcr store interaction's ID but doesn't expose it to cassette.MatchFunc, | |||
There was a problem hiding this comment.
Not from this PR, but this should say MatcherFunc instead of MatchFunc
| for i := range names { | ||
| for j := range opt.Hooks { | ||
| if names[i] == opt.Hooks[j].Name() { | ||
| opt.Hooks[j] = opt.Hooks[len(opt.Hooks)-1] |
There was a problem hiding this comment.
This changed the order of the hooks, is that ok? If we sort the hooks somewhere else, is it better to sort it in this method instead?
There was a problem hiding this comment.
hooks are sorted when creating the recorder based on OrderedFirstCompare rule, see NewHttpRecorder()
| // When enforced, HTTP interactions have to happen in the recorded order. | ||
| // Otherwise, HTTP interactions can happen in any order, but each matched record can only replay once | ||
| // By default, record ordering is enabled | ||
| func HttpRecordOrdering(enforced bool) HTTPVCROptions { |
There was a problem hiding this comment.
Is this a breaking change? I see opensearch test package is changed because of this. But I think if someone was using WithHttpPlackback this is the default behaviour.
There was a problem hiding this comment.
This is a new option. We only had DisableHttpRecordOrdering before. The reason opensearchtest need to update is due to the default behaviour change of NewHttpRecorder()
test/ittest/httpvcr.go
Outdated
| // HttpRecorder wrapper of recorder.Recorder, used to hold some value that normally inaccessible via wrapped recorder.Recorder. | ||
| // Note: Internal Use Only! this type is for other test utilities to re-configure recorder.Recorder | ||
| // HttpRecorder wrapper of recorder.RawRecorder, used to hold some value that normally inaccessible via wrapped recorder.RawRecorder. | ||
| // Note: This type is for other test utilities to re-configure recorder.RawRecorder | ||
| type HttpRecorder struct { | ||
| *recorder.Recorder | ||
| RawOptions *recorder.Options | ||
| Matcher cassette.MatcherFunc |
There was a problem hiding this comment.
If this is called OrigMatcher or something like that it would make it clear that this is not the effective matcher after AdditionalMatcherOptions is applied.
There was a problem hiding this comment.
changed it to InitMatcher
| FixedHttpRecordDuration(DefaultHTTPDuration), | ||
| FixedHttpRecordDuration(0), | ||
| FixedHttpRecordDuration(DefaultHTTPDuration), | ||
| HttpTransport(http.DefaultTransport), |
There was a problem hiding this comment.
developer doesn't need to comment out this line when going from recording mode to playback mode, right?
also, I think the recorder package already sets it to this value, so this line is optional?
There was a problem hiding this comment.
developers do need to comment out HttpRecordingMode() to switch from recording mode to playback mode. The default is playback.
FixedHttpRecordDuration() is a "recording" specific option. It takes no effect in playback mode. The playback counterpart options is ApplyHttpLatency()
I added an comment on the option method to clarify that.
| } | ||
|
|
||
| // StopRecorder stops the recorder extracted from the given context. | ||
| func StopRecorder(ctx context.Context) error { |
There was a problem hiding this comment.
Probably should add a comment on these methods that they are only required to be used in tests without using the test bootstrap.
|
We should probably add a readme on what features we added on top of recorder and cassette, and how it can be used independent of the test bootstrap. |
Description
ittesthas being a very useful tool to test components that communicate with external servers via HTTP. However,ittestwas designed to use withbootstrapandapptestand cannot be used as a standalone test utility.The purpose of this PR is to improve
ittestfor standalone usage. Projects that doesn't usebootstrap,apptestoruber/fxcan still leverage utilities provided byittestpackage.Type of Change
Checklist