Skip to content

test: add failing test for JSON-encoded strings stored by recorder#1231

Closed
ctstone wants to merge 1 commit intonock:masterfrom
ctstone:bug/recorder-json-string
Closed

test: add failing test for JSON-encoded strings stored by recorder#1231
ctstone wants to merge 1 commit intonock:masterfrom
ctstone:bug/recorder-json-string

Conversation

@ctstone
Copy link
Copy Markdown

@ctstone ctstone commented Sep 21, 2018

Failing test is 'records objects and correctly stores JSON string in body'. See discussion in #1229.

POST body is "foo", a JSON string, but the request is stored as a string literal, foo. When the object is loaded, it fails to match, and the stored response is not served.

t.equal(nock.recorder.play().length, 0);
var options = {
method: 'POST'
, host: 'google.com'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you please not post to an external server, instead spawn a local server for the test? See #1077 for more information, see #1083 for an example pull request doing just that

@ctstone
Copy link
Copy Markdown
Author

ctstone commented Sep 22, 2018 via email

@gr2m
Copy link
Copy Markdown
Member

gr2m commented Sep 24, 2018

Makes sense! Maybe consider adding a comment to existing test cases that use external servers so that that behavior is not copied to new tests?

That’s a great idea! Will do! /cc @RichardLitt

@RichardLitt
Copy link
Copy Markdown
Member

Agreed! @ctstone Any chance you want to help log those comments, so that we know where you took the example from?

@stale
Copy link
Copy Markdown

stale bot commented Mar 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

@paulmelnikow
Copy link
Copy Markdown
Member

I've refactored this to use async and got and rebased it, open as #1546. Thanks again for the legwork, @ctstone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants