Introduce VCR recordModes #1
Conversation
Sources/DVR/Session.swift
Outdated
| @@ -134,7 +139,7 @@ open class Session: URLSession { | |||
| } | |||
|
|
|||
| func finishTask(_ task: URLSessionTask, interaction: Interaction, playback: Bool) { | |||
| needsPersistence = needsPersistence || !playback | |||
| needsPersistence = (needsPersistence || !playback) && recordMode != .all | |||
There was a problem hiding this comment.
So if recordMode == .all then needsPersistence would be false, but if .all is effectively our re-record, then I don't see how this works.
There was a problem hiding this comment.
.all isn't re-record. .all is live. .once is record (and re-record). It's based off when I originally asked you about VCR having 4 playback modes while our TEST_MODE environment variable only has 3.
.all = live
.once = record
.none = playback
.newEpisode isn't used since we only have 3 TEST_MODE options
There was a problem hiding this comment.
I think we should re-think our TEST_MODE options
Sources/DVR/Session.swift
Outdated
| @@ -181,7 +186,7 @@ open class Session: URLSession { | |||
| } | |||
|
|
|||
| private func addTask(_ task: URLSessionTask) { | |||
| let shouldRecord = !recording | |||
| let shouldRecord = !recording && (recordMode == .newEpisodes || recordMode == .once) | |||
There was a problem hiding this comment.
From the descriptions in Azure/azure-sdk-for-ios#906 I should think that if recordMode is .all it should also record.
| if shouldRecord { | ||
| beginRecording() | ||
| } |
There was a problem hiding this comment.
Do we want to preserve this behavior where if you don't call "beginRecording" yourself it does it automagically only for that one request? Does Python behave that way? It seems wonky to me.
Sources/DVR/SessionDataTask.swift
Outdated
| if cassette != nil { | ||
| fatalError("[DVR] Invalid request. The request was not found in the cassette.") | ||
| } | ||
|
|
||
| // Cassette is missing. Record. | ||
| if session.recordingEnabled == false { | ||
| fatalError("[DVR] Recording is disabled.") | ||
| // Cassette is missing. Record. | ||
| if session.recordingEnabled == false { | ||
| fatalError("[DVR] Recording is disabled.") | ||
| } |
There was a problem hiding this comment.
I find this pretty hard to reason about. I would suggest:
// interaction not found
if cassette !=nil {
fatalError(... request not in cassette)
} else {
// if request not found and recording is not enabled, error
if session.recordingEnabled == false {
fatalError(...)
}
}Of course, this assumes recordingEnabled is set property based on the recordMode.
Sources/DVR/SessionDataTask.swift
Outdated
| queue.async { | ||
| completion(interaction.responseData, interaction.response, nil) | ||
|
|
||
| if session.recordMode == .none { |
There was a problem hiding this comment.
I don't think this works for the general case. The only recordMode in which you don't need to check for a matching interaction is .all because that always records. This will skip looking for a matching interaction for .once and .newEpisodes.
There was a problem hiding this comment.
This whole block probably needs to be refactored in terms of the four record modes based on whether there is a cassette and whether there is a match:
- .once = YES cassette, NO match => error; NO cassette => record
- .newEpisodes = YES cassette, NO match => record; NO cassette => record
- .none = YES cassette, NO match => error, NO cassette => error
- all = always record
|
I redid the PR. DVR now conforms the playback mode expectations set in VCR. .once will fail if a recording exists but doesn't contain the current request. |
Sources/DVR/SessionDataTask.swift
Outdated
| struct DVRError : Error , CustomStringConvertible { | ||
| let description : String | ||
|
|
||
| init(_ desc : String) { | ||
| description = desc | ||
| } | ||
|
|
||
| func throwError() throws { | ||
| throw self | ||
| } | ||
| } |
There was a problem hiding this comment.
Are you planning to use this somewhere? Typically, Swift errors are enums.
These changes add a recordingMode variable that is used to enforce environment settings without obscuring DVR's logic.