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

Allow extracting recorded requests #29

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pwalters
Copy link
Contributor

While not the main objective of MockDuck, it would be really useful as part of UI testing if MockDuck could be used to audit network activity.
This PR would allow an app to record requests with MockDuck, then extract the recordings for a host as MockRequestResponse objects. These objects can then be verified by the test suite to ensure network requests meet necessary requirements (parameters, post data, etc)

@pwalters pwalters added the enhancement New feature or request label Sep 18, 2019
@pwalters pwalters force-pushed the feature/allow-extracting-saved-requests branch from ba54a60 to c32e492 Compare September 20, 2019 16:36
@pwalters pwalters force-pushed the feature/allow-extracting-saved-requests branch from c32e492 to ccbf277 Compare September 20, 2019 19:06
///
/// - Parameter targetURL: URL that should be loaded from file
/// - Returns: MockRequestResponse if the request exists at that URL
func loadResponseFile(relativePath: String, baseURL: URL?) -> MockRequestResponse? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we could mark this private

func loadResponseFile(relativePath: String, baseURL: URL?) -> MockRequestResponse? {
guard let baseURL = baseURL else { return nil }
let targetURL = baseURL.appendingPathComponent(relativePath)
guard FileManager.default.fileExists(atPath: targetURL.path) else { return nil}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single guard here might make it clearer.

guard
    let baseURL = baseURL,
    case let targetURL = baseURL.appendingPathComponent(relativePath),
    FileManager.default.fileExists(atPath: targetURL.path)
    else { return nil }

guard
host == url.host,
scheme == url.scheme
else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: Whitespace

@@ -107,6 +107,10 @@ final class MockRequestResponse: Codable {
hashData.append(body)
}

if let bodyData = normalizedRequest.httpBodyStreamData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this break any existing recordings?

Copy link
Contributor

@abbasmousavi abbasmousavi Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the hash for the requests that have something in the .httpBodyStream and will break the recordings. We can tag and release a version for users that have some recordings already and then merge this and release a 2.0 version.

Copy link
Contributor

@abbasmousavi abbasmousavi Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better approach in my opinion is adding a delegate method that lets user compose the hash for each request based on their needs. We can keep the current hashing method as default to prevent broken recordings and we can expose a delegate so that people can compose the hash for each request based on their project needs. What do you think @scelis? If you agree with this approach we can merge this PR without the hashing change and I can open another PR to expose that delegate method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of bumping the version, what do you think about calculating multiple hashes for backwards compatibility? When loading, if the new/updated hash doesn't match anything on disk calculate the backwards-compatible hash and check for that file, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion, as I explained in another comment, is keeping the hashing code as is, so don’t breaking the previous versions, and expose an optional delegate method for people to calculate hash based on their needs because I think each project has different requirements here. For one project the headers are not important for the other one the body of the request. Some project need to remove the headers or the body from the hash to get same response on a set of request. It completely depends on the project needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a decent feature request, but that MockDuck should still provide basic hash-computation functionality that should work for the 95% use case.

@scelis scelis assigned pwalters and unassigned scelis Sep 25, 2019
@scelis scelis changed the base branch from master to main July 20, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants