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

Record & replay binary file response correctly #1067

Merged
merged 2 commits into from Feb 24, 2018

Conversation

gr2m
Copy link
Member

@gr2m gr2m commented Feb 20, 2018

The way nock currently implements the loading of previously recorded fixtures does not support proper binary responses, as nock cannot differentiate between a binary file encoded as hex string and a simple string. To work around that I've added a new flag to fixtures (nockDef): responseIsBinary.

I could not find a better workaround for it, but this works.

closes #1021, closes #1001, closes #524

@gr2m gr2m changed the title handle binary response WIP handle binary response Feb 20, 2018
@gr2m gr2m force-pushed the 1021/fix-binary-file-handling branch from eed00b5 to 1f077e3 Compare February 20, 2018 18:41
@gr2m gr2m changed the title WIP handle binary response WIP Record & replay binary file response correctly Feb 20, 2018
@gr2m gr2m force-pushed the 1021/fix-binary-file-handling branch from 1f077e3 to 8c7d60f Compare February 20, 2018 19:23
@gr2m
Copy link
Member Author

gr2m commented Feb 20, 2018

@n30n0v could you have a look at this? Any thoughts?

I wish I could avoid the addition of the new flag but I don’t see a way around it

@gr2m gr2m force-pushed the 1021/fix-binary-file-handling branch from a5dc32a to 08e96d2 Compare February 20, 2018 19:33
@gr2m gr2m changed the title WIP Record & replay binary file response correctly Record & replay binary file response correctly Feb 20, 2018
@gr2m
Copy link
Member Author

gr2m commented Feb 20, 2018

An alternatively could be to prefix the hex-encoded string with something like binary: and then check for that instead of the additional responseIsBinary flag. Maybe that's more elegant?

I also realised that this is only testing the response case, not the request case, I'll extend the test & implementation to cover a binary file upload, too

@gr2m gr2m changed the title Record & replay binary file response correctly WIP Record & replay binary file response correctly Feb 20, 2018
@gr2m
Copy link
Member Author

gr2m commented Feb 21, 2018

I've updated the test to also record a binary upload but that seems to work just fine without any extra handling

I'll squash the PR commits once it's good to merge

@gr2m gr2m changed the title WIP Record & replay binary file response correctly Record & replay binary file response correctly Feb 21, 2018
@gr2m gr2m force-pushed the 1021/fix-binary-file-handling branch from 6955622 to 2a91075 Compare February 21, 2018 20:25
@gr2m
Copy link
Member Author

gr2m commented Feb 21, 2018

Okay this is ready for review. @n30n0v @ierceg

In summary, nock recorder now adds a responseIsBinary flag to fixture definitions, besides scope, method, path, status, etc. It is only set to true if the response string (which is hex encoded) is a binary buffer, it is not set to false otherwise.

The only alternative I can think of is to prefix the hex string value of body with something like binary: or NOCKBINARYHEX:

@gr2m gr2m self-assigned this Feb 22, 2018
@simlu
Copy link

simlu commented Feb 24, 2018

I'm super exited about this pr! Thank you!

@gr2m
Copy link
Member Author

gr2m commented Feb 24, 2018

Thanks for the review @simlu 👍

I’ll move forward and merge that in

@gr2m gr2m merged commit 904d1b3 into master Feb 24, 2018
@gr2m gr2m deleted the 1021/fix-binary-file-handling branch February 24, 2018 18:05
@lock
Copy link

lock bot commented Sep 13, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants